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. 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
