| 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 |
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.
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.
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.
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.
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
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.
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
On Thu, May 19, 2022 at 1:11 PM Daniel Latypov <dlatypov@google.com> wrote: > > 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. 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/20220519164512.3180360-1-dlatypov@google.com > > It introduces an overwriting priority of > * --kconfig_add > * kunitconfig / --kunitconfig > * qemu_config
On Wed, May 18, 2022 at 3: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. > > Signed-off-by: David Gow <davidgow@google.com> 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 <brendanhiggins@google.com>
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'])
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