Re: [PATCH bpf-next] bpf: Helper script for running BPF presubmit tests

[KP Singh <kpsingh@kernel.org>]

On Wed, Jan 20, 2021 at 3:01 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 1/18/21 3:19 PM, KP Singh wrote:
> > The script runs the BPF selftests locally on the same kernel image
> > as they would run post submit in the BPF continuous integration
> > framework. The goal of the script is to run selftests locally in the
> > same environment to check if their changes would end up breaking the
> > BPF CI and reduce the back-and-forth between the maintainers and the
> > developers.
> >
> > Signed-off-by: KP Singh <kpsingh@kernel.org>
>
> Looks really nice! I gave it a run, some minor feedback:

Thanks! :)

>
> > ---
> >   tools/testing/selftests/bpf/run_in_vm.sh | 346 +++++++++++++++++++++++
> >   1 file changed, 346 insertions(+)
> >   create mode 100755 tools/testing/selftests/bpf/run_in_vm.sh
> >
> > diff --git a/tools/testing/selftests/bpf/run_in_vm.sh b/tools/testing/selftests/bpf/run_in_vm.sh
> > new file mode 100755
> > index 000000000000..a4f28f5cdd52
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/run_in_vm.sh
> > @@ -0,0 +1,346 @@
> > +#!/bin/bash
> > +# SPDX-License-Identifier: GPL-2.0
> > +
> > +set -u
> > +set -e

[...]

> > +
> > +  KBUILD_OUTPUT=<path_relative_to_cwd> $0 -- ./test_progs -t test_lsm
> > +
> > +Options:
> > +
> > +     -k)             Recompile the kernel with the config for selftests.
> > +     -i)             Update the rootfs image with a newer version.
> > +     -d)             Update the output directory (default: ${OUTPUT_DIR})
>
> Probably best to have a small howto in tools/testing/selftests/bpf/README.rst for
> people to have a 'getting started' point. Initially I forgot the -k, so VM paniced
> on boot, but after that it was working great modulo a small change below.

Makes sense, I totally forgot about the case when one already has a
precompiled kernel
and it does not work well with the image.

Will update the docs.

>
> [...]
> > +main()
> > +{
> > +     local script_dir="$(cd -P -- "$(dirname -- "${BASH_SOURCE[0]}")" && pwd -P)"
> > +     local kernel_checkout=$(realpath "${script_dir}"/../../../../)
> > +     local log_file="$(date +"bpf_selftests.%Y-%m-%d_%H-%M-%S.log")"

[...]

> > +             esac
> > +     done
> > +     shift $((OPTIND -1))
> > +
> > +     if [[ $# -eq 0 ]]; then
> > +             echo "No command specified, will run ${DEFAULT_COMMAND} in the vm"
> > +     else
> > +             command="$@"
> > +     fi
> > +
> > +     local kconfig_file="${OUTPUT_DIR}/latest.config"
> > +     local make_command="make -j KCONFIG_CONFIG=${kconfig_file}"
>
> I had to fix/limit this locally to -j <num-cpus> as otherwise this was OOM killing
> my box. make man page says 'if the -j option is given without an argument, make will
> not limit the number of jobs that can run simultaneously.'

I thought that -j without an option did something smart. I will update
it to be -j `<num_cpus>`
Thanks!

>
> > +
> > +     # Figure out where the kernel is being built.
> > +     # O takes precedence over KBUILD_OUTPUT.
> > +     if [[ "${O:=""}" != "" ]]; then
> > +             if is_rel_path "${O}"; then
> > +                     O="$(realpath "${PWD}/${O}")"
> > +             fi
>
> For future follow-up, would be amazing to auto-grab nightly build of llvm + pahole
> as well. And even further out maybe also to allow cross-compilation + testing on
> other archs. :)

Indeed, these are definitely on the radar :)

- KP

>
> Thanks,
> Daniel