Re: kunit.py should default to --build_dir=.kunit

Re: kunit.py should default to --build_dir=.kunit

[Brendan Higgins]

+open list:KERNEL SELFTEST FRAMEWORK In case anyone in kselftest has
any thoughts.

On Thu, Oct 10, 2019 at 7:05 PM Theodore Ts'o <theodore.tso@gmail.com> wrote:
>
> I've been experimenting with the ext4 kunit test case, and something that would be really helpful is if the default is to store the object files for the ARCM=um kernel and its .config file in the top-level directory .kunit.   That is, that the default for --build_dir should be .kunit.
>
> Why does this important?  Because  the kernel developer will want to be running unit tests as well as building kernels that can be run under whatever architecture they are normally developing for (for example, an x86 kernel that can be run using kvm; or a arm64 kernel that gets run on an Android device by using the "fastboot" command).   So that means we don't want to be overwriting the object files and .config files for building the kernel for x86 when building the kunit kernel using the um arch.   For example, for ext4, my ideal workflow might go something like this:

That's a good point.

> <hack hack hack>
> % ./tools/testing/kunit/kunit.py  run
> <watch to see that unit tests succeed, and since most of the object files have already been built for the kunit kernel in be stored in the .kunit directory, this will be fast, since only the modified files will need to be recompiled>
> % kbuild
> <this is a script that builds an x86 kernel in /build/ext4-64 that is designed to be run under either kvm or in a GCE VM; since the kunit object files are stored in /build/ext4-kunit, the pre-existing files when building for x86_64 haven't been disturbed, so this build is fast as well>
> % kvm-xfstests smoke
> <this will run xfstests using the kernel plucked from /build/ext-64, using kvm>
>
> The point is when I'm developing an ext4 feature, or reviewing and merging ext4 commits, I need to be able to maintain separate build trees and separate config files for ARCH=um as well as ARCH=x86_64, and if the ARCH=um are stored in the kernel sources, then building with O=... doesn't work:
>
> <tytso@lambda> {/usr/projects/linux/kunit}   (kunit)
> 1084% make O=/build/test-dir
> make[1]: Entering directory '/build/test-dir'
> ***
> *** The source tree is not clean, please run 'make mrproper'
> *** in /usr/projects/linux/kunit
> ***

Should we maybe drop `--build_dir` in favor of `O`?

> One of the other reasons why it would be good to use --build_dir by default is that way, building with a separate O= build directory is regularly tested.   Right now, "kunit.py --build_dir=" seems to be broken.

Good point.

> % ./tools/testing/kunit/kunit.py run --build_dir=/build/ext4-kunit
> Generating .config ...
> [22:04:12] Building KUnit Kernel ...
> /usr/projects/linux/kunit/arch/x86/um/user-offsets.c:20:10: fatal error: asm/syscalls_64.h: No such file or directory
>    20 | #include <asm/syscalls_64.h>
>       |          ^~~~~~~~~~~~~~~~~~~
> compilation terminated.
>
> (This appears to be an ARCH=um bug, not a kunit bug, though.)

