Re: [PATCH bpf-next v2 1/2] bpf: Helper script for running BPF presubmit tests

[KP Singh <kpsingh@kernel.org>]

[...]

> >
> > >
> > > 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.
> >
> > This is the kernel config. I wonder how we could check if there is something
> > new without downloading it, the file is called "latest.config".
> >
> > Maybe this is something we can add to the URL index as well in format similar
> >  to the image. But since it's just a config file I am not sure
> > it's worth the extra effort.
>
> Curl supports the following option. Given we have a local cache in
> .bpf_selftests, check if it already has .config and pass it as -z
> '.bpf_selftests/.config'? Would be nice, if it works out. If not, I
> agree, config is small enough to not go to great lengths to avoid
> downloading it.
>
> -z/--time-cond <date expression>
>
> (HTTP/FTP) Request a file that has been modified later than the given
> time and date, or one that has been modified before that time. The
> date expression can be all sorts of date strings or if it doesn't
> match any internal ones, it tries to get the time from a given file
> name instead.

This does not work with the github github raw URL so I had to do something like:

diff --git a/tools/testing/selftests/bpf/run_in_vm.sh
b/tools/testing/selftests/bpf/run_in_vm.sh
index 46fbb0422e9e..132017981776 100755
--- a/tools/testing/selftests/bpf/run_in_vm.sh
+++ b/tools/testing/selftests/bpf/run_in_vm.sh
@@ -14,6 +14,7 @@ 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"
+KCONFIG_API_URL="https://api.github.com/repos/libbpf/libbpf/contents/travis-ci/vmtest/configs/latest.config"
 INDEX_URL="https://raw.githubusercontent.com/libbpf/libbpf/master/travis-ci/vmtest/configs/INDEX"
 NUM_COMPILE_JOBS="$(nproc)"

@@ -236,6 +237,27 @@ is_rel_path()
        [[ ${path:0:1} != "/" ]]
 }

+update_kconfig()
+{
+       local kconfig_file="$1"
+       local update_command="curl -sLf ${KCONFIG_URL} -o ${kconfig_file}"
+       # Github does not return the "last-modified" header when
retrieving the raw contents of the file.
+       # Use the API call to get the last-modified time of the kernel
config and only update the config if
+       # it has been updated after the previously cached config was
created. This avoids unnecessarily
+       # compiling the kernel and selftests.
+       if [[ -f "${kconfig_file}" ]]; then
+               local last_modified_date="$(curl -sL -D -
"${KCONFIG_API_URL}" -o /dev/null | grep "last-modified" | awk -F ': '
'{print $2}')"
+               local remote_modified_timestamp="$(date -d
"${last_modified_date}" +"%s")"
+               local local_creation_timestamp="$(stat -c %W "${kconfig_file}")"
+
+               if [[ "${remote_modified_timestamp}" -gt
"${local_creation_timestamp}" ]]; then
+                       ${update_command}
+               fi
+       else
+               ${update_command}
+       fi
+}
+
 main()
 {
        local script_dir="$(cd -P -- "$(dirname --
"${BASH_SOURCE[0]}")" && pwd -P)"
@@ -314,7 +336,7 @@ main()

        mkdir -p "${OUTPUT_DIR}"
        mkdir -p "${mount_dir}"
-       curl -Lsf "${KCONFIG_URL}" -o "${kconfig_file}"
+       update_kconfig "${kconfig_file}"

        if [[ "${kernel_recompile}" == "no" && ! -f "${kernel_bzimage}" ]]; then
                echo "Kernel image not found in ${kernel_bzimage},
kernel will be recompiled"
>
>
> >
> > >
> > > 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 :(
> >
> > I changed this from not compiling the kernel by default, to compiling it and you
> > can "keep your old kernel" with -k. This is because users may run the script,
> > not compile the kernel and run into issues with the image not being able to
> > mount as the kernel does not have the right config.
> >
> > The -k is for people who know what they are doing :)
> >
> > so you can always run
> >
> >  ./bpf_presubmit.sh -k
> >
> > after you have the kernel built once.
>
> That's not what I'm saying. When running `make` to build Linux, if
> won't do much at all if nothing changed. That's a good property that
> saves tons of time. I'm saying your script somehow precludes that
> behavior and make does tons of unnecessary work. It might be because
> of always re-downloaded config, which might make the above (not
> redownloading it if it didn't change) more important.
>
> Sure -k might be used this way, but it's expected to happen
> automatically. I'm just pointing out that something is not wired
> optimally to allow make do its job properly.

Ah, now I see what you are saying and yeah, it was indeed the downloading
of the config every time that was causing the kernel and selftest to be
recompiled.

With the change I posted above this does not happen anymore. I guess, with
this we can simply remove the -k option?

>
> >
> > >
> > > 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?
> > >
>
> And this won't be solved with '-k' alone, probably?

Yeah..

>
> > > 5. Before VM is started there is:
> > >
> > >
> > > #!/bin/bash

[...]