Re: [Devel] [PATCH hci-8.1 v3 1/1] selftests/bpf: run the xsk veth tests in a network namespace of their own

[Konstantin Khorenko <khorenko@virtuozzo.com>]

Hi Eva,

thanks, all comments on v2 are addressed, and the code does what the
message says. What is left is the commit message, the split and one
redundant mount. To save a round, I reworked it myself and am posting
it as v4, three patches:

  1/3 selftests/bpf: xsk: exit with the status test_exit() is given
  2/3 selftests/bpf: xsk: take the fragment limit from the kernel's BTF
  3/3 selftests/bpf: run the xsk veth tests in a network namespace of
      their own

Please check that v4 still says what you meant. Details below.

> test_xsk.sh makes its veth pair in the namespace it runs in. Whatever
> the host does with a new interface, address configuration, neighbour
> discovery, happens on that pair, and the AF_XDP sockets under test
> receive the host's packets among their own. The tests count and inspect
> what they receive, so a preconfigured host fails some of them in every
> mode, and a bare one passes.
>
> Run the veth part in an unnamed network namespace with its own sysfs,
> which drops with its last process whenever the run ends. only -d names
> it, its pair outlives the run. The fragment limit came from the sysctl
> max_skb_frags, which has no file outside the initial namespace. Take it
> from the kernel's BTF as the size of skb_shared_info's frags array, fall
> back to sysctl as before if this info is missing. When the namespace
> can't be created, the tests are skipped, as without veth or ip. Other
> cases after a successful namespace creation fail.

For the veth runs the sysctl fallback cannot work any more:
max_skb_frags is in net_core_table, which is registered for init_net
only, and the veth part now always runs in a namespace of its own.
Sashiko was right about the message here. On our kernels it changes
nothing, BTF is always built in, but "as before" is not true. The
sysctl stays a fallback for kernels without BTF, which in practice
means -i runs in the initial namespace.

"only -d names it, its pair outlives the run" also needs a capital
letter and a "so".

>
> Fix test_exit() return status for skips, as since commit 895b62eed2ab
> ("selftests: xsk: fix reporting of failed tests"), test_exit() exited 1.

This is a fix of its own, with its own Fixes: tag, and mainline needs
the same one-liner: it still has "exit 1" there. As a separate commit
it can later be replaced by its ms/ version; folded into this patch it
cannot.

The BTF part is a fix of its own too: AF_XDP checks MAX_SKB_FRAGS
(xsk_build_skb()), not the sysctl, so a lowered sysctl makes
too_many_frags expect the wrong limit in any namespace.

>
> https://virtuozzo.atlassian.net/browse/VSTOR-150731
> Feature: fix selftests
>
> Signed-off-by: Eva Kurchatova <eva.kurchatova@virtuozzo.com>
> ---
>  tools/testing/selftests/bpf/test_xsk.sh    | 30 +++++++++++++++--
>  tools/testing/selftests/bpf/xsk_prereqs.sh |  2 +-
>  tools/testing/selftests/bpf/xskxceiver.c   | 39 +++++++++++++++++++++-
>  3 files changed, 67 insertions(+), 4 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/test_xsk.sh b/tools/testing/selftests/bpf/test_xsk.sh
> index 65aafe0003db..218528ddcdf8 100755
> --- a/tools/testing/selftests/bpf/test_xsk.sh
> +++ b/tools/testing/selftests/bpf/test_xsk.sh
> @@ -68,7 +68,8 @@
>  # Run with verbose output:
>  #   sudo ./test_xsk.sh -v
>  #
> -# Set up veth interfaces and leave them up so xskxceiver can be launched in a debugger:
> +# Set up veth interfaces in a network namespace and leave them up so
> +# xskxceiver can be launched in a debugger there:
>  #   sudo ./test_xsk.sh -d
>  #
>  # Run test suite for physical device in loopback mode
> @@ -162,8 +163,33 @@ if [ ! -z $ETH ]; then
>       VETH1=${ETH}
>  else
>       validate_root_exec
> -     validate_veth_support ${VETH0}
>       validate_ip_utility
> +     # The host configures and talks on any new interface in its namespace
> +     if [ -z "$XSK_NETNS" ]; then
> +             if ! unshare --net true; then
> +                     echo "skip all tests: cannot create a network namespace" >&2
> +                     test_exit $ksft_skip
> +             fi
> +             if [[ $debug -ne 1 ]]; then
> +                     # Unnamed, it goes with its last process; its own sysfs shows its interfaces
> +                     XSK_NETNS=1 exec unshare --net --mount sh -c \
> +                             'mount --make-rprivate / && mount -t sysfs sysfs /sys &&
> +                             exec bash "$0" "$@"' "$0" "$@"

unshare --mount already sets private propagation on the new mount
namespace by default (--propagation private), so the
"mount --make-rprivate /" step does nothing.

