diff mbox series

[1/2] kunit: tool: Add x86_64-smp architecture for SMP testing

Message ID 20220518073232.526443-1-davidgow@google.com (mailing list archive)
State New
Delegated to: Brendan Higgins
Headers show
Series [1/2] kunit: tool: Add x86_64-smp architecture for SMP testing | expand

Commit Message

David Gow May 18, 2022, 7:32 a.m. UTC
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 <davidgow@google.com>
---

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

Comments

Marco Elver May 18, 2022, 9:22 a.m. UTC | #1
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 <davidgow@google.com>

Acked-by: Marco Elver <elver@google.com>

> ---
> 
> 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 kunit-dev+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20220518073232.526443-1-davidgow%40google.com.
Daniel Latypov May 18, 2022, 3:31 p.m. UTC | #2
On Wed, May 18, 2022 at 12:32 AM 'David Gow' via KUnit Development
<kunit-dev@googlegroups.com> 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.
Marco Elver May 18, 2022, 3:35 p.m. UTC | #3
On Wed, 18 May 2022 at 17:31, Daniel Latypov <dlatypov@google.com> wrote:
>
> On Wed, May 18, 2022 at 12:32 AM 'David Gow' via KUnit Development
> <kunit-dev@googlegroups.com> 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.
Daniel Latypov May 18, 2022, 3:39 p.m. UTC | #4
On Wed, May 18, 2022 at 8:36 AM Marco Elver <elver@google.com> wrote:
>
> On Wed, 18 May 2022 at 17:31, Daniel Latypov <dlatypov@google.com> wrote:
> >
> > On Wed, May 18, 2022 at 12:32 AM 'David Gow' via KUnit Development
> > <kunit-dev@googlegroups.com> 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.
Daniel Latypov May 18, 2022, 5:05 p.m. UTC | #5
On Wed, May 18, 2022 at 8:39 AM Daniel Latypov <dlatypov@google.com> 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/20220518170124.2849497-1-dlatypov@google.com
David Gow May 19, 2022, 1:15 p.m. UTC | #6
On Wed, May 18, 2022 at 11:36 PM Marco Elver <elver@google.com> wrote:
>
> On Wed, 18 May 2022 at 17:31, Daniel Latypov <dlatypov@google.com> wrote:
> >
> > On Wed, May 18, 2022 at 12:32 AM 'David Gow' via KUnit Development
> > <kunit-dev@googlegroups.com> 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.
Daniel Latypov May 19, 2022, 5:11 p.m. UTC | #7
On Thu, May 19, 2022 at 6:15 AM David Gow <davidgow@google.com> 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/20220519164512.3180360-1-dlatypov@google.com

It introduces an overwriting priority of
* --kconfig_add
* kunitconfig / --kunitconfig
* qemu_config
diff mbox series

Patch

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'])