Song Liu <song@xxxxxxxxxx> writes: > 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. That's true, indeed. While putting the patch together I wondering about the need for a simpler / optimized version of for_each_cpu_wrap(). > 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> > Thanks! [...]
