Add a new QEMU config for kunit_tool, x86_64-smp, which provides an
8-cpu SMP setup. No other kunit_tool configurations provide an SMP
setup, so this is the best bet for testing things like KCSAN, which
require a multicore/multi-cpu system.
The choice of 8 CPUs is pretty arbitrary: it's enough to get tests like
KCSAN to run with a nontrivial number of worker threads, while still
working relatively quickly on older machines.
Signed-off-by: David Gow <[email protected]>
---
This is based off the discussion in:
https://groups.google.com/g/kasan-dev/c/A7XzC2pXRC8
---
tools/testing/kunit/qemu_configs/x86_64-smp.py | 13 +++++++++++++
1 file changed, 13 insertions(+)
create mode 100644 tools/testing/kunit/qemu_configs/x86_64-smp.py
diff --git a/tools/testing/kunit/qemu_configs/x86_64-smp.py b/tools/testing/kunit/qemu_configs/x86_64-smp.py
new file mode 100644
index 000000000000..a95623f5f8b7
--- /dev/null
+++ b/tools/testing/kunit/qemu_configs/x86_64-smp.py
@@ -0,0 +1,13 @@
+# SPDX-License-Identifier: GPL-2.0
+from ..qemu_config import QemuArchParams
+
+QEMU_ARCH = QemuArchParams(linux_arch='x86_64',
+ kconfig='''
+CONFIG_SERIAL_8250=y
+CONFIG_SERIAL_8250_CONSOLE=y
+CONFIG_SMP=y
+ ''',
+ qemu_arch='x86_64',
+ kernel_path='arch/x86/boot/bzImage',
+ kernel_command_line='console=ttyS0',
+ extra_qemu_params=['-smp', '8'])
--
2.36.0.550.gb090851708-goog
Add a .kunitconfig file, which provides a default, working config for
running the KCSAN tests. Note that it needs to run on an SMP machine, so
to run under kunit_tool, the x86_64-smp qemu-based setup should be used:
./tools/testing/kunit/kunit.py run --arch=x86_64-smp --kunitconfig=kernel/kcsan
Signed-off-by: David Gow <[email protected]>
---
kernel/kcsan/.kunitconfig | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
create mode 100644 kernel/kcsan/.kunitconfig
diff --git a/kernel/kcsan/.kunitconfig b/kernel/kcsan/.kunitconfig
new file mode 100644
index 000000000000..a8a815b1eb73
--- /dev/null
+++ b/kernel/kcsan/.kunitconfig
@@ -0,0 +1,20 @@
+# Note that the KCSAN tests need to run on an SMP setup.
+# Under kunit_tool, this can be done by using the x86_64-smp
+# qemu-based architecture:
+# ./tools/testing/kunit/kunit.py run --kunitconfig=kernel/kcsan --arch=x86_64-smp
+
+CONFIG_KUNIT=y
+
+CONFIG_DEBUG_KERNEL=y
+
+CONFIG_KCSAN=y
+CONFIG_KCSAN_KUNIT_TEST=y
+
+# Needed for test_barrier_nothreads
+CONFIG_KCSAN_STRICT=y
+CONFIG_KCSAN_WEAK_MEMORY=y
+
+# This prevents the test from timing out on many setups. Feel free to remove
+# (or alter) this, in conjunction with setting a different test timeout with,
+# for example, the --timeout kunit_tool option.
+CONFIG_KCSAN_REPORT_ONCE_IN_MS=100
--
2.36.0.550.gb090851708-goog
On Wed, May 18, 2022 at 03:32PM +0800, David Gow wrote:
> Add a .kunitconfig file, which provides a default, working config for
> running the KCSAN tests. Note that it needs to run on an SMP machine, so
> to run under kunit_tool, the x86_64-smp qemu-based setup should be used:
> ./tools/testing/kunit/kunit.py run --arch=x86_64-smp --kunitconfig=kernel/kcsan
>
> Signed-off-by: David Gow <[email protected]>
Reviewed-by: Marco Elver <[email protected]>
Thanks for adding this.
> ---
> kernel/kcsan/.kunitconfig | 20 ++++++++++++++++++++
> 1 file changed, 20 insertions(+)
> create mode 100644 kernel/kcsan/.kunitconfig
>
> diff --git a/kernel/kcsan/.kunitconfig b/kernel/kcsan/.kunitconfig
> new file mode 100644
> index 000000000000..a8a815b1eb73
> --- /dev/null
> +++ b/kernel/kcsan/.kunitconfig
> @@ -0,0 +1,20 @@
> +# Note that the KCSAN tests need to run on an SMP setup.
> +# Under kunit_tool, this can be done by using the x86_64-smp
> +# qemu-based architecture:
> +# ./tools/testing/kunit/kunit.py run --kunitconfig=kernel/kcsan --arch=x86_64-smp
> +
> +CONFIG_KUNIT=y
> +
> +CONFIG_DEBUG_KERNEL=y
> +
> +CONFIG_KCSAN=y
> +CONFIG_KCSAN_KUNIT_TEST=y
> +
> +# Needed for test_barrier_nothreads
> +CONFIG_KCSAN_STRICT=y
> +CONFIG_KCSAN_WEAK_MEMORY=y
Note, KCSAN_STRICT implies KCSAN_WEAK_MEMORY.
Also, a bunch of the test cases' outcomes depend on KCSAN's
"strictness". I think to cover the various combinations would be too
complex, but we can just settle on testing KCSAN_STRICT=y.
The end result is the same, but you could drop the
CONFIG_KCSAN_WEAK_MEMORY=y line, and let the latest KCSAN_STRICT
defaults decide (I don't expect them to change any time soon).
If you want it to be more explicit, it's also fine leaving the
CONFIG_KCSAN_WEAK_MEMORY=y line in.
> +# This prevents the test from timing out on many setups. Feel free to remove
> +# (or alter) this, in conjunction with setting a different test timeout with,
> +# for example, the --timeout kunit_tool option.
> +CONFIG_KCSAN_REPORT_ONCE_IN_MS=100
> --
> 2.36.0.550.gb090851708-goog
>
On Wed, May 18, 2022 at 03:32PM +0800, 'David Gow' via KUnit Development wrote:
> Add a new QEMU config for kunit_tool, x86_64-smp, which provides an
> 8-cpu SMP setup. No other kunit_tool configurations provide an SMP
> setup, so this is the best bet for testing things like KCSAN, which
> require a multicore/multi-cpu system.
>
> The choice of 8 CPUs is pretty arbitrary: it's enough to get tests like
> KCSAN to run with a nontrivial number of worker threads, while still
> working relatively quickly on older machines.
>
> Signed-off-by: David Gow <[email protected]>
Acked-by: Marco Elver <[email protected]>
> ---
>
> This is based off the discussion in:
> https://groups.google.com/g/kasan-dev/c/A7XzC2pXRC8
>
> ---
> tools/testing/kunit/qemu_configs/x86_64-smp.py | 13 +++++++++++++
> 1 file changed, 13 insertions(+)
> create mode 100644 tools/testing/kunit/qemu_configs/x86_64-smp.py
>
> diff --git a/tools/testing/kunit/qemu_configs/x86_64-smp.py b/tools/testing/kunit/qemu_configs/x86_64-smp.py
> new file mode 100644
> index 000000000000..a95623f5f8b7
> --- /dev/null
> +++ b/tools/testing/kunit/qemu_configs/x86_64-smp.py
> @@ -0,0 +1,13 @@
> +# SPDX-License-Identifier: GPL-2.0
> +from ..qemu_config import QemuArchParams
> +
> +QEMU_ARCH = QemuArchParams(linux_arch='x86_64',
> + kconfig='''
> +CONFIG_SERIAL_8250=y
> +CONFIG_SERIAL_8250_CONSOLE=y
> +CONFIG_SMP=y
> + ''',
> + qemu_arch='x86_64',
> + kernel_path='arch/x86/boot/bzImage',
> + kernel_command_line='console=ttyS0',
> + extra_qemu_params=['-smp', '8'])
> --
> 2.36.0.550.gb090851708-goog
>
> --
> You received this message because you are subscribed to the Google Groups "KUnit Development" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to [email protected].
> To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20220518073232.526443-1-davidgow%40google.com.
On Wed, May 18, 2022 at 12:32 AM 'David Gow' via KUnit Development
<[email protected]> wrote:
>
> Add a new QEMU config for kunit_tool, x86_64-smp, which provides an
> 8-cpu SMP setup. No other kunit_tool configurations provide an SMP
> setup, so this is the best bet for testing things like KCSAN, which
> require a multicore/multi-cpu system.
>
> The choice of 8 CPUs is pretty arbitrary: it's enough to get tests like
> KCSAN to run with a nontrivial number of worker threads, while still
> working relatively quickly on older machines.
>
Since it's arbitrary, I somewhat prefer the idea of leaving up
entirely to the caller
i.e.
$ kunit.py run --kconfig_add=CONFIG_SMP=y --qemu_args '-smp 8'
We could add CONFIG_SMP=y to the default qemu_configs/*.py and do
$ kunit.py run --qemu_args '-smp 8'
but I'd prefer the first, even if it is more verbose.
Marco, does this seem reasonable from your perspective?
I think that a new --qemu_args would be generically useful for adhoc
use and light enough that people won't need to add qemu_configs much.
E.g. I can see people wanting multiple NUMA nodes, a specific -cpu, and so on.
On Wed, 18 May 2022 at 17:31, Daniel Latypov <[email protected]> wrote:
>
> On Wed, May 18, 2022 at 12:32 AM 'David Gow' via KUnit Development
> <[email protected]> wrote:
> >
> > Add a new QEMU config for kunit_tool, x86_64-smp, which provides an
> > 8-cpu SMP setup. No other kunit_tool configurations provide an SMP
> > setup, so this is the best bet for testing things like KCSAN, which
> > require a multicore/multi-cpu system.
> >
> > The choice of 8 CPUs is pretty arbitrary: it's enough to get tests like
> > KCSAN to run with a nontrivial number of worker threads, while still
> > working relatively quickly on older machines.
> >
>
> Since it's arbitrary, I somewhat prefer the idea of leaving up
> entirely to the caller
> i.e.
> $ kunit.py run --kconfig_add=CONFIG_SMP=y --qemu_args '-smp 8'
>
> We could add CONFIG_SMP=y to the default qemu_configs/*.py and do
> $ kunit.py run --qemu_args '-smp 8'
> but I'd prefer the first, even if it is more verbose.
>
> Marco, does this seem reasonable from your perspective?
Either way works. But I wouldn't mind a sane default though, where
that default can be overridden with custom number of CPUs.
> I think that a new --qemu_args would be generically useful for adhoc
> use and light enough that people won't need to add qemu_configs much.
> E.g. I can see people wanting multiple NUMA nodes, a specific -cpu, and so on.
On Wed, May 18, 2022 at 8:36 AM Marco Elver <[email protected]> wrote:
>
> On Wed, 18 May 2022 at 17:31, Daniel Latypov <[email protected]> wrote:
> >
> > On Wed, May 18, 2022 at 12:32 AM 'David Gow' via KUnit Development
> > <[email protected]> wrote:
> > >
> > > Add a new QEMU config for kunit_tool, x86_64-smp, which provides an
> > > 8-cpu SMP setup. No other kunit_tool configurations provide an SMP
> > > setup, so this is the best bet for testing things like KCSAN, which
> > > require a multicore/multi-cpu system.
> > >
> > > The choice of 8 CPUs is pretty arbitrary: it's enough to get tests like
> > > KCSAN to run with a nontrivial number of worker threads, while still
> > > working relatively quickly on older machines.
> > >
> >
> > Since it's arbitrary, I somewhat prefer the idea of leaving up
> > entirely to the caller
> > i.e.
> > $ kunit.py run --kconfig_add=CONFIG_SMP=y --qemu_args '-smp 8'
> >
> > We could add CONFIG_SMP=y to the default qemu_configs/*.py and do
> > $ kunit.py run --qemu_args '-smp 8'
> > but I'd prefer the first, even if it is more verbose.
> >
> > Marco, does this seem reasonable from your perspective?
>
> Either way works. But I wouldn't mind a sane default though, where
> that default can be overridden with custom number of CPUs.
>
Ack.
Let me clean up what I have for --qemu_args and send it out for discussion.
One downside I see to adding more qemu_configs is that --arch now
becomes more kunit-specific.
Before, a user could assume "oh, it's just what I pass in to make ARCH=...".
This new "--arch=x86_64-smp" violates that. I don't personally see it
being that confusing, but I still worry.
On Wed, May 18, 2022 at 8:39 AM Daniel Latypov <[email protected]> wrote:
> > Either way works. But I wouldn't mind a sane default though, where
> > that default can be overridden with custom number of CPUs.
> >
>
> Ack.
> Let me clean up what I have for --qemu_args and send it out for discussion.
Sent out as https://lore.kernel.org/linux-kselftest/[email protected]
On Wed, May 18, 2022 at 12:32 AM David Gow <[email protected]> wrote:
> diff --git a/kernel/kcsan/.kunitconfig b/kernel/kcsan/.kunitconfig
> new file mode 100644
> index 000000000000..a8a815b1eb73
> --- /dev/null
> +++ b/kernel/kcsan/.kunitconfig
> @@ -0,0 +1,20 @@
> +# Note that the KCSAN tests need to run on an SMP setup.
> +# Under kunit_tool, this can be done by using the x86_64-smp
> +# qemu-based architecture:
> +# ./tools/testing/kunit/kunit.py run --kunitconfig=kernel/kcsan --arch=x86_64-smp
Just noting here, if we go with --qemu_args [1], then we'd change this to
--arch=x86_64 --qemu_args='-smp 8'
and then probably add
CONFIG_SMP=y
to this file.
[1] https://lore.kernel.org/linux-kselftest/[email protected]
> +
> +CONFIG_KUNIT=y
> +
> +CONFIG_DEBUG_KERNEL=y
> +
> +CONFIG_KCSAN=y
> +CONFIG_KCSAN_KUNIT_TEST=y
> +
> +# Needed for test_barrier_nothreads
> +CONFIG_KCSAN_STRICT=y
> +CONFIG_KCSAN_WEAK_MEMORY=y
> +
> +# This prevents the test from timing out on many setups. Feel free to remove
> +# (or alter) this, in conjunction with setting a different test timeout with,
> +# for example, the --timeout kunit_tool option.
> +CONFIG_KCSAN_REPORT_ONCE_IN_MS=100
Tangent:
Ah this reminds me, unfortunately you can't use --kconfig_add to
overwrite this atm.
Right now, it'll just blindly try to append and then complain that one
of the two copies of the option is missing.
That might be a feature to look into.
Or at least, we can maybe give a better error message.
E.g. with the default kunitconfig, the error currently looks like
# Try to overwrite CONFIG_KUNIT_ALL_TESTS=y
$ ./tools/testing/kunit/kunit.py config --kconfig_add=CONFIG_KUNIT_ALL_TESTS=m
...
ERROR:root:Not all Kconfig options selected in kunitconfig were in the
generated .config.
This is probably due to unsatisfied dependencies.
Missing: CONFIG_KUNIT_ALL_TESTS=m
On Wed, May 18, 2022 at 5:21 PM Marco Elver <[email protected]> wrote:
>
> On Wed, May 18, 2022 at 03:32PM +0800, David Gow wrote:
> > Add a .kunitconfig file, which provides a default, working config for
> > running the KCSAN tests. Note that it needs to run on an SMP machine, so
> > to run under kunit_tool, the x86_64-smp qemu-based setup should be used:
> > ./tools/testing/kunit/kunit.py run --arch=x86_64-smp --kunitconfig=kernel/kcsan
> >
> > Signed-off-by: David Gow <[email protected]>
>
> Reviewed-by: Marco Elver <[email protected]>
>
> Thanks for adding this.
>
> > ---
> > kernel/kcsan/.kunitconfig | 20 ++++++++++++++++++++
> > 1 file changed, 20 insertions(+)
> > create mode 100644 kernel/kcsan/.kunitconfig
> >
> > diff --git a/kernel/kcsan/.kunitconfig b/kernel/kcsan/.kunitconfig
> > new file mode 100644
> > index 000000000000..a8a815b1eb73
> > --- /dev/null
> > +++ b/kernel/kcsan/.kunitconfig
> > @@ -0,0 +1,20 @@
> > +# Note that the KCSAN tests need to run on an SMP setup.
> > +# Under kunit_tool, this can be done by using the x86_64-smp
> > +# qemu-based architecture:
> > +# ./tools/testing/kunit/kunit.py run --kunitconfig=kernel/kcsan --arch=x86_64-smp
> > +
> > +CONFIG_KUNIT=y
> > +
> > +CONFIG_DEBUG_KERNEL=y
> > +
> > +CONFIG_KCSAN=y
> > +CONFIG_KCSAN_KUNIT_TEST=y
> > +
> > +# Needed for test_barrier_nothreads
> > +CONFIG_KCSAN_STRICT=y
> > +CONFIG_KCSAN_WEAK_MEMORY=y
>
> Note, KCSAN_STRICT implies KCSAN_WEAK_MEMORY.
>
> Also, a bunch of the test cases' outcomes depend on KCSAN's
> "strictness". I think to cover the various combinations would be too
> complex, but we can just settle on testing KCSAN_STRICT=y.
It's definitely possible to either have multiple .kunitconfigs, each
of which could have slightly different setups, e.g.:
- kernel/kcsan/.kunitconfig (defualt)
- kernel/kcsan/strict.kunitconfig (passed explicitly when desired)
Equally, if we got rid of KCSAN_STRICT in the .kunitconfig, you could
override it with --kconfig_add, e.g.
- ./tools/testing/kunit/kunit.py run --kunitconfig=kernel/kcsan
--arch=x86_64-smp
- ./tools/testing/kunit/kunit.py run --kunitconfig=kernel/kcsan
--arch=x86_64-smp --kconfig_add CONFIG_KSCAN_STRICT=y
> The end result is the same, but you could drop the
> CONFIG_KCSAN_WEAK_MEMORY=y line, and let the latest KCSAN_STRICT
> defaults decide (I don't expect them to change any time soon).
>
> If you want it to be more explicit, it's also fine leaving the
> CONFIG_KCSAN_WEAK_MEMORY=y line in.
Do you have a preference here? Or to get rid of both and default to
the non-strict version mentioned above?
>
> > +# This prevents the test from timing out on many setups. Feel free to remove
> > +# (or alter) this, in conjunction with setting a different test timeout with,
> > +# for example, the --timeout kunit_tool option.
> > +CONFIG_KCSAN_REPORT_ONCE_IN_MS=100
> > --
> > 2.36.0.550.gb090851708-goog
> >
On Thu, May 19, 2022 at 6:15 AM David Gow <[email protected]> wrote:
>
> I tend to agree that having both would be nice: I think there are
> enough useful "machine configs" that trying to maintain, e.g, a 1:1
> mapping with kernel architectures is going to leave a bunch of things
> on the table, particularly as we add more tests for, e.g., drivers and
> specific CPU models.
I agree that we don't necessarily need to maintain a 1:1 mapping.
But I feel like we should have a pretty convincing reason for doing
so, e.g. support for a CPU that requires we add in a bunch of
kconfigs.
This particular one feels simple enough to me.
Given we already have to put specific instructions in the
kcsan/.kunitconfig, I don't know if there's much of a difference in
cost between these two commands
$ ./tools/testing/kunit/kunit.py run --kunitconfig=kernel/kcsan
--arch=x86_64-smp
$ ./tools/testing/kunit/kunit.py run --kunitconfig=kernel/kcsan
--arch=x86_64 --kconfig_add CONFIG_SMP=y --qemu_args "-smp 8"
I've generally learned to prefer more explicit commands like the
second, even if they're quite a bit longer.
But I have the following biases
* I use FZF heavily, so I don't re-type long commands much
* I'm the person who proposed --kconfig_add and --qemu_args, so of
course I'd think the longer form is easy to understand.
so I'm not in a position to object to this change.
Changing topics:
Users can overwrite the '-smp 8' here via --qemu_args [1], so I'm much
less worried about hard-coding any specific value in this file
anymore.
And given that, I think a more "natural" value for this file would be "-smp 2".
I think anything that needs more than that should explicitly should --qemu_args.
Thoughts?
[1] tested with --qemu_args='-smp 4' --qemu_args='-smp 8'
and I see the following in the test.log
smpboot: Allowing 8 CPUs, 0 hotplug CPUs
so QEMU respects the last value passed in, as expected.
>
> The problem, of course, is that the --kconfig_add flags don't allow us
> to override anything explicitly stated in either the kunitconfig or
> qemu_config (and I imagine there could be problems with --qemu_config,
> too).
This patch would fix that.
https://lore.kernel.org/linux-kselftest/[email protected]
It introduces an overwriting priority of
* --kconfig_add
* kunitconfig / --kunitconfig
* qemu_config
On Thu, 19 May 2022 at 15:08, David Gow <[email protected]> wrote:
>
> On Wed, May 18, 2022 at 5:21 PM Marco Elver <[email protected]> wrote:
> >
> > On Wed, May 18, 2022 at 03:32PM +0800, David Gow wrote:
> > > Add a .kunitconfig file, which provides a default, working config for
> > > running the KCSAN tests. Note that it needs to run on an SMP machine, so
> > > to run under kunit_tool, the x86_64-smp qemu-based setup should be used:
> > > ./tools/testing/kunit/kunit.py run --arch=x86_64-smp --kunitconfig=kernel/kcsan
> > >
> > > Signed-off-by: David Gow <[email protected]>
> >
> > Reviewed-by: Marco Elver <[email protected]>
> >
> > Thanks for adding this.
> >
> > > ---
> > > kernel/kcsan/.kunitconfig | 20 ++++++++++++++++++++
> > > 1 file changed, 20 insertions(+)
> > > create mode 100644 kernel/kcsan/.kunitconfig
> > >
> > > diff --git a/kernel/kcsan/.kunitconfig b/kernel/kcsan/.kunitconfig
> > > new file mode 100644
> > > index 000000000000..a8a815b1eb73
> > > --- /dev/null
> > > +++ b/kernel/kcsan/.kunitconfig
> > > @@ -0,0 +1,20 @@
> > > +# Note that the KCSAN tests need to run on an SMP setup.
> > > +# Under kunit_tool, this can be done by using the x86_64-smp
> > > +# qemu-based architecture:
> > > +# ./tools/testing/kunit/kunit.py run --kunitconfig=kernel/kcsan --arch=x86_64-smp
> > > +
> > > +CONFIG_KUNIT=y
> > > +
> > > +CONFIG_DEBUG_KERNEL=y
> > > +
> > > +CONFIG_KCSAN=y
> > > +CONFIG_KCSAN_KUNIT_TEST=y
> > > +
> > > +# Needed for test_barrier_nothreads
> > > +CONFIG_KCSAN_STRICT=y
> > > +CONFIG_KCSAN_WEAK_MEMORY=y
> >
> > Note, KCSAN_STRICT implies KCSAN_WEAK_MEMORY.
> >
> > Also, a bunch of the test cases' outcomes depend on KCSAN's
> > "strictness". I think to cover the various combinations would be too
> > complex, but we can just settle on testing KCSAN_STRICT=y.
>
> It's definitely possible to either have multiple .kunitconfigs, each
> of which could have slightly different setups, e.g.:
> - kernel/kcsan/.kunitconfig (defualt)
> - kernel/kcsan/strict.kunitconfig (passed explicitly when desired)
>
> Equally, if we got rid of KCSAN_STRICT in the .kunitconfig, you could
> override it with --kconfig_add, e.g.
> - ./tools/testing/kunit/kunit.py run --kunitconfig=kernel/kcsan
> --arch=x86_64-smp
> - ./tools/testing/kunit/kunit.py run --kunitconfig=kernel/kcsan
> --arch=x86_64-smp --kconfig_add CONFIG_KSCAN_STRICT=y
>
> > The end result is the same, but you could drop the
> > CONFIG_KCSAN_WEAK_MEMORY=y line, and let the latest KCSAN_STRICT
> > defaults decide (I don't expect them to change any time soon).
> >
> > If you want it to be more explicit, it's also fine leaving the
> > CONFIG_KCSAN_WEAK_MEMORY=y line in.
>
> Do you have a preference here? Or to get rid of both and default to
> the non-strict version mentioned above?
I'd keep it simple for now, and remove both lines i.e. make non-strict
the default. It's easy to just run with --kconfig_add
CONFIG_KCSAN_STRICT=y, along with other variations. I know that
rcutoruture uses KCSAN_STRICT=y by default, so it's already getting
coverage there. ;-)
Thanks,
-- Marco
On Wed, May 18, 2022 at 11:36 PM Marco Elver <[email protected]> wrote:
>
> On Wed, 18 May 2022 at 17:31, Daniel Latypov <[email protected]> wrote:
> >
> > On Wed, May 18, 2022 at 12:32 AM 'David Gow' via KUnit Development
> > <[email protected]> wrote:
> > >
> > > Add a new QEMU config for kunit_tool, x86_64-smp, which provides an
> > > 8-cpu SMP setup. No other kunit_tool configurations provide an SMP
> > > setup, so this is the best bet for testing things like KCSAN, which
> > > require a multicore/multi-cpu system.
> > >
> > > The choice of 8 CPUs is pretty arbitrary: it's enough to get tests like
> > > KCSAN to run with a nontrivial number of worker threads, while still
> > > working relatively quickly on older machines.
> > >
> >
> > Since it's arbitrary, I somewhat prefer the idea of leaving up
> > entirely to the caller
> > i.e.
> > $ kunit.py run --kconfig_add=CONFIG_SMP=y --qemu_args '-smp 8'
> >
> > We could add CONFIG_SMP=y to the default qemu_configs/*.py and do
> > $ kunit.py run --qemu_args '-smp 8'
> > but I'd prefer the first, even if it is more verbose.
> >
> > Marco, does this seem reasonable from your perspective?
>
> Either way works. But I wouldn't mind a sane default though, where
> that default can be overridden with custom number of CPUs.
>
I tend to agree that having both would be nice: I think there are
enough useful "machine configs" that trying to maintain, e.g, a 1:1
mapping with kernel architectures is going to leave a bunch of things
on the table, particularly as we add more tests for, e.g., drivers and
specific CPU models.
The problem, of course, is that the --kconfig_add flags don't allow us
to override anything explicitly stated in either the kunitconfig or
qemu_config (and I imagine there could be problems with --qemu_config,
too).
> > I think that a new --qemu_args would be generically useful for adhoc
> > use and light enough that people won't need to add qemu_configs much.
> > E.g. I can see people wanting multiple NUMA nodes, a specific -cpu, and so on.
On Thu, May 19, 2022 at 1:11 PM Daniel Latypov <[email protected]> wrote:
>
> On Thu, May 19, 2022 at 6:15 AM David Gow <[email protected]> wrote:
> >
> > I tend to agree that having both would be nice: I think there are
> > enough useful "machine configs" that trying to maintain, e.g, a 1:1
> > mapping with kernel architectures is going to leave a bunch of things
> > on the table, particularly as we add more tests for, e.g., drivers and
> > specific CPU models.
>
> I agree that we don't necessarily need to maintain a 1:1 mapping.
> But I feel like we should have a pretty convincing reason for doing
> so, e.g. support for a CPU that requires we add in a bunch of
> kconfigs.
Agreed. That being said, if we have a good convention for archs that
are not in arch/, then it should be OK. The biggest thing is that all
archs passed into ARCH=, if supported, should have a default with the
same value for kunittool; as long as that is the case, I don't think
anyone will get confused.
> This particular one feels simple enough to me.
> Given we already have to put specific instructions in the
> kcsan/.kunitconfig, I don't know if there's much of a difference in
> cost between these two commands
>
> $ ./tools/testing/kunit/kunit.py run --kunitconfig=kernel/kcsan
> --arch=x86_64-smp
> $ ./tools/testing/kunit/kunit.py run --kunitconfig=kernel/kcsan
> --arch=x86_64 --kconfig_add CONFIG_SMP=y --qemu_args "-smp 8"
Also agree.
> I've generally learned to prefer more explicit commands like the
> second, even if they're quite a bit longer.
I agree, but I think I learned this from you :-)
> But I have the following biases
> * I use FZF heavily, so I don't re-type long commands much
Same.
> * I'm the person who proposed --kconfig_add and --qemu_args, so of
> course I'd think the longer form is easy to understand.
> so I'm not in a position to object to this change.
Yeah, I think I am a bit biased on this too, but I don't terribly care
one way or the other.
> Changing topics:
> Users can overwrite the '-smp 8' here via --qemu_args [1], so I'm much
> less worried about hard-coding any specific value in this file
> anymore.
> And given that, I think a more "natural" value for this file would be "-smp 2".
> I think anything that needs more than that should explicitly should --qemu_args.
>
> Thoughts?
If we have time, we could bring this topic up at LPC?
> [1] tested with --qemu_args='-smp 4' --qemu_args='-smp 8'
> and I see the following in the test.log
> smpboot: Allowing 8 CPUs, 0 hotplug CPUs
> so QEMU respects the last value passed in, as expected.
>
> >
> > The problem, of course, is that the --kconfig_add flags don't allow us
> > to override anything explicitly stated in either the kunitconfig or
> > qemu_config (and I imagine there could be problems with --qemu_config,
> > too).
>
> This patch would fix that.
> https://lore.kernel.org/linux-kselftest/[email protected]
>
> It introduces an overwriting priority of
> * --kconfig_add
> * kunitconfig / --kunitconfig
> * qemu_config
On Wed, May 18, 2022 at 3:32 AM David Gow <[email protected]> wrote:
>
> Add a .kunitconfig file, which provides a default, working config for
> running the KCSAN tests. Note that it needs to run on an SMP machine, so
> to run under kunit_tool, the x86_64-smp qemu-based setup should be used:
> ./tools/testing/kunit/kunit.py run --arch=x86_64-smp --kunitconfig=kernel/kcsan
>
> Signed-off-by: David Gow <[email protected]>
Ack, but I think Marco settled on removing CONFIG_KCSAN_STRICT=y and
CONFIG_KCSAN_WEAK_MEMORY=y.
Acked-by: Brendan Higgins <[email protected]>
On Wed, May 18, 2022 at 3:32 AM 'David Gow' via KUnit Development
<[email protected]> wrote:
>
> Add a new QEMU config for kunit_tool, x86_64-smp, which provides an
> 8-cpu SMP setup. No other kunit_tool configurations provide an SMP
> setup, so this is the best bet for testing things like KCSAN, which
> require a multicore/multi-cpu system.
>
> The choice of 8 CPUs is pretty arbitrary: it's enough to get tests like
> KCSAN to run with a nontrivial number of worker threads, while still
> working relatively quickly on older machines.
>
> Signed-off-by: David Gow <[email protected]>
I know there is some discussion on this patch, but I think this patch
is good as implemented; we could always delete this config if we
change our policies later.
Reviewed-by: Brendan Higgins <[email protected]>
On Thu, May 19, 2022 at 6:24 AM Marco Elver <[email protected]> wrote:
> I'd keep it simple for now, and remove both lines i.e. make non-strict
> the default. It's easy to just run with --kconfig_add
> CONFIG_KCSAN_STRICT=y, along with other variations. I know that
> rcutoruture uses KCSAN_STRICT=y by default, so it's already getting
> coverage there. ;-)
David decided to drop the parent patch (the new QEMU config) now
--qemu_args was merged into the kunit tree.
Did we want a standalone v2 of this patch?
Based on Marco's comments, we'd change:
* drop CONFIG_KCSAN_STRICT=y per this comment [1]
* drop CONFIG_KCSAN_WEAK_MEMORY per previous comments
Then for --qemu_args changes:
* add CONFIG_SMP=y explicitly to this file
* update the comment to show to include --qemu_args="-smp 8"
Does this sound right?
[1] Note: there's also patches in kunit now so you could do
--kconfig_add=CONFIG_KCSAN_STRICT=n to explicitly disable it. This
wasn't possible before. Does that change what we want for the default?
Daniel
On Thu, 14 Jul 2022 at 22:23, Daniel Latypov <[email protected]> wrote:
>
> On Thu, May 19, 2022 at 6:24 AM Marco Elver <[email protected]> wrote:
> > I'd keep it simple for now, and remove both lines i.e. make non-strict
> > the default. It's easy to just run with --kconfig_add
> > CONFIG_KCSAN_STRICT=y, along with other variations. I know that
> > rcutoruture uses KCSAN_STRICT=y by default, so it's already getting
> > coverage there. ;-)
>
> David decided to drop the parent patch (the new QEMU config) now
> --qemu_args was merged into the kunit tree.
> Did we want a standalone v2 of this patch?
>
> Based on Marco's comments, we'd change:
> * drop CONFIG_KCSAN_STRICT=y per this comment [1]
> * drop CONFIG_KCSAN_WEAK_MEMORY per previous comments
> Then for --qemu_args changes:
> * add CONFIG_SMP=y explicitly to this file
> * update the comment to show to include --qemu_args="-smp 8"
>
> Does this sound right?
Yes, sounds good to me, and thanks for remembering this. I'd prefer a
close-to-default config.
> [1] Note: there's also patches in kunit now so you could do
> --kconfig_add=CONFIG_KCSAN_STRICT=n to explicitly disable it. This
> wasn't possible before. Does that change what we want for the default?
I'd just have KCSAN_STRICT=n by default, and if desired it can be
added per kconfig_add just the same way.
Thanks,
-- Marco
On Thu, Jul 14, 2022 at 2:41 PM Marco Elver <[email protected]> wrote:
>
> On Thu, 14 Jul 2022 at 22:23, Daniel Latypov <[email protected]> wrote:
> >
> > On Thu, May 19, 2022 at 6:24 AM Marco Elver <[email protected]> wrote:
> > > I'd keep it simple for now, and remove both lines i.e. make non-strict
> > > the default. It's easy to just run with --kconfig_add
> > > CONFIG_KCSAN_STRICT=y, along with other variations. I know that
> > > rcutoruture uses KCSAN_STRICT=y by default, so it's already getting
> > > coverage there. ;-)
> >
> > David decided to drop the parent patch (the new QEMU config) now
> > --qemu_args was merged into the kunit tree.
> > Did we want a standalone v2 of this patch?
> >
> > Based on Marco's comments, we'd change:
> > * drop CONFIG_KCSAN_STRICT=y per this comment [1]
> > * drop CONFIG_KCSAN_WEAK_MEMORY per previous comments
> > Then for --qemu_args changes:
> > * add CONFIG_SMP=y explicitly to this file
> > * update the comment to show to include --qemu_args="-smp 8"
> >
> > Does this sound right?
>
> Yes, sounds good to me, and thanks for remembering this. I'd prefer a
> close-to-default config.
>
> > [1] Note: there's also patches in kunit now so you could do
> > --kconfig_add=CONFIG_KCSAN_STRICT=n to explicitly disable it. This
> > wasn't possible before. Does that change what we want for the default?
>
> I'd just have KCSAN_STRICT=n by default, and if desired it can be
> added per kconfig_add just the same way.
Ack.
So concretely, so then a final result like this?
$ cat kernel/kcsan/.kunitconfig
# Note that the KCSAN tests need to run on an SMP setup.
# Under kunit_tool, this can be done by using the x86_64-smp
# qemu-based architecture:
# ./tools/testing/kunit/kunit.py run --kunitconfig=kernel/kcsan
--arch=x86_64 --qemu_args='-smp 8'
CONFIG_KUNIT=y
CONFIG_DEBUG_KERNEL=y
CONFIG_KCSAN=y
CONFIG_KCSAN_KUNIT_TEST=y
# Need some level of concurrency to test a concurrency sanitizer.
CONFIG_SMP=y
# This prevents the test from timing out on many setups. Feel free to remove
# (or alter) this, in conjunction with setting a different test timeout with,
# for example, the --timeout kunit_tool option.
CONFIG_KCSAN_REPORT_ONCE_IN_MS=100
On Thu, Jul 14, 2022 at 4:45 PM Daniel Latypov <[email protected]> wrote:
> Ack.
> So concretely, so then a final result like this?
>
> $ cat kernel/kcsan/.kunitconfig
> # Note that the KCSAN tests need to run on an SMP setup.
> # Under kunit_tool, this can be done by using the x86_64-smp
> # qemu-based architecture:
Oops, this bit would need to be updated to something like:
# Under kunit_tool, this can be done by using --qemu_args:
> # ./tools/testing/kunit/kunit.py run --kunitconfig=kernel/kcsan
> --arch=x86_64 --qemu_args='-smp 8'
>
> CONFIG_KUNIT=y
>
> CONFIG_DEBUG_KERNEL=y
>
> CONFIG_KCSAN=y
> CONFIG_KCSAN_KUNIT_TEST=y
>
> # Need some level of concurrency to test a concurrency sanitizer.
> CONFIG_SMP=y
>
> # This prevents the test from timing out on many setups. Feel free to remove
> # (or alter) this, in conjunction with setting a different test timeout with,
> # for example, the --timeout kunit_tool option.
> CONFIG_KCSAN_REPORT_ONCE_IN_MS=100
On Fri, Jul 15, 2022 at 7:48 AM Daniel Latypov <[email protected]> wrote:
>
> On Thu, Jul 14, 2022 at 4:45 PM Daniel Latypov <[email protected]> wrote:
> > Ack.
> > So concretely, so then a final result like this?
> >
> > $ cat kernel/kcsan/.kunitconfig
> > # Note that the KCSAN tests need to run on an SMP setup.
> > # Under kunit_tool, this can be done by using the x86_64-smp
> > # qemu-based architecture:
>
> Oops, this bit would need to be updated to something like:
>
> # Under kunit_tool, this can be done by using --qemu_args:
>
> > # ./tools/testing/kunit/kunit.py run --kunitconfig=kernel/kcsan
> > --arch=x86_64 --qemu_args='-smp 8'
> >
> > CONFIG_KUNIT=y
> >
> > CONFIG_DEBUG_KERNEL=y
> >
> > CONFIG_KCSAN=y
> > CONFIG_KCSAN_KUNIT_TEST=y
> >
> > # Need some level of concurrency to test a concurrency sanitizer.
> > CONFIG_SMP=y
> >
> > # This prevents the test from timing out on many setups. Feel free to remove
> > # (or alter) this, in conjunction with setting a different test timeout with,
> > # for example, the --timeout kunit_tool option.
> > CONFIG_KCSAN_REPORT_ONCE_IN_MS=100
Thanks everyone. I've sent out a v2 with just this patch here:
https://lore.kernel.org/linux-kselftest/[email protected]/
I expect we'll take it in via the KUnit branch, as it's most useful
with the --qemu_args option.
Cheers,
-- David