Yeah, I encountered this before. Some file is not getting properly
cleaned up by `make mrproper`. It works if you do `git clean -fdx` (I
know that's not a real solution for most people). Nevertheless, it
sounds like we need to sit down and actually solve this problem since
it is affecting users now.

I think you make a compelling argument. Anyone else have any thoughts on this?

Re: kunit.py should default to --build_dir=.kunit

[Randy Dunlap]

On 10/11/19 4:19 AM, Brendan Higgins wrote:
> +open list:KERNEL SELFTEST FRAMEWORK In case anyone in kselftest has
> any thoughts.
> 
> On Thu, Oct 10, 2019 at 7:05 PM Theodore Ts'o <theodore.tso@gmail.com> wrote:
>>
>> I've been experimenting with the ext4 kunit test case, and something that would be really helpful is if the default is to store the object files for the ARCM=um kernel and its .config file in the top-level directory .kunit.   That is, that the default for --build_dir should be .kunit.
>>
>> Why does this important?  Because  the kernel developer will want to be running unit tests as well as building kernels that can be run under whatever architecture they are normally developing for (for example, an x86 kernel that can be run using kvm; or a arm64 kernel that gets run on an Android device by using the "fastboot" command).   So that means we don't want to be overwriting the object files and .config files for building the kernel for x86 when building the kunit kernel using the um arch.   For example, for ext4, my ideal workflow might go something like this:
> 
> That's a good point.
> 
>> <hack hack hack>
>> % ./tools/testing/kunit/kunit.py  run
>> <watch to see that unit tests succeed, and since most of the object files have already been built for the kunit kernel in be stored in the .kunit directory, this will be fast, since only the modified files will need to be recompiled>
>> % kbuild
>> <this is a script that builds an x86 kernel in /build/ext4-64 that is designed to be run under either kvm or in a GCE VM; since the kunit object files are stored in /build/ext4-kunit, the pre-existing files when building for x86_64 haven't been disturbed, so this build is fast as well>
>> % kvm-xfstests smoke
>> <this will run xfstests using the kernel plucked from /build/ext-64, using kvm>
>>
>> The point is when I'm developing an ext4 feature, or reviewing and merging ext4 commits, I need to be able to maintain separate build trees and separate config files for ARCH=um as well as ARCH=x86_64, and if the ARCH=um are stored in the kernel sources, then building with O=... doesn't work:
>>
>> <tytso@lambda> {/usr/projects/linux/kunit}   (kunit)
>> 1084% make O=/build/test-dir
>> make[1]: Entering directory '/build/test-dir'
>> ***
>> *** The source tree is not clean, please run 'make mrproper'
>> *** in /usr/projects/linux/kunit
>> ***
> 
> Should we maybe drop `--build_dir` in favor of `O`?

Yes, preferably be consistent with the rest of the kernel makefiles.

>> One of the other reasons why it would be good to use --build_dir by default is that way, building with a separate O= build directory is regularly tested.   Right now, "kunit.py --build_dir=" seems to be broken.
> 
> Good point.
> 
>> % ./tools/testing/kunit/kunit.py run --build_dir=/build/ext4-kunit
>> Generating .config ...
>> [22:04:12] Building KUnit Kernel ...
>> /usr/projects/linux/kunit/arch/x86/um/user-offsets.c:20:10: fatal error: asm/syscalls_64.h: No such file or directory
>>    20 | #include <asm/syscalls_64.h>
>>       |          ^~~~~~~~~~~~~~~~~~~
>> compilation terminated.
>>
>> (This appears to be an ARCH=um bug, not a kunit bug, though.)
> 
> Yeah, I encountered this before. Some file is not getting properly
> cleaned up by `make mrproper`. It works if you do `git clean -fdx` (I
> know that's not a real solution for most people). Nevertheless, it
> sounds like we need to sit down and actually solve this problem since
> it is affecting users now.
> 
> I think you make a compelling argument. Anyone else have any thoughts on this?
> 


-- 
~Randy

Re: kunit.py should default to --build_dir=.kunit

[Brendan Higgins]

Sorry for the delayed reply. I was on vacation.

On Fri, Oct 11, 2019 at 5:16 AM Theodore Ts'o <theodore.tso@gmail.com> wrote:
>
>
>
> On Friday, October 11, 2019 at 7:19:49 AM UTC-4, Brendan Higgins wrote:
>>
>>
>> Should we maybe drop `--build_dir` in favor of `O`?
>
>
> How about if "make kunit" results in "./tools/testing/kunit/kunit.py run --build_dir=/.kunit --allconfig"
>
> ... where --allconfig automatically creates kunitconfig but in includes all of the CONFIG options which depends on CONFIG_KUNIT, so that all unit tests are run?    That way, we make it super easy for people to run the unit tests.  Since most users are used using make targets, this I bet will significantly increase the number of developers using kunit, because it will be super-duper convenient for them.
>
> Also, it would be nice if kunit.py first looks for kunitconfig in build_dir, and then in the top-level of the kernel sources, and we put .kunit in .gitignore.   That will make "git status" look nice and clean.
>
> What do folks think?

Having something like --allconfig is the ultimate goal. I had been
talking to Luis and Shuah about this for some time.

I think the best way to make this work would be for kunit_tool to be
able to detect all the tests with CONFIG_KUNIT as you suggest (or
something like it). Luis actually already suggested it; however, we
identified that this would likely not be as easy as it sounds as it is
possible to have mutually exclusive CONFIGs. Luis pointed out that
some researchers are currently working on a sat solver for Kconfig
that we could use to potentially address this problem. Nevertheless, a
complete solution in this regard is actually somewhat difficult.

Shuah's solution was just to use CONFIG fragments in the meantime
similar to what kselftest already does. I was leaning in that
direction since kselftest already does that and we know that it works.

Shuah, Luis, does this still match what you have been thinking?

Re: kunit.py should default to --build_dir=.kunit

[Brendan Higgins]

On Fri, Oct 11, 2019 at 7:56 AM Randy Dunlap <rdunlap@infradead.org> wrote:
>
> On 10/11/19 4:19 AM, Brendan Higgins wrote:
> > +open list:KERNEL SELFTEST FRAMEWORK In case anyone in kselftest has
> > any thoughts.
> >
> > On Thu, Oct 10, 2019 at 7:05 PM Theodore Ts'o <theodore.tso@gmail.com> wrote:
> >>
> >> I've been experimenting with the ext4 kunit test case, and something that would be really helpful is if the default is to store the object files for the ARCM=um kernel and its .config file in the top-level directory .kunit.   That is, that the default for --build_dir should be .kunit.
> >>
> >> Why does this important?  Because  the kernel developer will want to be running unit tests as well as building kernels that can be run under whatever architecture they are normally developing for (for example, an x86 kernel that can be run using kvm; or a arm64 kernel that gets run on an Android device by using the "fastboot" command).   So that means we don't want to be overwriting the object files and .config files for building the kernel for x86 when building the kunit kernel using the um arch.   For example, for ext4, my ideal workflow might go something like this:
> >
> > That's a good point.
> >
> >> <hack hack hack>
> >> % ./tools/testing/kunit/kunit.py  run
> >> <watch to see that unit tests succeed, and since most of the object files have already been built for the kunit kernel in be stored in the .kunit directory, this will be fast, since only the modified files will need to be recompiled>
> >> % kbuild
> >> <this is a script that builds an x86 kernel in /build/ext4-64 that is designed to be run under either kvm or in a GCE VM; since the kunit object files are stored in /build/ext4-kunit, the pre-existing files when building for x86_64 haven't been disturbed, so this build is fast as well>
> >> % kvm-xfstests smoke
> >> <this will run xfstests using the kernel plucked from /build/ext-64, using kvm>
> >>
> >> The point is when I'm developing an ext4 feature, or reviewing and merging ext4 commits, I need to be able to maintain separate build trees and separate config files for ARCH=um as well as ARCH=x86_64, and if the ARCH=um are stored in the kernel sources, then building with O=... doesn't work:
> >>
> >> <tytso@lambda> {/usr/projects/linux/kunit}   (kunit)
> >> 1084% make O=/build/test-dir
> >> make[1]: Entering directory '/build/test-dir'
> >> ***
> >> *** The source tree is not clean, please run 'make mrproper'
> >> *** in /usr/projects/linux/kunit
> >> ***
> >
> > Should we maybe drop `--build_dir` in favor of `O`?
>
> Yes, preferably be consistent with the rest of the kernel makefiles.

Alright, probably a good idea to make this change fairly soon then
before we have to worry about backwards compatibility and such.

Re: kunit.py should default to --build_dir=.kunit

[Theodore Y. Ts'o]

On Wed, Oct 16, 2019 at 02:04:35PM -0700, Brendan Higgins wrote:
> > > Should we maybe drop `--build_dir` in favor of `O`?
> >
> > Yes, preferably be consistent with the rest of the kernel makefiles.
> 
> Alright, probably a good idea to make this change fairly soon then
> before we have to worry about backwards compatibility and such.

I'm not sure how this would work; so something like:

    .../kunit.py run O=/build_dir

Should other flags we can pass in via the makefile processing, such as
V=1, etc., also work?  What other things can we pass in via after the
"run" command?

And if we're going to go this far, maybe we should make "make kunit"
run tools/testing/kunit/kunit.py?
    

Some minor other nits if you're going to be making changes to
kunit.py's CLI parsing:

1) It would be nice if there was a help command so that "kunit.py
   help" does what kunit.py -h does.

2) The top-level help message should indicate that "kunit.py run"
   takes various optional arguments and the way to find them is
   "kunit.py run -h".  This was *not* obvious, and the way I figured
   out there was even --build_dir option was via purusing the source
   code.  (It wasn't in the documentation that I could find.)

3) And maybe then "kunit.py help run" should display the help message
   for "kunit.py urn".  This would make it consistent with other tools
   that some of us might be familiar with (e.g., gcloud, gsutil, etc.)

Of course, if the front entry for kunit starts being "make kunit" as
opposed to ./tools/testing/kunit/kunit.py, then we really need to
figure out how to pass in the equivalent of --timeout.  (Maybe
--raw_output is enabled if we run make kunit V=1?).  And of course,
all of this would need to be documented.

					- Ted

Re: kunit.py should default to --build_dir=.kunit

[Luis Chamberlain]

On Wed, Oct 16, 2019 at 02:02:52PM -0700, Brendan Higgins wrote:
> Shuah's solution was just to use CONFIG fragments in the meantime
> similar to what kselftest already does. I was leaning in that
> direction since kselftest already does that and we know that it works.
> 
> Shuah, Luis, does this still match what you have been thinking?

I personally never use the selftest full config thing myself, however I
do use subcomponent selftests configs as hints to edit my .config to add
what I need and then run 'make menuconfig', in hopes that that leaves a
.config with all that is needed.

So indeed, I believe ethis works well for now, and it works for me.

I've hinted elsewhere that there is a difference between what kernel
features you have enabled Vs what components are needed / should we
built to test the current target kernel .config. And even then, what we
test in userspace is in my view different than what should be configured
in the kernel. To scale this I think a respective .config for userspace
and respective symbols for testing may be in order, this way the
userspace tests can only be visible say if you enabled certain features
in your kernel.  How this gets exposed, etc, is a separate question,
however I think this can be addressed later, and I believe Knut will
likely be dealing with it during the KTF merge to kunit work as
currently it addresses this via generic netlink, and we want something
simple to start off with.

   Luis

Re: kunit.py should default to --build_dir=.kunit

[Brendan Higgins]

On Thu, Oct 17, 2019 at 5:51 AM Theodore Y. Ts'o <tytso@mit.edu> wrote:
>
> On Wed, Oct 16, 2019 at 02:04:35PM -0700, Brendan Higgins wrote:
> > > > Should we maybe drop `--build_dir` in favor of `O`?
> > >
> > > Yes, preferably be consistent with the rest of the kernel makefiles.
> >
> > Alright, probably a good idea to make this change fairly soon then
> > before we have to worry about backwards compatibility and such.
>
> I'm not sure how this would work; so something like:
>
>     .../kunit.py run O=/build_dir

Seems reasonable to me.

> Should other flags we can pass in via the makefile processing, such as
> V=1, etc., also work?  What other things can we pass in via after the
> "run" command?

Hmmm...that's a good point. I don't know about V; probably need to
improve how kunit_tool displays build information for that to be
useful. I don't think that W is likely to be useful since I think that
is semantically a different operation than just running KUnit tests.

Probably don't want to forward ARCH, or CROSS_COMPILE or any of those.

Supporting some of these[1] seems useful.

> And if we're going to go this far, maybe we should make "make kunit"
> run tools/testing/kunit/kunit.py?

That seems reasonable. I was holding off on that initially because I
thought it might be a bridge too far in terms of putting KUnit in a
highly visible place. However, in hindsight, I think we crossed that
bridge a long time ago with putting tests is very visible places. So
yeah, now is probably a good time to do that.

> Some minor other nits if you're going to be making changes to
> kunit.py's CLI parsing:
>
> 1) It would be nice if there was a help command so that "kunit.py
>    help" does what kunit.py -h does.

