| Message ID | 20221110122128.105214-1-xukuohai@huawei.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 4b45cd81f737d79d0fbfc0d320a1e518e7f0bbf0 |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf-next,v4] bpf: Initialize same number of free nodes for each pcpu_freelist | expand |
On 2022/11/10 20:21, Xu Kuohai wrote: > pcpu_freelist_populate() initializes nr_elems / num_possible_cpus() + 1 > free nodes for some CPUs, and then possibly one CPU with fewer nodes, > followed by remaining cpus with 0 nodes. For example, when nr_elems == 256 > and num_possible_cpus() == 32, CPU 0~27 each gets 9 free nodes, CPU 28 gets > 4 free nodes, CPU 29~31 get 0 free nodes, while in fact each CPU should get > 8 nodes equally. > > This patch initializes nr_elems / num_possible_cpus() free nodes for each > CPU firstly, then allocates the remaining free nodes by one for each CPU > until no free nodes left. > > Signed-off-by: Xu Kuohai <xukuohai@huawei.com> > Acked-by: Yonghong Song <yhs@fb.com> > --- > v4: Remove unneeded min() > v3: Simplify code as suggested by Andrii > v2: Update commit message and add Yonghong's ack > --- > kernel/bpf/percpu_freelist.c | 26 +++++++++++++------------- > 1 file changed, 13 insertions(+), 13 deletions(-) > > diff --git a/kernel/bpf/percpu_freelist.c b/kernel/bpf/percpu_freelist.c > index b6e7f5c5b9ab..27f2c4aff623 100644 > --- a/kernel/bpf/percpu_freelist.c > +++ b/kernel/bpf/percpu_freelist.c > @@ -100,22 +100,22 @@ void pcpu_freelist_populate(struct pcpu_freelist *s, void *buf, u32 elem_size, > u32 nr_elems) > { > struct pcpu_freelist_head *head; > - int i, cpu, pcpu_entries; > + unsigned int cpu, cpu_idx, i, j, n, m; > > - pcpu_entries = nr_elems / num_possible_cpus() + 1; > - i = 0; > + n = nr_elems / num_possible_cpus(); > + m = nr_elems % num_possible_cpus(); > + > + cpu_idx = 0; > > for_each_possible_cpu(cpu) { > -again: > - head = per_cpu_ptr(s->freelist, cpu); > - /* No locking required as this is not visible yet. */ > - pcpu_freelist_push_node(head, buf); > - i++; > - buf += elem_size; > - if (i == nr_elems) > - break; > - if (i % pcpu_entries) > - goto again; > + j = n + (cpu_idx < m ? 1 : 0); > + for (i = 0; i < j; i++) { > + head = per_cpu_ptr(s->freelist, cpu); Better move it out of "i-loop", and rename "j" to a meaningful name to avoid possible misuse. > + /* No locking required as this is not visible yet. */ > + pcpu_freelist_push_node(head, buf); > + buf += elem_size; > + } > + cpu_idx++; > } > } >
On 11/11/2022 11:53 AM, wuqiang wrote: > On 2022/11/10 20:21, Xu Kuohai wrote: >> pcpu_freelist_populate() initializes nr_elems / num_possible_cpus() + 1 >> free nodes for some CPUs, and then possibly one CPU with fewer nodes, >> followed by remaining cpus with 0 nodes. For example, when nr_elems == 256 >> and num_possible_cpus() == 32, CPU 0~27 each gets 9 free nodes, CPU 28 gets >> 4 free nodes, CPU 29~31 get 0 free nodes, while in fact each CPU should get >> 8 nodes equally. >> >> This patch initializes nr_elems / num_possible_cpus() free nodes for each >> CPU firstly, then allocates the remaining free nodes by one for each CPU >> until no free nodes left. >> >> Signed-off-by: Xu Kuohai <xukuohai@huawei.com> >> Acked-by: Yonghong Song <yhs@fb.com> >> --- >> v4: Remove unneeded min() >> v3: Simplify code as suggested by Andrii >> v2: Update commit message and add Yonghong's ack >> --- >> kernel/bpf/percpu_freelist.c | 26 +++++++++++++------------- >> 1 file changed, 13 insertions(+), 13 deletions(-) >> >> diff --git a/kernel/bpf/percpu_freelist.c b/kernel/bpf/percpu_freelist.c >> index b6e7f5c5b9ab..27f2c4aff623 100644 >> --- a/kernel/bpf/percpu_freelist.c >> +++ b/kernel/bpf/percpu_freelist.c >> @@ -100,22 +100,22 @@ void pcpu_freelist_populate(struct pcpu_freelist *s, void *buf, u32 elem_size, >> u32 nr_elems) >> { >> struct pcpu_freelist_head *head; >> - int i, cpu, pcpu_entries; >> + unsigned int cpu, cpu_idx, i, j, n, m; >> - pcpu_entries = nr_elems / num_possible_cpus() + 1; >> - i = 0; >> + n = nr_elems / num_possible_cpus(); >> + m = nr_elems % num_possible_cpus(); >> + >> + cpu_idx = 0; >> for_each_possible_cpu(cpu) { >> -again: >> - head = per_cpu_ptr(s->freelist, cpu); >> - /* No locking required as this is not visible yet. */ >> - pcpu_freelist_push_node(head, buf); >> - i++; >> - buf += elem_size; >> - if (i == nr_elems) >> - break; >> - if (i % pcpu_entries) >> - goto again; >> + j = n + (cpu_idx < m ? 1 : 0); >> + for (i = 0; i < j; i++) { >> + head = per_cpu_ptr(s->freelist, cpu); > > Better move it out of "i-loop", OK, will do > and rename "j" to a meaningful name to avoid > possible misuse. > The loop is short enough to be readable and "j" is not used elsewhere, so I think it's good to keep the name simple. >> + /* No locking required as this is not visible yet. */ >> + pcpu_freelist_push_node(head, buf); >> + buf += elem_size; >> + } >> + cpu_idx++; >> } >> } > > .
On Thu, Nov 10, 2022 at 11:00 PM Xu Kuohai <xukuohai@huawei.com> wrote: > > On 11/11/2022 11:53 AM, wuqiang wrote: > > On 2022/11/10 20:21, Xu Kuohai wrote: > >> pcpu_freelist_populate() initializes nr_elems / num_possible_cpus() + 1 > >> free nodes for some CPUs, and then possibly one CPU with fewer nodes, > >> followed by remaining cpus with 0 nodes. For example, when nr_elems == 256 > >> and num_possible_cpus() == 32, CPU 0~27 each gets 9 free nodes, CPU 28 gets > >> 4 free nodes, CPU 29~31 get 0 free nodes, while in fact each CPU should get > >> 8 nodes equally. > >> > >> This patch initializes nr_elems / num_possible_cpus() free nodes for each > >> CPU firstly, then allocates the remaining free nodes by one for each CPU > >> until no free nodes left. > >> > >> Signed-off-by: Xu Kuohai <xukuohai@huawei.com> > >> Acked-by: Yonghong Song <yhs@fb.com> > >> --- > >> v4: Remove unneeded min() > >> v3: Simplify code as suggested by Andrii > >> v2: Update commit message and add Yonghong's ack > >> --- > >> kernel/bpf/percpu_freelist.c | 26 +++++++++++++------------- > >> 1 file changed, 13 insertions(+), 13 deletions(-) > >> > >> diff --git a/kernel/bpf/percpu_freelist.c b/kernel/bpf/percpu_freelist.c > >> index b6e7f5c5b9ab..27f2c4aff623 100644 > >> --- a/kernel/bpf/percpu_freelist.c > >> +++ b/kernel/bpf/percpu_freelist.c > >> @@ -100,22 +100,22 @@ void pcpu_freelist_populate(struct pcpu_freelist *s, void *buf, u32 elem_size, > >> u32 nr_elems) > >> { > >> struct pcpu_freelist_head *head; > >> - int i, cpu, pcpu_entries; > >> + unsigned int cpu, cpu_idx, i, j, n, m; > >> - pcpu_entries = nr_elems / num_possible_cpus() + 1; > >> - i = 0; > >> + n = nr_elems / num_possible_cpus(); > >> + m = nr_elems % num_possible_cpus(); > >> + > >> + cpu_idx = 0; > >> for_each_possible_cpu(cpu) { > >> -again: > >> - head = per_cpu_ptr(s->freelist, cpu); > >> - /* No locking required as this is not visible yet. */ > >> - pcpu_freelist_push_node(head, buf); > >> - i++; > >> - buf += elem_size; > >> - if (i == nr_elems) > >> - break; > >> - if (i % pcpu_entries) > >> - goto again; > >> + j = n + (cpu_idx < m ? 1 : 0); > >> + for (i = 0; i < j; i++) { > >> + head = per_cpu_ptr(s->freelist, cpu); > > > > Better move it out of "i-loop", > > OK, will do > I did that while applying. Also added Fixes: e19494edab82 ("bpf: introduce percpu_freelist") Please don't forget to add Fixes tag for future patches. Applied to bpf tree. > > and rename "j" to a meaningful name to avoid > > possible misuse. > > > The loop is short enough to be readable and "j" is not used elsewhere, so I > think it's good to keep the name simple. > > >> + /* No locking required as this is not visible yet. */ > >> + pcpu_freelist_push_node(head, buf); > >> + buf += elem_size; > >> + } > >> + cpu_idx++; > >> } > >> } > > > > . >
Hello: This patch was applied to bpf/bpf.git (master) by Andrii Nakryiko <andrii@kernel.org>: On Thu, 10 Nov 2022 07:21:28 -0500 you wrote: > pcpu_freelist_populate() initializes nr_elems / num_possible_cpus() + 1 > free nodes for some CPUs, and then possibly one CPU with fewer nodes, > followed by remaining cpus with 0 nodes. For example, when nr_elems == 256 > and num_possible_cpus() == 32, CPU 0~27 each gets 9 free nodes, CPU 28 gets > 4 free nodes, CPU 29~31 get 0 free nodes, while in fact each CPU should get > 8 nodes equally. > > [...] Here is the summary with links: - [bpf-next,v4] bpf: Initialize same number of free nodes for each pcpu_freelist https://git.kernel.org/bpf/bpf/c/4b45cd81f737 You are awesome, thank you!
On 11/12/2022 4:12 AM, Andrii Nakryiko wrote: > On Thu, Nov 10, 2022 at 11:00 PM Xu Kuohai <xukuohai@huawei.com> wrote: >> >> On 11/11/2022 11:53 AM, wuqiang wrote: >>> On 2022/11/10 20:21, Xu Kuohai wrote: >>>> pcpu_freelist_populate() initializes nr_elems / num_possible_cpus() + 1 >>>> free nodes for some CPUs, and then possibly one CPU with fewer nodes, >>>> followed by remaining cpus with 0 nodes. For example, when nr_elems == 256 >>>> and num_possible_cpus() == 32, CPU 0~27 each gets 9 free nodes, CPU 28 gets >>>> 4 free nodes, CPU 29~31 get 0 free nodes, while in fact each CPU should get >>>> 8 nodes equally. >>>> >>>> This patch initializes nr_elems / num_possible_cpus() free nodes for each >>>> CPU firstly, then allocates the remaining free nodes by one for each CPU >>>> until no free nodes left. >>>> >>>> Signed-off-by: Xu Kuohai <xukuohai@huawei.com> >>>> Acked-by: Yonghong Song <yhs@fb.com> >>>> --- >>>> v4: Remove unneeded min() >>>> v3: Simplify code as suggested by Andrii >>>> v2: Update commit message and add Yonghong's ack >>>> --- >>>> kernel/bpf/percpu_freelist.c | 26 +++++++++++++------------- >>>> 1 file changed, 13 insertions(+), 13 deletions(-) >>>> >>>> diff --git a/kernel/bpf/percpu_freelist.c b/kernel/bpf/percpu_freelist.c >>>> index b6e7f5c5b9ab..27f2c4aff623 100644 >>>> --- a/kernel/bpf/percpu_freelist.c >>>> +++ b/kernel/bpf/percpu_freelist.c >>>> @@ -100,22 +100,22 @@ void pcpu_freelist_populate(struct pcpu_freelist *s, void *buf, u32 elem_size, >>>> u32 nr_elems) >>>> { >>>> struct pcpu_freelist_head *head; >>>> - int i, cpu, pcpu_entries; >>>> + unsigned int cpu, cpu_idx, i, j, n, m; >>>> - pcpu_entries = nr_elems / num_possible_cpus() + 1; >>>> - i = 0; >>>> + n = nr_elems / num_possible_cpus(); >>>> + m = nr_elems % num_possible_cpus(); >>>> + >>>> + cpu_idx = 0; >>>> for_each_possible_cpu(cpu) { >>>> -again: >>>> - head = per_cpu_ptr(s->freelist, cpu); >>>> - /* No locking required as this is not visible yet. */ >>>> - pcpu_freelist_push_node(head, buf); >>>> - i++; >>>> - buf += elem_size; >>>> - if (i == nr_elems) >>>> - break; >>>> - if (i % pcpu_entries) >>>> - goto again; >>>> + j = n + (cpu_idx < m ? 1 : 0); >>>> + for (i = 0; i < j; i++) { >>>> + head = per_cpu_ptr(s->freelist, cpu); >>> >>> Better move it out of "i-loop", >> >> OK, will do >> > > I did that while applying. Also added > > Fixes: e19494edab82 ("bpf: introduce percpu_freelist") > > Please don't forget to add Fixes tag for future patches. > OK, thanks for the kind reminder > Applied to bpf tree. > >>> and rename "j" to a meaningful name to avoid >>> possible misuse. >>> >> The loop is short enough to be readable and "j" is not used elsewhere, so I >> think it's good to keep the name simple. >> >>>> + /* No locking required as this is not visible yet. */ >>>> + pcpu_freelist_push_node(head, buf); >>>> + buf += elem_size; >>>> + } >>>> + cpu_idx++; >>>> } >>>> } >>> >>> . >> > .
diff --git a/kernel/bpf/percpu_freelist.c b/kernel/bpf/percpu_freelist.c index b6e7f5c5b9ab..27f2c4aff623 100644 --- a/kernel/bpf/percpu_freelist.c +++ b/kernel/bpf/percpu_freelist.c @@ -100,22 +100,22 @@ void pcpu_freelist_populate(struct pcpu_freelist *s, void *buf, u32 elem_size, u32 nr_elems) { struct pcpu_freelist_head *head; - int i, cpu, pcpu_entries; + unsigned int cpu, cpu_idx, i, j, n, m; - pcpu_entries = nr_elems / num_possible_cpus() + 1; - i = 0; + n = nr_elems / num_possible_cpus(); + m = nr_elems % num_possible_cpus(); + + cpu_idx = 0; for_each_possible_cpu(cpu) { -again: - head = per_cpu_ptr(s->freelist, cpu); - /* No locking required as this is not visible yet. */ - pcpu_freelist_push_node(head, buf); - i++; - buf += elem_size; - if (i == nr_elems) - break; - if (i % pcpu_entries) - goto again; + j = n + (cpu_idx < m ? 1 : 0); + for (i = 0; i < j; i++) { + head = per_cpu_ptr(s->freelist, cpu); + /* No locking required as this is not visible yet. */ + pcpu_freelist_push_node(head, buf); + buf += elem_size; + } + cpu_idx++; } }