diff mbox series

net: bridge: Slightly optimize 'find_portno()'

Message ID 00c39d09c8df7ad0673bf2043f6566d6ef08b789.1636916479.git.christophe.jaillet@wanadoo.fr (mailing list archive)
State Accepted
Commit cc0be1ad686fb29a4d127948486f40b17fb34b50
Delegated to: Netdev Maintainers
Headers show
Series net: bridge: Slightly optimize 'find_portno()' | expand

Checks

Context Check Description
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix warning Target tree name not specified in the subject
netdev/cover_letter success Single patches do not need cover letters
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers success CCed 6 of 6 maintainers
netdev/build_clang success Errors and warnings before: 0 this patch: 0
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 14 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
netdev/tree_selection success Guessing tree name failed - patch did not apply

Commit Message

Christophe JAILLET Nov. 14, 2021, 7:02 p.m. UTC
The 'inuse' bitmap is local to this function. So we can use the
non-atomic '__set_bit()' to save a few cycles.

While at it, also remove some useless {}.

Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
 net/bridge/br_if.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Comments

Dan Carpenter Nov. 15, 2021, 12:35 p.m. UTC | #1
On Sun, Nov 14, 2021 at 08:02:35PM +0100, Christophe JAILLET wrote:
> The 'inuse' bitmap is local to this function. So we can use the
> non-atomic '__set_bit()' to save a few cycles.
> 
> While at it, also remove some useless {}.

I like the {} and tend to add it in new code.  There isn't a rule about
this one way or the other.

regards,
dan carpenter
Nikolay Aleksandrov Nov. 15, 2021, 12:57 p.m. UTC | #2
On 14/11/2021 21:02, Christophe JAILLET wrote:
> The 'inuse' bitmap is local to this function. So we can use the
> non-atomic '__set_bit()' to save a few cycles.
> 
> While at it, also remove some useless {}.
> 
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
>  net/bridge/br_if.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
> index c1183fef1f21..64b2d4fb50f5 100644
> --- a/net/bridge/br_if.c
> +++ b/net/bridge/br_if.c
> @@ -397,10 +397,10 @@ static int find_portno(struct net_bridge *br)
>  	if (!inuse)
>  		return -ENOMEM;
>  
> -	set_bit(0, inuse);	/* zero is reserved */
> -	list_for_each_entry(p, &br->port_list, list) {
> -		set_bit(p->port_no, inuse);
> -	}
> +	__set_bit(0, inuse);	/* zero is reserved */
> +	list_for_each_entry(p, &br->port_list, list)
> +		__set_bit(p->port_no, inuse);
> +
>  	index = find_first_zero_bit(inuse, BR_MAX_PORTS);
>  	bitmap_free(inuse);
>  
> 

This should be targeted at net-next.
The patch itself looks ok, TBH it's a slow path so speed
doesn't really matter but it's a straight-forward change.

Acked-by: Nikolay Aleksandrov <nikolay@nvidia.com>
patchwork-bot+netdevbpf@kernel.org Nov. 15, 2021, 2:10 p.m. UTC | #3
Hello:

This patch was applied to netdev/net-next.git (master)
by David S. Miller <davem@davemloft.net>:

On Sun, 14 Nov 2021 20:02:35 +0100 you wrote:
> The 'inuse' bitmap is local to this function. So we can use the
> non-atomic '__set_bit()' to save a few cycles.
> 
> While at it, also remove some useless {}.
> 
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> 
> [...]

Here is the summary with links:
  - net: bridge: Slightly optimize 'find_portno()'
    https://git.kernel.org/netdev/net-next/c/cc0be1ad686f

You are awesome, thank you!
Christophe JAILLET Nov. 15, 2021, 6:35 p.m. UTC | #4
Le 15/11/2021 à 13:35, Dan Carpenter a écrit :
> On Sun, Nov 14, 2021 at 08:02:35PM +0100, Christophe JAILLET wrote:
>> The 'inuse' bitmap is local to this function. So we can use the
>> non-atomic '__set_bit()' to save a few cycles.
>>
>> While at it, also remove some useless {}.
> 
> I like the {} and tend to add it in new code.  There isn't a rule about
> this one way or the other.
> 
> regards,
> dan carpenter
> 
> 
> 

Hi Dan,

- checkpatch prefers the style without {}
- Usually, greg k-h and Joe Perches give feed-back that extra {} should 
be removed.
- in https://www.kernel.org/doc/html/v5.13/process/coding-style.html, 
after "Rationale: K&R":
    "Do not unnecessarily use braces where a single statement will do."


My own preference is to have {}. It is the standard used on another 
project I work on (i.e. httpd) and it helps when you add some code (it 
avoids unexpected behavior if you forget to add some missing {})

My understanding is that on the HUGE code base of Linux, emphasis is put 
on saving some lines of code, reducing the length of lines and avoiding 
the need to read some extra char.
I'm also fine with it.

CJ
Dan Carpenter Nov. 16, 2021, 8:32 a.m. UTC | #5
On Mon, Nov 15, 2021 at 07:35:48PM +0100, Christophe JAILLET wrote:
> Le 15/11/2021 à 13:35, Dan Carpenter a écrit :
> > On Sun, Nov 14, 2021 at 08:02:35PM +0100, Christophe JAILLET wrote:
> > > The 'inuse' bitmap is local to this function. So we can use the
> > > non-atomic '__set_bit()' to save a few cycles.
> > > 
> > > While at it, also remove some useless {}.
> > 
> > I like the {} and tend to add it in new code.  There isn't a rule about
> > this one way or the other.
> > 

[ heavily snipped ]

> 
> - checkpatch prefers the style without {}

Not for these.

> - Usually, greg k-h and Joe Perches give feed-back that extra {} should be
> removed.

I can't find any reference to that.

> - in https://www.kernel.org/doc/html/v5.13/process/coding-style.html, after
> "Rationale: K&R":
>    "Do not unnecessarily use braces where a single statement will do."

There are exceptions for readability.  For example, mutiline indents
get it whether they need or not.  Do while statements get braces.

Quite a lot of people don't use braces for list_for_each() unless it's
required, definitely, but I think it's allowable.

regards,
dan carpenter
diff mbox series

Patch

diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
index c1183fef1f21..64b2d4fb50f5 100644
--- a/net/bridge/br_if.c
+++ b/net/bridge/br_if.c
@@ -397,10 +397,10 @@  static int find_portno(struct net_bridge *br)
 	if (!inuse)
 		return -ENOMEM;
 
-	set_bit(0, inuse);	/* zero is reserved */
-	list_for_each_entry(p, &br->port_list, list) {
-		set_bit(p->port_no, inuse);
-	}
+	__set_bit(0, inuse);	/* zero is reserved */
+	list_for_each_entry(p, &br->port_list, list)
+		__set_bit(p->port_no, inuse);
+
 	index = find_first_zero_bit(inuse, BR_MAX_PORTS);
 	bitmap_free(inuse);