Seems reasonable.

> 2) The top-level help message should indicate that "kunit.py run"
>    takes various optional arguments and the way to find them is
>    "kunit.py run -h".  This was *not* obvious, and the way I figured
>    out there was even --build_dir option was via purusing the source
>    code.  (It wasn't in the documentation that I could find.)

Also reasonable.

> 3) And maybe then "kunit.py help run" should display the help message
>    for "kunit.py urn".  This would make it consistent with other tools
>    that some of us might be familiar with (e.g., gcloud, gsutil, etc.)

That's reasonable, but also a little harder. At that point, I think we
might need to write our own flag library to support all this. So, I
will put this as a TODO, but we probably won't get to it for a while.

> Of course, if the front entry for kunit starts being "make kunit" as
> opposed to ./tools/testing/kunit/kunit.py, then we really need to
> figure out how to pass in the equivalent of --timeout.  (Maybe

Yeah, that's true. We should probably also by default set --timeout to
a reasonable default instead of infinite.

> --raw_output is enabled if we run make kunit V=1?).  And of course,

Hmmm...that seems like a good idea.

> all of this would need to be documented.

Yeah, we very much need to improve our documentation, especially for
the kunit_tool. That is very high on our TODO list.

[1] https://www.gnu.org/software/make/manual/html_node/Options-Summary.html