> +             fi
> +             # The pair of a run by hand outlives it, in a namespace with a name
> +             XSK_NETNS=xsk${VETH0_POSTFIX}
> +             ip netns add ${XSK_NETNS} || exit 1
> +             trap "ip netns del ${XSK_NETNS}" EXIT
> +             # This run's veth names were never created; the pair is in the namespace
> +             trap "exit 1" INT
> +             XSK_NETNS=${XSK_NETNS} ip netns exec ${XSK_NETNS} bash "$0" "$@" || exit
> +             trap - EXIT
> +             echo "The pair is up in network namespace ${XSK_NETNS}," \
> +                     "use ip netns exec ${XSK_NETNS};" \
> +                     "ip netns del ${XSK_NETNS} removes it"
> +             exit 0
> +     fi
> +     validate_veth_support ${VETH0}
>       setup_vethPairs
>
>       retval=$?
> diff --git a/tools/testing/selftests/bpf/xsk_prereqs.sh b/tools/testing/selftests/bpf/xsk_prereqs.sh
> index 47c7b8064f38..12176d85cd6c 100755
> --- a/tools/testing/selftests/bpf/xsk_prereqs.sh
> +++ b/tools/testing/selftests/bpf/xsk_prereqs.sh
> @@ -50,7 +50,7 @@ test_exit()
>       if [ $1 -ne 0 ]; then
>               test_status $1 $(basename $0)
>       fi
> -     exit 1
> +     exit $1
>  }
>
>  cleanup_iface()
> diff --git a/tools/testing/selftests/bpf/xskxceiver.c b/tools/testing/selftests/bpf/xskxceiver.c
> index 11f047b8af75..a4a0b4e2516c 100644
> --- a/tools/testing/selftests/bpf/xskxceiver.c
> +++ b/tools/testing/selftests/bpf/xskxceiver.c
> @@ -103,6 +103,7 @@
>  #include "xsk.h"
>  #include "xskxceiver.h"
>  #include <bpf/bpf.h>
> +#include <bpf/btf.h>
>  #include <linux/filter.h>
>  #include "../kselftest.h"
>  #include "xsk_xdp_common.h"
> @@ -325,12 +326,48 @@ static bool ifobj_zc_avail(struct ifobject *ifobject)
>       return zc_avail;
>  }
>
> +/* AF_XDP checks MAX_SKB_FRAGS, which the sysctl only follows by default */
> +static unsigned int btf_max_skb_frags(void)
> +{
> +     unsigned int max_skb_frags = 0;
> +     const struct btf_member *m;
> +     const struct btf_type *t;
> +     struct btf *btf;
> +     int id, i;
> +
> +     btf = btf__load_vmlinux_btf();
> +     if (!btf)
> +             return 0;
> +
> +     id = btf__find_by_name_kind(btf, "skb_shared_info", BTF_KIND_STRUCT);
> +     if (id < 0)
> +             goto out;
> +
> +     t = btf__type_by_id(btf, id);
> +     m = btf_members(t);
> +     for (i = 0; i < btf_vlen(t); i++, m++) {
> +             if (strcmp(btf__name_by_offset(btf, m->name_off), "frags"))
> +                     continue;
> +             t = btf__type_by_id(btf, m->type);
> +             if (btf_is_array(t))
> +                     max_skb_frags = btf_array(t)->nelems;
> +             break;
> +     }
> +out:
> +     btf__free(btf);
> +     return max_skb_frags;
> +}
> +
>  #define MAX_SKB_FRAGS_PATH "/proc/sys/net/core/max_skb_frags"
>  static unsigned int get_max_skb_frags(void)
>  {
> -     unsigned int max_skb_frags = 0;
> +     unsigned int max_skb_frags = btf_max_skb_frags();
>       FILE *file;
>
> +     if (max_skb_frags)
> +             return max_skb_frags;
> +
> +     ksft_print_msg("No MAX_SKB_FRAGS in the kernel's BTF, reading %s\n", MAX_SKB_FRAGS_PATH);
>       file = fopen(MAX_SKB_FRAGS_PATH, "r");
>       if (!file) {
>               ksft_print_msg("Error opening %s\n", MAX_SKB_FRAGS_PATH);
>
[ ... ]

Mainline
--------

VSTOR-151679 needs a few updates:

- "without the -f option vz adds" is stale: v4 has no -f.
- Order: the test_exit() fix first, it stands alone and carries a
  Fixes: tag; then BTF; then the namespace.
- Mainline reads the sysctl in two places, xskxceiver.c and
  prog_tests/xsk.c (both via read_procfs_val(MAX_SKB_FRAGS_PATH), both
  fall back to 17). Put the BTF helper into prog_tests/test_xsk.c,
  which both use, and replace both reads.
- prog_tests/xdp_adjust_frags.c reads the same sysctl. It is a
  different test, but expect the question in review.
- In mainline the veth runs already exist in test_progs, in a namespace
  of their own (test_ns_xsk_skb, test_ns_xsk_drv). The reviewers may
  prefer dropping the veth mode from test_xsk.sh to isolating it. Look
  at what test_xsk.sh still covers that test_progs does not, busy poll
  for instance, and say it in the cover letter.

--
Best regards,

Konstantin Khorenko,
Virtuozzo Linux Kernel Team

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel