netdev - Re: [PATCH 3/4] flowcache: make struct flow_cache_percpu::hash_rnd

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sun, 19 Mar 2017 16:11:59 -0700
From:   Eric Dumazet <eric.dumazet@...il.com>
To:     Alexey Dobriyan <adobriyan@...il.com>
Cc:     steffen.klassert@...unet.com, herbert@...dor.apana.org.au,
        davem@...emloft.net, netdev@...r.kernel.org
Subject: Re: [PATCH 3/4] flowcache: make struct
 flow_cache_percpu::hash_rnd_recalc bool

On Mon, 2017-03-20 at 01:27 +0300, Alexey Dobriyan wrote:
> ->hash_rnd_recalc is only used in boolean context.
> 
> Space savings on x86_64 come from the fact that "MOV rm8, imm8" is
> shorter than "MOV rm32, imm32" by at least 3 bytes.
> 
> 	add/remove: 0/0 grow/shrink: 0/3 up/down: 0/-10 (-10)
> 	function                                     old     new   delta
> 	flow_cache_new_hashrnd                       166     163      -3
> 	flow_cache_cpu_up_prep                       171     168      -3
> 	flow_cache_lookup                           1148    1144      -4
> 	Total: Before=170822872, After=170822862, chg -0.00%
> 
> Signed-off-by: Alexey Dobriyan <adobriyan@...il.com>
> ---
> 
>  include/net/flowcache.h |    2 +-
>  net/core/flow.c         |    6 +++---
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> --- a/include/net/flowcache.h
> +++ b/include/net/flowcache.h
> @@ -10,7 +10,7 @@ struct flow_cache_percpu {
>  	struct hlist_head		*hash_table;
>  	int				hash_count;
>  	u32				hash_rnd;
> -	int				hash_rnd_recalc;
> +	bool				hash_rnd_recalc;

hash_rnd_recalc can be written from flow_cache_new_hashrnd() without any
locking.

Some arches do not have the ability to store an u8 atomically.

So your patch adds a possibility that in the future, we might have a
bug, if another field is added there.

Basically the whole integer here should be reserved, or converted to a
real flag (that can be manipulated with clear_bit() and set_bit()),
but this would consume 8 bytes instead of 4 ;)