Re: [PATCH bpf-next 2/2] selftests/bpf: Add selftest for bits iter

[Yafang Shao <laoar.shao@gmail.com>]

On Fri, Feb 23, 2024 at 1:36 AM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Sun, Feb 18, 2024 at 3:49 AM Yafang Shao <laoar.shao@gmail.com> wrote:
> >
> > Add selftests for the newly added bits iter.
> > - bits_iter_success
> >   - The number of CPUs should be expected when iterating over a cpumask
> >   - percpu data extracted from the percpu struct should be expected
> >   - RCU lock is not required
> >   - It is fine without calling bpf_iter_cpumask_next()
> >   - It can work as expected when invalid arguments are passed
> >
> > - bits_iter_failure
> >   - bpf_iter_bits_destroy() is required after calling
> >     bpf_iter_bits_new()
> >   - bpf_iter_bits_destroy() can only destroy an initialized iter
> >   - bpf_iter_bits_next() must use an initialized iter
> >
> > Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> > ---
> >  tools/testing/selftests/bpf/config            |   1 +
> >  .../selftests/bpf/prog_tests/bits_iter.c      | 180 ++++++++++++++++++
> >  .../bpf/progs/test_bits_iter_failure.c        |  53 ++++++
> >  .../bpf/progs/test_bits_iter_success.c        | 146 ++++++++++++++
> >  4 files changed, 380 insertions(+)
> >  create mode 100644 tools/testing/selftests/bpf/prog_tests/bits_iter.c
> >  create mode 100644 tools/testing/selftests/bpf/progs/test_bits_iter_failure.c
> >  create mode 100644 tools/testing/selftests/bpf/progs/test_bits_iter_success.c
> >
> > diff --git a/tools/testing/selftests/bpf/config b/tools/testing/selftests/bpf/config
> > index 01f241ea2c67..dd4b0935e35f 100644
> > --- a/tools/testing/selftests/bpf/config
> > +++ b/tools/testing/selftests/bpf/config
> > @@ -78,6 +78,7 @@ CONFIG_NF_CONNTRACK_MARK=y
> >  CONFIG_NF_DEFRAG_IPV4=y
> >  CONFIG_NF_DEFRAG_IPV6=y
> >  CONFIG_NF_NAT=y
> > +CONFIG_PSI=y
> >  CONFIG_RC_CORE=y
> >  CONFIG_SECURITY=y
> >  CONFIG_SECURITYFS=y
> > diff --git a/tools/testing/selftests/bpf/prog_tests/bits_iter.c b/tools/testing/selftests/bpf/prog_tests/bits_iter.c
> > new file mode 100644
> > index 000000000000..778a7c942dba
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/prog_tests/bits_iter.c
> > @@ -0,0 +1,180 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/* Copyright (c) 2024 Yafang Shao <laoar.shao@gmail.com> */
> > +
> > +#define _GNU_SOURCE
> > +#include <sched.h>
> > +
> > +#include <test_progs.h>
> > +#include "test_bits_iter_success.skel.h"
> > +#include "test_bits_iter_failure.skel.h"
> > +#include "cgroup_helpers.h"
> > +
> > +static const char * const positive_testcases[] = {
> > +       "cpumask_iter",
> > +};
> > +
> > +static const char * const negative_testcases[] = {
> > +       "null_pointer",
> > +       "zero_bit",
> > +       "no_mem",
> > +       "invalid_bits"
> > +};
> > +
> > +static int read_percpu_data(struct bpf_link *link, int nr_cpu_exp, int nr_running_exp)
> > +{
> > +       int iter_fd, len, item, nr_running, psi_running, nr_cpus, err = -1;
> > +       char buf[128];
> > +       size_t left;
> > +       char *p;
> > +
> > +       iter_fd = bpf_iter_create(bpf_link__fd(link));
> > +       if (!ASSERT_GE(iter_fd, 0, "iter_fd"))
> > +               return -1;
> > +
> > +       memset(buf, 0, sizeof(buf));
> > +       left = ARRAY_SIZE(buf);
> > +       p = buf;
> > +       while ((len = read(iter_fd, p, left)) > 0) {
> > +               p += len;
> > +               left -= len;
> > +       }
> > +
> > +       item = sscanf(buf, "nr_running %u nr_cpus %u psi_running %u\n",
> > +                     &nr_running, &nr_cpus, &psi_running);
> > +       if (!ASSERT_EQ(item, 3, "seq_format"))
> > +               goto out;
> > +       if (!ASSERT_EQ(nr_cpus, nr_cpu_exp, "nr_cpus"))
> > +               goto out;
> > +       if (!ASSERT_GE(nr_running, nr_running_exp, "nr_running"))
> > +               goto out;
> > +       if (!ASSERT_GE(psi_running, nr_running_exp, "psi_running"))
> > +               goto out;
> > +
> > +       err = 0;
> > +out:
> > +       close(iter_fd);
> > +       return err;
> > +}
>
> ..
> > +
> > +       /* Case 1): Enable all possible CPUs */
> > +       CPU_ZERO(&set);
> > +       for (i = 0; i < nr_cpus; i++)
> > +               CPU_SET(i, &set);
> > +       err = sched_setaffinity(skel->bss->pid, sizeof(set), &set);
> > +       if (!ASSERT_OK(err, "setaffinity_all_cpus"))
> > +               goto free_link;
> > +       err = read_percpu_data(link, nr_cpus, 1);
> > +       if (!ASSERT_OK(err, "read_percpu_data"))
> > +               goto free_link;
>
> The patch 1 looks good, but this test fails on s390.
>
> read_percpu_data:FAIL:nr_cpus unexpected nr_cpus: actual 0 != expected 2
> verify_iter_success:FAIL:read_percpu_data unexpected error: -1 (errno 95)
>
> Please see CI.
>
> So either add it to DENYLIST.s390x in the same commit or make it work.
>
> pw-bot: cr

The reason for the failure on s390x architecture is currently unclear.
One plausible explanation is that total_nr_cpus remains 0 when
executing the following code:

    bpf_for_each(bits, cpu, p->cpus_ptr, total_nr_cpus)

This is despite setting total_nr_cpus to the value obtained from
libbpf_num_possible_cpus():

    skel->bss->total_nr_cpus = libbpf_num_possible_cpus();

A potential workaround could involve using a hardcoded number of CPUs,
such as 8192, instead of relying on total_nr_cpus. This approach might
mitigate the issue temporarily.

-- 
Regards
Yafang