On Tue, Jan 26, 2021 at 3:10 AM Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> wrote: > > On Fri, Jan 22, 2021 at 4:44 PM KP Singh <kpsingh@xxxxxxxxxx> 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 allow contributors 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@xxxxxxxxxx> > > --- > > This is great, thanks a lot for working on this! This is great > especially for ad-hoc contributors who don't have qemu workflow setup. > Below are some comments for the extra polish :) > > 1. There is this long list output at the beginning: > > https://libbpf-vmtest.s3-us-west-1.amazonaws.com/x86_64/vmlinux-5.5.0.zst > https://libbpf-vmtest.... > > Can we omit that? > > 2. Then something is re-downloaded every single time: > > % Total % Received % Xferd Average Speed Time Time Time Current > Dload Upload Total Spent Left Speed > 100 77713 100 77713 0 0 509k 0 --:--:-- --:--:-- --:--:-- 512k > > Unless it's to check if something newer appeared in S3, would be nice > to skip that step. > > 3. Every single time I run the script it actually rebuilds kernel. > Somehow Linux Makefile's logic to do nothing if nothing changed in > Linux source code doesn't kick in, I wonder why? It's quite annoying > and time-consuming for frequent selftest reruns. What's weird is that > individual .o's are not re-built, but kernel is still re-linked and > BTF is re-generated, which is the slow part :( > > 4. Selftests are re-built from scratch every single time, even if > nothing changed. Again, strange because they won't do it normally. And > given there is a fixed re-usable .bpf_selftests "cache directory", we > should be able to set everything up so that no extra compilation is > performed, no? > > 5. Before VM is started there is: > > > #!/bin/bash > > { > > cd /root/bpf > echo ./test_progs > ./test_progs > } 2>&1 | tee /root/bpf_selftests.2021-01-25_17-56-11.log > poweroff -f > > > Which is probably useful in rare cases for debugging purposes, but is > just distracting in common case. Would it be able to have verbose flag > for your script that would omit output like this by default? > > 6. Was too lazy to check, but once VM boots and before specified > command is run, there is a bunch of verbose script echoing: > > + for path in /etc/rcS.d/S* > > If that's part of libbpf CI's image, let's fix it there. If not, let's > fix it in your script? > > 7. Is it just me, or when ./test_progs is run inside VM, it's output > is somehow heavily buffered and delayed? I get no output for a while, > and then a whole bunch of lines with already passed tests. Curious if > anyone else noticed that as well. When I run the same image locally > and manually (not through your script), I don't have this issue. > > 8. I noticed that even if the command succeeds (e.g., ./test_progs in > my case), the script exits with non-zero error code (32 in my case). > That's suboptimal, because you can't use that script to detect test > failures. > > But again, it's the polish feedback, great work! > > > tools/testing/selftests/bpf/run_in_vm.sh | 353 +++++++++++++++++++++++ > > 1 file changed, 353 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..09bb9705acb3 > > --- /dev/null > > +++ b/tools/testing/selftests/bpf/run_in_vm.sh > > @@ -0,0 +1,353 @@ > > +#!/bin/bash > > +# SPDX-License-Identifier: GPL-2.0 > > + > > +set -u > > +set -e > > + > > +QEMU_BINARY="${QEMU_BINARY:="qemu-system-x86_64"}" > > +X86_BZIMAGE="arch/x86/boot/bzImage" > > Might be worth it to mention that this only works with x86_64 (due to > image restrictions at least, right?). Thanks, I added a comment here. I really hope someone can contribute support for other architectures too :) > > > +DEFAULT_COMMAND="./test_progs" > > +MOUNT_DIR="mnt" > > +ROOTFS_IMAGE="root.img" > > +OUTPUT_DIR="$HOME/.bpf_selftests" > > +KCONFIG_URL="https://raw.githubusercontent.com/libbpf/libbpf/master/travis-ci/vmtest/configs/latest.config"; > > +INDEX_URL="https://raw.githubusercontent.com/libbpf/libbpf/master/travis-ci/vmtest/configs/INDEX"; > > +NUM_COMPILE_JOBS="$(nproc)" > > + > > +usage() > > +{ > > + cat <<EOF > > +Usage: $0 [-k] [-i] [-d <output_dir>] -- [<command>] > > + > > +<command> is the command you would normally run when you are in > > +tools/testing/selftests/bpf. e.g: > > + > > + $0 -- ./test_progs -t test_lsm > > + > > +If no command is specified, "${DEFAULT_COMMAND}" will be run by > > +default. > > + > > +If you build your kernel using KBUILD_OUTPUT= or O= options, these > > +can be passed as environment variables to the script: > > + > > + O=<path_relative_to_cwd> $0 -- ./test_progs -t test_lsm > > "relative_to_cwd" is a bit misleading, it could be an absolute path as > well, I presume. So I'd just say "O=<kernel_build_path>" or something > along those lines. Sorry, I missed this in my other reply, updated. > > > + > > +or > > + > > + KBUILD_OUTPUT=<path_relative_to_cwd> $0 -- ./test_progs -t test_lsm > > + > > [...]
