diff mbox series

[2/2] kcsan: test: Add a .kunitconfig to run KCSAN tests

Message ID 20220518073232.526443-2-davidgow@google.com (mailing list archive)
State New
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 .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 <davidgow@google.com>
---
 kernel/kcsan/.kunitconfig | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)
 create mode 100644 kernel/kcsan/.kunitconfig

Comments

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

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

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
>
Daniel Latypov May 18, 2022, 5:12 p.m. UTC | #2
On Wed, May 18, 2022 at 12:32 AM David Gow <davidgow@google.com> 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/20220518170124.2849497-1-dlatypov@google.com

> +
> +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
David Gow May 19, 2022, 1:08 p.m. UTC | #3
On Wed, May 18, 2022 at 5:21 PM Marco Elver <elver@google.com> 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 <davidgow@google.com>
>
> Reviewed-by: Marco Elver <elver@google.com>
>
> 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
> >
Marco Elver May 19, 2022, 1:24 p.m. UTC | #4
On Thu, 19 May 2022 at 15:08, David Gow <davidgow@google.com> wrote:
>
> On Wed, May 18, 2022 at 5:21 PM Marco Elver <elver@google.com> 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 <davidgow@google.com>
> >
> > Reviewed-by: Marco Elver <elver@google.com>
> >
> > 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
Brendan Higgins July 6, 2022, 7:53 p.m. UTC | #5
On Wed, May 18, 2022 at 3:32 AM David Gow <davidgow@google.com> 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 <davidgow@google.com>

Ack, but I think Marco settled on removing CONFIG_KCSAN_STRICT=y and
CONFIG_KCSAN_WEAK_MEMORY=y.

Acked-by: Brendan Higgins <brendanhiggins@google.com>
Daniel Latypov July 14, 2022, 8:22 p.m. UTC | #6
On Thu, May 19, 2022 at 6:24 AM Marco Elver <elver@google.com> 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
Marco Elver July 14, 2022, 9:40 p.m. UTC | #7
On Thu, 14 Jul 2022 at 22:23, Daniel Latypov <dlatypov@google.com> wrote:
>
> On Thu, May 19, 2022 at 6:24 AM Marco Elver <elver@google.com> 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
Daniel Latypov July 14, 2022, 11:45 p.m. UTC | #8
On Thu, Jul 14, 2022 at 2:41 PM Marco Elver <elver@google.com> wrote:
>
> On Thu, 14 Jul 2022 at 22:23, Daniel Latypov <dlatypov@google.com> wrote:
> >
> > On Thu, May 19, 2022 at 6:24 AM Marco Elver <elver@google.com> 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
Daniel Latypov July 14, 2022, 11:47 p.m. UTC | #9
On Thu, Jul 14, 2022 at 4:45 PM Daniel Latypov <dlatypov@google.com> 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
David Gow July 15, 2022, 6:49 a.m. UTC | #10
On Fri, Jul 15, 2022 at 7:48 AM Daniel Latypov <dlatypov@google.com> wrote:
>
> On Thu, Jul 14, 2022 at 4:45 PM Daniel Latypov <dlatypov@google.com> 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/20220715064052.2673958-1-davidgow@google.com/

I expect we'll take it in via the KUnit branch, as it's most useful
with the --qemu_args option.

Cheers,
-- David
diff mbox series

Patch

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