| Message ID | 20170319222743.GC17015@avx2 |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
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@gmail.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 ;)
From: Alexey Dobriyan <adobriyan@gmail.com> Date: Mon, 20 Mar 2017 01:27:43 +0300 > ->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@gmail.com> I agree with Eric Dumazet that we might have atomicity issues in the future because of this change. Why don't you drop this and resubmit just the other 3 patches which seem to be much less controversial? Thanks.
On Wed, Mar 22, 2017 at 5:09 AM, David Miller <davem@davemloft.net> wrote: > From: Alexey Dobriyan <adobriyan@gmail.com> > Date: Mon, 20 Mar 2017 01:27:43 +0300 > >> ->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@gmail.com> > > I agree with Eric Dumazet that we might have atomicity issues in the > future because of this change. > > Why don't you drop this and resubmit just the other 3 patches which > seem to be much less controversial? Sure, bool patch was last minute change.
--- 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; struct tasklet_struct flush_tasklet; }; --- a/net/core/flow.c +++ b/net/core/flow.c @@ -56,7 +56,7 @@ static void flow_cache_new_hashrnd(unsigned long arg) int i; for_each_possible_cpu(i) - per_cpu_ptr(fc->percpu, i)->hash_rnd_recalc = 1; + per_cpu_ptr(fc->percpu, i)->hash_rnd_recalc = true; fc->rnd_timer.expires = jiffies + FLOW_HASH_RND_PERIOD; add_timer(&fc->rnd_timer); @@ -155,7 +155,7 @@ static void flow_new_hash_rnd(struct flow_cache *fc, struct flow_cache_percpu *fcp) { get_random_bytes(&fcp->hash_rnd, sizeof(u32)); - fcp->hash_rnd_recalc = 0; + fcp->hash_rnd_recalc = false; __flow_cache_shrink(fc, fcp, 0); } @@ -412,7 +412,7 @@ static int flow_cache_cpu_prepare(struct flow_cache *fc, int cpu) pr_err("NET: failed to allocate flow cache sz %u\n", sz); return -ENOMEM; } - fcp->hash_rnd_recalc = 1; + fcp->hash_rnd_recalc = true; fcp->hash_count = 0; tasklet_init(&fcp->flush_tasklet, flow_cache_flush_tasklet, 0); }
->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@gmail.com> --- include/net/flowcache.h | 2 +- net/core/flow.c | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-)