Re: kunit.py should default to --build_dir=.kunit

[Brendan Higgins]

On Fri, Oct 18, 2019 at 5:43 AM Luis Chamberlain <mcgrof@kernel.org> wrote:
>
> On Wed, Oct 16, 2019 at 02:02:52PM -0700, Brendan Higgins wrote:
> > Shuah's solution was just to use CONFIG fragments in the meantime
> > similar to what kselftest already does. I was leaning in that
> > direction since kselftest already does that and we know that it works.
> >
> > Shuah, Luis, does this still match what you have been thinking?
>
> I personally never use the selftest full config thing myself, however I
> do use subcomponent selftests configs as hints to edit my .config to add
> what I need and then run 'make menuconfig', in hopes that that leaves a
> .config with all that is needed.
>
> So indeed, I believe ethis works well for now, and it works for me.

Okay, good to know. So that is probably the right thing to do until we
can come up with a good solution to the dynamically generating an
allconfig problem.

> I've hinted elsewhere that there is a difference between what kernel
> features you have enabled Vs what components are needed / should we
> built to test the current target kernel .config. And even then, what we
> test in userspace is in my view different than what should be configured
> in the kernel. To scale this I think a respective .config for userspace

Sure.

> and respective symbols for testing may be in order, this way the
> userspace tests can only be visible say if you enabled certain features
> in your kernel.  How this gets exposed, etc, is a separate question,

I think that is a reasonable idea, but I don't think that really
applies here. I don't think it really makes sense to have the `make
kunit` that Ted is describing here do anything involving userspace.
That should just run the KUnit tests in the kernel. In anycase, I
expressed my ideas on the matter in more detail on the hybrid testing
thread[1].

> however I think this can be addressed later, and I believe Knut will
> likely be dealing with it during the KTF merge to kunit work as
> currently it addresses this via generic netlink, and we want something
> simple to start off with.

Cheers!

[1] https://lore.kernel.org/linux-kselftest/CAFd5g459xmO+=QPhnnXVO8+dB_t1PViXxK-Fz6Zp+sp5suJZ2w@mail.gmail.com/