* [PATCH v2 bpf-next] libbpf: Use sysconf to simplify libbpf_num_possible_cpus
@ 2021-09-22 7:07 Muhammad Falak R Wani
2021-09-22 21:22 ` Daniel Borkmann
0 siblings, 1 reply; 4+ messages in thread
From: Muhammad Falak R Wani @ 2021-09-22 7:07 UTC (permalink / raw)
To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
Cc: Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, netdev, bpf, linux-kernel, Muhammad Falak R Wani
Simplify libbpf_num_possible_cpus by using sysconf(_SC_NPROCESSORS_CONF)
instead of parsing a file.
This patch is a part ([0]) of libbpf-1.0 milestone.
[0] Closes: https://github.com/libbpf/libbpf/issues/383
Signed-off-by: Muhammad Falak R Wani <falakreyaz@gmail.com>
---
tools/lib/bpf/libbpf.c | 17 ++++-------------
1 file changed, 4 insertions(+), 13 deletions(-)
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index ef5db34bf913..f1c0abe5b58d 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -10898,25 +10898,16 @@ int parse_cpu_mask_file(const char *fcpu, bool **mask, int *mask_sz)
int libbpf_num_possible_cpus(void)
{
- static const char *fcpu = "/sys/devices/system/cpu/possible";
static int cpus;
- int err, n, i, tmp_cpus;
- bool *mask;
+ int tmp_cpus;
tmp_cpus = READ_ONCE(cpus);
if (tmp_cpus > 0)
return tmp_cpus;
- err = parse_cpu_mask_file(fcpu, &mask, &n);
- if (err)
- return libbpf_err(err);
-
- tmp_cpus = 0;
- for (i = 0; i < n; i++) {
- if (mask[i])
- tmp_cpus++;
- }
- free(mask);
+ tmp_cpus = sysconf(_SC_NPROCESSORS_CONF);
+ if (tmp_cpus < 1)
+ return libbpf_err(-EINVAL);
WRITE_ONCE(cpus, tmp_cpus);
return tmp_cpus;
--
2.17.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v2 bpf-next] libbpf: Use sysconf to simplify libbpf_num_possible_cpus
2021-09-22 7:07 [PATCH v2 bpf-next] libbpf: Use sysconf to simplify libbpf_num_possible_cpus Muhammad Falak R Wani
@ 2021-09-22 21:22 ` Daniel Borkmann
2021-09-22 21:38 ` Andrii Nakryiko
0 siblings, 1 reply; 4+ messages in thread
From: Daniel Borkmann @ 2021-09-22 21:22 UTC (permalink / raw)
To: Muhammad Falak R Wani, Alexei Starovoitov, Andrii Nakryiko
Cc: Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, netdev, bpf, linux-kernel
On 9/22/21 9:07 AM, Muhammad Falak R Wani wrote:
> Simplify libbpf_num_possible_cpus by using sysconf(_SC_NPROCESSORS_CONF)
> instead of parsing a file.
> This patch is a part ([0]) of libbpf-1.0 milestone.
>
> [0] Closes: https://github.com/libbpf/libbpf/issues/383
>
> Signed-off-by: Muhammad Falak R Wani <falakreyaz@gmail.com>
> ---
> tools/lib/bpf/libbpf.c | 17 ++++-------------
> 1 file changed, 4 insertions(+), 13 deletions(-)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index ef5db34bf913..f1c0abe5b58d 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -10898,25 +10898,16 @@ int parse_cpu_mask_file(const char *fcpu, bool **mask, int *mask_sz)
>
> int libbpf_num_possible_cpus(void)
> {
> - static const char *fcpu = "/sys/devices/system/cpu/possible";
> static int cpus;
> - int err, n, i, tmp_cpus;
> - bool *mask;
> + int tmp_cpus;
>
> tmp_cpus = READ_ONCE(cpus);
> if (tmp_cpus > 0)
> return tmp_cpus;
>
> - err = parse_cpu_mask_file(fcpu, &mask, &n);
> - if (err)
> - return libbpf_err(err);
> -
> - tmp_cpus = 0;
> - for (i = 0; i < n; i++) {
> - if (mask[i])
> - tmp_cpus++;
> - }
> - free(mask);
> + tmp_cpus = sysconf(_SC_NPROCESSORS_CONF);
> + if (tmp_cpus < 1)
> + return libbpf_err(-EINVAL);
This approach is unfortunately broken, see also commit e00c7b216f34 ("bpf: fix
multiple issues in selftest suite and samples") for more details:
3) Current selftest suite code relies on sysconf(_SC_NPROCESSORS_CONF) for
retrieving the number of possible CPUs. This is broken at least in our
scenario and really just doesn't work.
glibc tries a number of things for retrieving _SC_NPROCESSORS_CONF.
First it tries equivalent of /sys/devices/system/cpu/cpu[0-9]* | wc -l,
if that fails, depending on the config, it either tries to count CPUs
in /proc/cpuinfo, or returns the _SC_NPROCESSORS_ONLN value instead.
If /proc/cpuinfo has some issue, it returns just 1 worst case. This
oddity is nothing new [1], but semantics/behaviour seems to be settled.
_SC_NPROCESSORS_ONLN will parse /sys/devices/system/cpu/online, if
that fails it looks into /proc/stat for cpuX entries, and if also that
fails for some reason, /proc/cpuinfo is consulted (and returning 1 if
unlikely all breaks down).
While that might match num_possible_cpus() from the kernel in some
cases, it's really not guaranteed with CPU hotplugging, and can result
in a buffer overflow since the array in user space could have too few
number of slots, and on perpcu map lookup, the kernel will write beyond
that memory of the value buffer.
William Tu reported such mismatches:
[...] The fact that sysconf(_SC_NPROCESSORS_CONF) != num_possible_cpu()
happens when CPU hotadd is enabled. For example, in Fusion when
setting vcpu.hotadd = "TRUE" or in KVM, setting ./qemu-system-x86_64
-smp 2, maxcpus=4 ... the num_possible_cpu() will be 4 and sysconf()
will be 2 [2]. [...]
Documentation/cputopology.txt says /sys/devices/system/cpu/possible
outputs cpu_possible_mask. That is the same as in num_possible_cpus(),
so first step would be to fix the _SC_NPROCESSORS_CONF calls with our
own implementation. Later, we could add support to bpf(2) for passing
a mask via CPU_SET(3), for example, to just select a subset of CPUs.
BPF samples code needs this fix as well (at least so that people stop
copying this). Thus, define bpf_num_possible_cpus() once in selftests
and import it from there for the sample code to avoid duplicating it.
The remaining sysconf(_SC_NPROCESSORS_CONF) in samples are unrelated.
Thanks,
Daniel
> WRITE_ONCE(cpus, tmp_cpus);
> return tmp_cpus;
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2 bpf-next] libbpf: Use sysconf to simplify libbpf_num_possible_cpus
2021-09-22 21:22 ` Daniel Borkmann
@ 2021-09-22 21:38 ` Andrii Nakryiko
2021-09-23 2:49 ` Muhammad Falak Wani
0 siblings, 1 reply; 4+ messages in thread
From: Andrii Nakryiko @ 2021-09-22 21:38 UTC (permalink / raw)
To: Daniel Borkmann
Cc: Muhammad Falak R Wani, Alexei Starovoitov, Andrii Nakryiko,
Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Networking, bpf, open list
On Wed, Sep 22, 2021 at 2:22 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 9/22/21 9:07 AM, Muhammad Falak R Wani wrote:
> > Simplify libbpf_num_possible_cpus by using sysconf(_SC_NPROCESSORS_CONF)
> > instead of parsing a file.
> > This patch is a part ([0]) of libbpf-1.0 milestone.
> >
> > [0] Closes: https://github.com/libbpf/libbpf/issues/383
> >
> > Signed-off-by: Muhammad Falak R Wani <falakreyaz@gmail.com>
> > ---
> > tools/lib/bpf/libbpf.c | 17 ++++-------------
> > 1 file changed, 4 insertions(+), 13 deletions(-)
> >
> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > index ef5db34bf913..f1c0abe5b58d 100644
> > --- a/tools/lib/bpf/libbpf.c
> > +++ b/tools/lib/bpf/libbpf.c
> > @@ -10898,25 +10898,16 @@ int parse_cpu_mask_file(const char *fcpu, bool **mask, int *mask_sz)
> >
> > int libbpf_num_possible_cpus(void)
> > {
> > - static const char *fcpu = "/sys/devices/system/cpu/possible";
> > static int cpus;
> > - int err, n, i, tmp_cpus;
> > - bool *mask;
> > + int tmp_cpus;
> >
> > tmp_cpus = READ_ONCE(cpus);
> > if (tmp_cpus > 0)
> > return tmp_cpus;
> >
> > - err = parse_cpu_mask_file(fcpu, &mask, &n);
> > - if (err)
> > - return libbpf_err(err);
> > -
> > - tmp_cpus = 0;
> > - for (i = 0; i < n; i++) {
> > - if (mask[i])
> > - tmp_cpus++;
> > - }
> > - free(mask);
> > + tmp_cpus = sysconf(_SC_NPROCESSORS_CONF);
> > + if (tmp_cpus < 1)
> > + return libbpf_err(-EINVAL);
>
> This approach is unfortunately broken, see also commit e00c7b216f34 ("bpf: fix
> multiple issues in selftest suite and samples") for more details:
Oh, that predates me. Thanks, Daniel!
Sorry, Muhammad, seems like current implementation is there for a
reason and will have to stay. Thanks a lot for working on this,
though. Hopefully you can help with other issues, though.
[...]
>
> Thanks,
> Daniel
>
> > WRITE_ONCE(cpus, tmp_cpus);
> > return tmp_cpus;
> >
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2 bpf-next] libbpf: Use sysconf to simplify libbpf_num_possible_cpus
2021-09-22 21:38 ` Andrii Nakryiko
@ 2021-09-23 2:49 ` Muhammad Falak Wani
0 siblings, 0 replies; 4+ messages in thread
From: Muhammad Falak Wani @ 2021-09-23 2:49 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Daniel Borkmann, Muhammad Falak R Wani, Alexei Starovoitov,
Andrii Nakryiko, Martin KaFai Lau, Song Liu, Yonghong Song,
John Fastabend, KP Singh, Networking, bpf, open list
> > This approach is unfortunately broken, see also commit e00c7b216f34 ("bpf: fix
> > multiple issues in selftest suite and samples") for more details:
>
> Oh, that predates me. Thanks, Daniel!
Thank you Daniel for the context.
>
> Sorry, Muhammad, seems like current implementation is there for a
> reason and will have to stay. Thanks a lot for working on this,
> though. Hopefully you can help with other issues, though.
>
No worries at all, it was a good experience for me & I will
try to help here and there for sure.
Thank you again!
-mfrw
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-09-23 2:49 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-22 7:07 [PATCH v2 bpf-next] libbpf: Use sysconf to simplify libbpf_num_possible_cpus Muhammad Falak R Wani
2021-09-22 21:22 ` Daniel Borkmann
2021-09-22 21:38 ` Andrii Nakryiko
2021-09-23 2:49 ` Muhammad Falak Wani
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).