On Thu, Sep 8, 2022 at 3:45 AM Punit Agrawal <punit.agrawal@xxxxxxxxxxxxx> wrote: > > Hi Song, > > Thanks for taking a look. > > Song Liu <song@xxxxxxxxxx> writes: > > > On Wed, Sep 7, 2022 at 8:58 AM Punit Agrawal > > <punit.agrawal@xxxxxxxxxxxxx> wrote: > >> > >> In the percpu freelist code, it is a common pattern to iterate over > >> the possible CPUs mask starting with the current CPU. The pattern is > >> implemented using a hand rolled while loop with the loop variable > >> increment being open-coded. > >> > >> Simplify the code by using for_each_cpu_wrap() helper to iterate over > >> the possible cpus starting with the current CPU. As a result, some of > >> the special-casing in the loop also gets simplified. > >> > >> No functional change intended. > >> > >> Signed-off-by: Punit Agrawal <punit.agrawal@xxxxxxxxxxxxx> > >> --- > >> v1 -> v2: > >> * Fixed the incorrect transformation changing semantics of __pcpu_freelist_push_nmi() > >> > >> Previous version - > >> v1: https://lore.kernel.org/all/20220817130807.68279-1-punit.agrawal@xxxxxxxxxxxxx/ > >> > >> kernel/bpf/percpu_freelist.c | 48 ++++++++++++------------------------ > >> 1 file changed, 16 insertions(+), 32 deletions(-) > >> > >> diff --git a/kernel/bpf/percpu_freelist.c b/kernel/bpf/percpu_freelist.c > >> index 00b874c8e889..b6e7f5c5b9ab 100644 > >> --- a/kernel/bpf/percpu_freelist.c > >> +++ b/kernel/bpf/percpu_freelist.c > >> @@ -58,23 +58,21 @@ static inline void ___pcpu_freelist_push_nmi(struct pcpu_freelist *s, > >> { > >> int cpu, orig_cpu; > >> > >> - orig_cpu = cpu = raw_smp_processor_id(); > >> + orig_cpu = raw_smp_processor_id(); > >> while (1) { > >> - struct pcpu_freelist_head *head; > >> + for_each_cpu_wrap(cpu, cpu_possible_mask, orig_cpu) { > >> + struct pcpu_freelist_head *head; > >> > >> - head = per_cpu_ptr(s->freelist, cpu); > >> - if (raw_spin_trylock(&head->lock)) { > >> - pcpu_freelist_push_node(head, node); > >> - raw_spin_unlock(&head->lock); > >> - return; > >> + head = per_cpu_ptr(s->freelist, cpu); > >> + if (raw_spin_trylock(&head->lock)) { > >> + pcpu_freelist_push_node(head, node); > >> + raw_spin_unlock(&head->lock); > >> + return; > >> + } > >> } > >> - cpu = cpumask_next(cpu, cpu_possible_mask); > >> - if (cpu >= nr_cpu_ids) > >> - cpu = 0; > > > > I personally don't like nested loops here. Maybe we can keep > > the original while loop and use cpumask_next_wrap()? > > Out of curiosity, is there a reason to avoid nesting here? The nested > loop avoids the "cpu == orig_cpu" unnecessary check every iteration. for_each_cpu_wrap is a more complex loop, so we are using some checks either way. OTOH, the nesting is not too deep (two loops then one if), so I guess current version is fine. Acked-by: Song Liu <song@xxxxxxxxxx> > > As suggested, it's possible to use cpumask_next_wrap() like below - > > diff --git a/kernel/bpf/percpu_freelist.c b/kernel/bpf/percpu_freelist.c > index 00b874c8e889..19e8eab70c40 100644 > --- a/kernel/bpf/percpu_freelist.c > +++ b/kernel/bpf/percpu_freelist.c > @@ -68,9 +68,7 @@ static inline void ___pcpu_freelist_push_nmi(struct pcpu_freelist *s, > raw_spin_unlock(&head->lock); > return; > } > - cpu = cpumask_next(cpu, cpu_possible_mask); > - if (cpu >= nr_cpu_ids) > - cpu = 0; > + cpu = cpumask_next_wrap(cpu, cpu_possible_mask, orig_cpu, false); > > /* cannot lock any per cpu lock, try extralist */ > if (cpu == orig_cpu && > > > I can send an updated patch if this is preferred. > > Thanks, > Punit
