Re: [PATCH v3 1/2] kunit: add a KUnit test for SLUB debugging functionality
On Thu, Apr 8, 2021 at 10:19 AM Daniel Latypov wrote:
>
> On Thu, Apr 8, 2021 at 3:30 AM Marco Elver wrote:
> >
> > On Tue, 6 Apr 2021 at 12:57, Vlastimil Babka wrote:
> > >
> > >
> > > On 4/1/21 11:24 PM, Marco Elver wrote:
> > > > On Thu, 1 Apr 2021 at 21:04, Daniel Latypov wrote:
> > > >> > }
> > > >> > #else
> > > >> > static inline bool slab_add_kunit_errors(void) { return
> > > >> > false; }
> > > >> > #endif
> > > >> >
> > > >> > And anywhere you want to increase the error count, you'd call
> > > >> > slab_add_kunit_errors().
> > > >> >
> > > >> > Another benefit of this approach is that if KUnit is disabled, there
> > > >> > is
> > > >> > zero overhead and no additional code generated (vs. the current
> > > >> > approach).
> > > >>
> > > >> The resource approach looks really good, but...
> > > >> You'd be picking up a dependency on
> > > >> https://lore.kernel.org/linux-kselftest/20210311152314.3814916-2-dlaty...@google.com/
> > > >> current->kunit_test will always be NULL unless CONFIG_KASAN=y &&
> > > >> CONFIG_KUNIT=y at the moment.
> > > >> My patch drops the CONFIG_KASAN requirement and opens it up to all
> > > >> tests.
> > > >
> > > > Oh, that's a shame, but hopefully it'll be in -next soon.
> > > >
> > > >> At the moment, it's just waiting another look over from Brendan or
> > > >> David.
> > > >> Any ETA on that, folks? :)
> > > >>
> > > >> So if you don't want to get blocked on that for now, I think it's fine
> > > >> to add:
> > > >> #ifdef CONFIG_SLUB_KUNIT_TEST
> > > >> int errors;
> > > >> #endif
> > > >
> > > > Until kunit fixes setting current->kunit_test, a cleaner workaround
> > > > that would allow to do the patch with kunit_resource, is to just have
> > > > an .init/.exit function that sets it ("current->kunit_test = test;").
> > > > And then perhaps add a note ("FIXME: ...") to remove it once the above
> > > > patch has landed.
> > > >
> > > > At least that way we get the least intrusive change for mm/slub.c, and
> > > > the test is the only thing that needs a 2-line patch to clean up
> > > > later.
> > >
> > > So when testing internally Oliver's new version with your suggestions
> > > (thanks
> > > again for those), I got lockdep splats because slab_add_kunit_errors is
> > > called
> > > also from irq disabled contexts, and kunit_find_named_resource will call
> > > spin_lock(>lock) that's not irq safe. Can we make the lock irq
> > > safe? I
> > > tried the change below and it makde the problem go away. If you agree, the
> > > question is how to proceed - make it part of Oliver's patch series and let
> > > Andrew pick it all with eventually kunit team's acks on this patch, or
> > > whatnot.
> >
> > From what I can tell it should be fine to make it irq safe (ack for
> > your patch below). Regarding patch logistics, I'd probably add it to
> > the series. If that ends up not working, we'll find out sooner or
> > later.
> >
> > (FYI, the prerequisite patch for current->kunit_test is in -next now.)
>
> Yep.
> There's also two follow-up patches in
> https://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git/log/?h=kunit
>
> >
> > KUnit maintainers, do you have any preferences?
>
> Poked offline and Brendan and David seemed fine either way.
> So probably just include it in this patch series for convenience.
>
> Brendan also mentioned KUnit used to use spin_lock_irqsave/restore()
> but had been told to not use it until necessary.
> See
> https://lore.kernel.org/linux-kselftest/20181016235120.138227-3-brendanhigg...@google.com/
> So I think there's no objections to the patch itself either.
>
> But I'd wait for Brendan to chime in to confirm.
That's correct. Before KUnit was accepted upstream, early versions of
the patchset used the irqsave/restore versions. I was asked to remove
them until they were necessary, and it looks like that time is now :-)
So yes, I would be happy to see this patch go in. Looks good to me the
way you have it below. Send it out as its own patch in your series and
I will give it a Reviewed-by.
Thanks!
> > > 8<
> > >
> > > commit ab28505477892e9824c57ac338c88aec2ec0abce
> > > Author: Vlastimil Babka
> > > Date: Tue Apr 6 12:28:07 2021 +0200
> > >
> > > kunit: make test->lock irq safe
> > >
> > > diff --git a/include/kunit/test.h b/include/kunit/test.h
> > > index 49601c4b98b8..524d4789af22 100644
> > > --- a/include/kunit/test.h
> > > +++ b/include/kunit/test.h
> > > @@ -515,8 +515,9 @@ kunit_find_resource(struct kunit *test,
> > > void *match_data)
> > > {
> > > struct kunit_resource *res, *found = NULL;
> > > + unsigned long flags;
> > >
> > > - spin_lock(>lock);
> > > + spin_lock_irqsave(>lock, flags);
> > >
> > > list_for_each_entry_reverse(res, >resources, node) {
> > > if (match(test, res, (void *)match_data)) {
> > > @@ -526,7 +527,7 @@ kunit_find_resource(struct kunit *test,
> > > }
>
Re: [PATCH v3 1/2] kunit: add a KUnit test for SLUB debugging functionality
On Thu, Apr 8, 2021 at 3:30 AM Marco Elver wrote:
>
> On Tue, 6 Apr 2021 at 12:57, Vlastimil Babka wrote:
> >
> >
> > On 4/1/21 11:24 PM, Marco Elver wrote:
> > > On Thu, 1 Apr 2021 at 21:04, Daniel Latypov wrote:
> > >> > }
> > >> > #else
> > >> > static inline bool slab_add_kunit_errors(void) { return false;
> > >> > }
> > >> > #endif
> > >> >
> > >> > And anywhere you want to increase the error count, you'd call
> > >> > slab_add_kunit_errors().
> > >> >
> > >> > Another benefit of this approach is that if KUnit is disabled, there is
> > >> > zero overhead and no additional code generated (vs. the current
> > >> > approach).
> > >>
> > >> The resource approach looks really good, but...
> > >> You'd be picking up a dependency on
> > >> https://lore.kernel.org/linux-kselftest/20210311152314.3814916-2-dlaty...@google.com/
> > >> current->kunit_test will always be NULL unless CONFIG_KASAN=y &&
> > >> CONFIG_KUNIT=y at the moment.
> > >> My patch drops the CONFIG_KASAN requirement and opens it up to all tests.
> > >
> > > Oh, that's a shame, but hopefully it'll be in -next soon.
> > >
> > >> At the moment, it's just waiting another look over from Brendan or David.
> > >> Any ETA on that, folks? :)
> > >>
> > >> So if you don't want to get blocked on that for now, I think it's fine
> > >> to add:
> > >> #ifdef CONFIG_SLUB_KUNIT_TEST
> > >> int errors;
> > >> #endif
> > >
> > > Until kunit fixes setting current->kunit_test, a cleaner workaround
> > > that would allow to do the patch with kunit_resource, is to just have
> > > an .init/.exit function that sets it ("current->kunit_test = test;").
> > > And then perhaps add a note ("FIXME: ...") to remove it once the above
> > > patch has landed.
> > >
> > > At least that way we get the least intrusive change for mm/slub.c, and
> > > the test is the only thing that needs a 2-line patch to clean up
> > > later.
> >
> > So when testing internally Oliver's new version with your suggestions
> > (thanks
> > again for those), I got lockdep splats because slab_add_kunit_errors is
> > called
> > also from irq disabled contexts, and kunit_find_named_resource will call
> > spin_lock(>lock) that's not irq safe. Can we make the lock irq safe? I
> > tried the change below and it makde the problem go away. If you agree, the
> > question is how to proceed - make it part of Oliver's patch series and let
> > Andrew pick it all with eventually kunit team's acks on this patch, or
> > whatnot.
>
> From what I can tell it should be fine to make it irq safe (ack for
> your patch below). Regarding patch logistics, I'd probably add it to
> the series. If that ends up not working, we'll find out sooner or
> later.
>
> (FYI, the prerequisite patch for current->kunit_test is in -next now.)
Yep.
There's also two follow-up patches in
https://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git/log/?h=kunit
>
> KUnit maintainers, do you have any preferences?
Poked offline and Brendan and David seemed fine either way.
So probably just include it in this patch series for convenience.
Brendan also mentioned KUnit used to use spin_lock_irqsave/restore()
but had been told to not use it until necessary.
See
https://lore.kernel.org/linux-kselftest/20181016235120.138227-3-brendanhigg...@google.com/
So I think there's no objections to the patch itself either.
But I'd wait for Brendan to chime in to confirm.
>
> > 8<
> >
> > commit ab28505477892e9824c57ac338c88aec2ec0abce
> > Author: Vlastimil Babka
> > Date: Tue Apr 6 12:28:07 2021 +0200
> >
> > kunit: make test->lock irq safe
> >
> > diff --git a/include/kunit/test.h b/include/kunit/test.h
> > index 49601c4b98b8..524d4789af22 100644
> > --- a/include/kunit/test.h
> > +++ b/include/kunit/test.h
> > @@ -515,8 +515,9 @@ kunit_find_resource(struct kunit *test,
> > void *match_data)
> > {
> > struct kunit_resource *res, *found = NULL;
> > + unsigned long flags;
> >
> > - spin_lock(>lock);
> > + spin_lock_irqsave(>lock, flags);
> >
> > list_for_each_entry_reverse(res, >resources, node) {
> > if (match(test, res, (void *)match_data)) {
> > @@ -526,7 +527,7 @@ kunit_find_resource(struct kunit *test,
> > }
> > }
> >
> > - spin_unlock(>lock);
> > + spin_unlock_irqrestore(>lock, flags);
> >
> > return found;
> > }
> > diff --git a/lib/kunit/test.c b/lib/kunit/test.c
> > index ec9494e914ef..2c62eeb45b82 100644
> > --- a/lib/kunit/test.c
> > +++ b/lib/kunit/test.c
> > @@ -442,6 +442,7 @@ int kunit_add_resource(struct kunit *test,
> >void *data)
> > {
> > int ret = 0;
> > + unsigned long flags;
> >
> > res->free = free;
> > kref_init(>refcount);
> > @@ -454,10 +455,10 @@ int kunit_add_resource(struct kunit *test,
> > res->data = data;
> > }
> >
> > - spin_lock(>lock);
> > +
Re: [PATCH v3 1/2] kunit: add a KUnit test for SLUB debugging functionality
On Tue, 6 Apr 2021 at 12:57, Vlastimil Babka wrote:
>
>
> On 4/1/21 11:24 PM, Marco Elver wrote:
> > On Thu, 1 Apr 2021 at 21:04, Daniel Latypov wrote:
> >> > }
> >> > #else
> >> > static inline bool slab_add_kunit_errors(void) { return false; }
> >> > #endif
> >> >
> >> > And anywhere you want to increase the error count, you'd call
> >> > slab_add_kunit_errors().
> >> >
> >> > Another benefit of this approach is that if KUnit is disabled, there is
> >> > zero overhead and no additional code generated (vs. the current
> >> > approach).
> >>
> >> The resource approach looks really good, but...
> >> You'd be picking up a dependency on
> >> https://lore.kernel.org/linux-kselftest/20210311152314.3814916-2-dlaty...@google.com/
> >> current->kunit_test will always be NULL unless CONFIG_KASAN=y &&
> >> CONFIG_KUNIT=y at the moment.
> >> My patch drops the CONFIG_KASAN requirement and opens it up to all tests.
> >
> > Oh, that's a shame, but hopefully it'll be in -next soon.
> >
> >> At the moment, it's just waiting another look over from Brendan or David.
> >> Any ETA on that, folks? :)
> >>
> >> So if you don't want to get blocked on that for now, I think it's fine to
> >> add:
> >> #ifdef CONFIG_SLUB_KUNIT_TEST
> >> int errors;
> >> #endif
> >
> > Until kunit fixes setting current->kunit_test, a cleaner workaround
> > that would allow to do the patch with kunit_resource, is to just have
> > an .init/.exit function that sets it ("current->kunit_test = test;").
> > And then perhaps add a note ("FIXME: ...") to remove it once the above
> > patch has landed.
> >
> > At least that way we get the least intrusive change for mm/slub.c, and
> > the test is the only thing that needs a 2-line patch to clean up
> > later.
>
> So when testing internally Oliver's new version with your suggestions (thanks
> again for those), I got lockdep splats because slab_add_kunit_errors is called
> also from irq disabled contexts, and kunit_find_named_resource will call
> spin_lock(>lock) that's not irq safe. Can we make the lock irq safe? I
> tried the change below and it makde the problem go away. If you agree, the
> question is how to proceed - make it part of Oliver's patch series and let
> Andrew pick it all with eventually kunit team's acks on this patch, or
> whatnot.
>From what I can tell it should be fine to make it irq safe (ack for
your patch below). Regarding patch logistics, I'd probably add it to
the series. If that ends up not working, we'll find out sooner or
later.
(FYI, the prerequisite patch for current->kunit_test is in -next now.)
KUnit maintainers, do you have any preferences?
> 8<
>
> commit ab28505477892e9824c57ac338c88aec2ec0abce
> Author: Vlastimil Babka
> Date: Tue Apr 6 12:28:07 2021 +0200
>
> kunit: make test->lock irq safe
>
> diff --git a/include/kunit/test.h b/include/kunit/test.h
> index 49601c4b98b8..524d4789af22 100644
> --- a/include/kunit/test.h
> +++ b/include/kunit/test.h
> @@ -515,8 +515,9 @@ kunit_find_resource(struct kunit *test,
> void *match_data)
> {
> struct kunit_resource *res, *found = NULL;
> + unsigned long flags;
>
> - spin_lock(>lock);
> + spin_lock_irqsave(>lock, flags);
>
> list_for_each_entry_reverse(res, >resources, node) {
> if (match(test, res, (void *)match_data)) {
> @@ -526,7 +527,7 @@ kunit_find_resource(struct kunit *test,
> }
> }
>
> - spin_unlock(>lock);
> + spin_unlock_irqrestore(>lock, flags);
>
> return found;
> }
> diff --git a/lib/kunit/test.c b/lib/kunit/test.c
> index ec9494e914ef..2c62eeb45b82 100644
> --- a/lib/kunit/test.c
> +++ b/lib/kunit/test.c
> @@ -442,6 +442,7 @@ int kunit_add_resource(struct kunit *test,
>void *data)
> {
> int ret = 0;
> + unsigned long flags;
>
> res->free = free;
> kref_init(>refcount);
> @@ -454,10 +455,10 @@ int kunit_add_resource(struct kunit *test,
> res->data = data;
> }
>
> - spin_lock(>lock);
> + spin_lock_irqsave(>lock, flags);
> list_add_tail(>node, >resources);
> /* refcount for list is established by kref_init() */
> - spin_unlock(>lock);
> + spin_unlock_irqrestore(>lock, flags);
>
> return ret;
> }
> @@ -515,9 +516,11 @@ EXPORT_SYMBOL_GPL(kunit_alloc_and_get_resource);
>
> void kunit_remove_resource(struct kunit *test, struct kunit_resource *res)
> {
> - spin_lock(>lock);
> + unsigned long flags;
> +
> + spin_lock_irqsave(>lock, flags);
> list_del(>node);
> - spin_unlock(>lock);
> + spin_unlock_irqrestore(>lock, flags);
> kunit_put_resource(res);
> }
> EXPORT_SYMBOL_GPL(kunit_remove_resource);
> @@ -597,6 +600,7 @@ EXPORT_SYMBOL_GPL(kunit_kfree);
> void kunit_cleanup(struct kunit *test)
> {
> struct kunit_resource *res;
> + unsigned long flags;
>
> /*
>
Re: [PATCH v3 1/2] kunit: add a KUnit test for SLUB debugging functionality
On 4/1/21 11:24 PM, Marco Elver wrote:
> On Thu, 1 Apr 2021 at 21:04, Daniel Latypov wrote:
>> > }
>> > #else
>> > static inline bool slab_add_kunit_errors(void) { return false; }
>> > #endif
>> >
>> > And anywhere you want to increase the error count, you'd call
>> > slab_add_kunit_errors().
>> >
>> > Another benefit of this approach is that if KUnit is disabled, there is
>> > zero overhead and no additional code generated (vs. the current
>> > approach).
>>
>> The resource approach looks really good, but...
>> You'd be picking up a dependency on
>> https://lore.kernel.org/linux-kselftest/20210311152314.3814916-2-dlaty...@google.com/
>> current->kunit_test will always be NULL unless CONFIG_KASAN=y &&
>> CONFIG_KUNIT=y at the moment.
>> My patch drops the CONFIG_KASAN requirement and opens it up to all tests.
>
> Oh, that's a shame, but hopefully it'll be in -next soon.
>
>> At the moment, it's just waiting another look over from Brendan or David.
>> Any ETA on that, folks? :)
>>
>> So if you don't want to get blocked on that for now, I think it's fine to
>> add:
>> #ifdef CONFIG_SLUB_KUNIT_TEST
>> int errors;
>> #endif
>
> Until kunit fixes setting current->kunit_test, a cleaner workaround
> that would allow to do the patch with kunit_resource, is to just have
> an .init/.exit function that sets it ("current->kunit_test = test;").
> And then perhaps add a note ("FIXME: ...") to remove it once the above
> patch has landed.
>
> At least that way we get the least intrusive change for mm/slub.c, and
> the test is the only thing that needs a 2-line patch to clean up
> later.
So when testing internally Oliver's new version with your suggestions (thanks
again for those), I got lockdep splats because slab_add_kunit_errors is called
also from irq disabled contexts, and kunit_find_named_resource will call
spin_lock(>lock) that's not irq safe. Can we make the lock irq safe? I
tried the change below and it makde the problem go away. If you agree, the
question is how to proceed - make it part of Oliver's patch series and let
Andrew pick it all with eventually kunit team's acks on this patch, or whatnot.
8<
commit ab28505477892e9824c57ac338c88aec2ec0abce
Author: Vlastimil Babka
Date: Tue Apr 6 12:28:07 2021 +0200
kunit: make test->lock irq safe
diff --git a/include/kunit/test.h b/include/kunit/test.h
index 49601c4b98b8..524d4789af22 100644
--- a/include/kunit/test.h
+++ b/include/kunit/test.h
@@ -515,8 +515,9 @@ kunit_find_resource(struct kunit *test,
void *match_data)
{
struct kunit_resource *res, *found = NULL;
+ unsigned long flags;
- spin_lock(>lock);
+ spin_lock_irqsave(>lock, flags);
list_for_each_entry_reverse(res, >resources, node) {
if (match(test, res, (void *)match_data)) {
@@ -526,7 +527,7 @@ kunit_find_resource(struct kunit *test,
}
}
- spin_unlock(>lock);
+ spin_unlock_irqrestore(>lock, flags);
return found;
}
diff --git a/lib/kunit/test.c b/lib/kunit/test.c
index ec9494e914ef..2c62eeb45b82 100644
--- a/lib/kunit/test.c
+++ b/lib/kunit/test.c
@@ -442,6 +442,7 @@ int kunit_add_resource(struct kunit *test,
void *data)
{
int ret = 0;
+ unsigned long flags;
res->free = free;
kref_init(>refcount);
@@ -454,10 +455,10 @@ int kunit_add_resource(struct kunit *test,
res->data = data;
}
- spin_lock(>lock);
+ spin_lock_irqsave(>lock, flags);
list_add_tail(>node, >resources);
/* refcount for list is established by kref_init() */
- spin_unlock(>lock);
+ spin_unlock_irqrestore(>lock, flags);
return ret;
}
@@ -515,9 +516,11 @@ EXPORT_SYMBOL_GPL(kunit_alloc_and_get_resource);
void kunit_remove_resource(struct kunit *test, struct kunit_resource *res)
{
- spin_lock(>lock);
+ unsigned long flags;
+
+ spin_lock_irqsave(>lock, flags);
list_del(>node);
- spin_unlock(>lock);
+ spin_unlock_irqrestore(>lock, flags);
kunit_put_resource(res);
}
EXPORT_SYMBOL_GPL(kunit_remove_resource);
@@ -597,6 +600,7 @@ EXPORT_SYMBOL_GPL(kunit_kfree);
void kunit_cleanup(struct kunit *test)
{
struct kunit_resource *res;
+ unsigned long flags;
/*
* test->resources is a stack - each allocation must be freed in the
@@ -608,9 +612,9 @@ void kunit_cleanup(struct kunit *test)
* protect against the current node being deleted, not the next.
*/
while (true) {
- spin_lock(>lock);
+ spin_lock_irqsave(>lock, flags);
if (list_empty(>resources)) {
- spin_unlock(>lock);
+ spin_unlock_irqrestore(>lock, flags);
break;
}
res = list_last_entry(>resources,
@@ -621,7 +625,7 @@ void
Re: [PATCH v3 1/2] kunit: add a KUnit test for SLUB debugging functionality
On Thu, Apr 1, 2021 at 12:04 PM 'Daniel Latypov' via KUnit Development
wrote:
>
> On Thu, Apr 1, 2021 at 2:16 AM 'Marco Elver' via KUnit Development
> wrote:
[...]
> > #else
> > static inline bool slab_add_kunit_errors(void) { return false; }
> > #endif
> >
> > And anywhere you want to increase the error count, you'd call
> > slab_add_kunit_errors().
> >
> > Another benefit of this approach is that if KUnit is disabled, there is
> > zero overhead and no additional code generated (vs. the current
> > approach).
>
> The resource approach looks really good, but...
> You'd be picking up a dependency on
> https://lore.kernel.org/linux-kselftest/20210311152314.3814916-2-dlaty...@google.com/
> current->kunit_test will always be NULL unless CONFIG_KASAN=y &&
> CONFIG_KUNIT=y at the moment.
> My patch drops the CONFIG_KASAN requirement and opens it up to all tests.
>
> At the moment, it's just waiting another look over from Brendan or David.
> Any ETA on that, folks? :)
I just gave a "Reviewed-by" and sent it off to Shuah. Should be
available in 5.13.
Cheers
Re: [PATCH v3 1/2] kunit: add a KUnit test for SLUB debugging functionality
On Thu, 1 Apr 2021 at 21:04, Daniel Latypov wrote:
...
> > > --- a/include/linux/slub_def.h
> > > +++ b/include/linux/slub_def.h
> > > @@ -133,6 +133,8 @@ struct kmem_cache {
> > > unsigned int usersize; /* Usercopy region size */
> > >
> > > struct kmem_cache_node *node[MAX_NUMNODES];
> > > +
> > > + int errors; /* Number of errors in cache */
> >
> > So, I think it's bad design to add a new field 'errors', just for the
> > test. This will increase kmem_cache size for all builds, which is
> > unnecessary.
> >
> > Is there use to retrieve 'errors' elsewhere?
> >
> > While you could guard this with #ifdef CONFIG_SLUB_DEBUG or so, there's
> > a better design option if this is just for the KUnit test's benefit: use
> > kunit_resource.
> >
> > The way it'd work is that for each test (you can add a common init
> > function) you add a named resource, in this case just an 'int' I guess,
> > that slab would be able to retrieve if this test is being run.
> >
> > In the test somewhere, you could add something like this:
> >
> >
> > static struct kunit_resource resource;
> > static int slab_errors;
> >
> > ..
> >
> > static int test_init(struct kunit *test)
> > {
> > slab_errors = 0;
> > kunit_add_named_resource(test, NULL, NULL, ,
> > "slab_errors", _errors);
> > return 0;
> > }
> >
> > .. tests now check slab_errors .
> >
> > and then in slub.c you'd have:
> >
> > #if IS_ENABLED(CONFIG_KUNIT)
> > static bool slab_add_kunit_errors(void)
> > {
> > struct kunit_resource *resource;
> >
> > if (likely(!current->kunit_test))
> > return false;
> > resource = kunit_find_named_resource(current->kunit_test,
> > "slab_errors");
> > if (!resource)
> > return false;
> > (*(int *)resource->data)++;
> > kunit_put_resource(resource);
return true;
was missing.
> > }
> > #else
> > static inline bool slab_add_kunit_errors(void) { return false; }
> > #endif
> >
> > And anywhere you want to increase the error count, you'd call
> > slab_add_kunit_errors().
> >
> > Another benefit of this approach is that if KUnit is disabled, there is
> > zero overhead and no additional code generated (vs. the current
> > approach).
>
> The resource approach looks really good, but...
> You'd be picking up a dependency on
> https://lore.kernel.org/linux-kselftest/20210311152314.3814916-2-dlaty...@google.com/
> current->kunit_test will always be NULL unless CONFIG_KASAN=y &&
> CONFIG_KUNIT=y at the moment.
> My patch drops the CONFIG_KASAN requirement and opens it up to all tests.
Oh, that's a shame, but hopefully it'll be in -next soon.
> At the moment, it's just waiting another look over from Brendan or David.
> Any ETA on that, folks? :)
>
> So if you don't want to get blocked on that for now, I think it's fine to add:
> #ifdef CONFIG_SLUB_KUNIT_TEST
> int errors;
> #endif
Until kunit fixes setting current->kunit_test, a cleaner workaround
that would allow to do the patch with kunit_resource, is to just have
an .init/.exit function that sets it ("current->kunit_test = test;").
And then perhaps add a note ("FIXME: ...") to remove it once the above
patch has landed.
At least that way we get the least intrusive change for mm/slub.c, and
the test is the only thing that needs a 2-line patch to clean up
later.
Thanks,
-- Marco
Re: [PATCH v3 1/2] kunit: add a KUnit test for SLUB debugging functionality
On Thu, Apr 1, 2021 at 2:16 AM 'Marco Elver' via KUnit Development
wrote:
>
> [Note, if you'd like me to see future versions, please Cc me, otherwise
> it's unlikely I see it in time. Also add kunit-...@googlegroups.com if
> perhaps a KUnit dev should have another look, too.]
>
> On Wed, Mar 31, 2021 at 10:51AM +0200, glit...@gmail.com wrote:
> > From: Oliver Glitta
> >
> > SLUB has resiliency_test() function which is hidden behind #ifdef
> > SLUB_RESILIENCY_TEST that is not part of Kconfig, so nobody
> > runs it. KUnit should be a proper replacement for it.
> >
> > Try changing byte in redzone after allocation and changing
> > pointer to next free node, first byte, 50th byte and redzone
> > byte. Check if validation finds errors.
> >
> > There are several differences from the original resiliency test:
> > Tests create own caches with known state instead of corrupting
> > shared kmalloc caches.
> >
> > The corruption of freepointer uses correct offset, the original
> > resiliency test got broken with freepointer changes.
> >
> > Scratch changing random byte test, because it does not have
> > meaning in this form where we need deterministic results.
> >
> > Add new option CONFIG_SLUB_KUNIT_TEST in Kconfig.
> >
> > Add a counter field "errors" to struct kmem_cache to count number
> > of errors detected in cache.
> >
> > Silence bug report in SLUB test. Add SLAB_SILENT_ERRORS debug flag.
> > Add SLAB_SILENT_ERRORS flag to SLAB_NEVER_MERGE, SLAB_DEBUG_FLAGS,
> > SLAB_FLAGS_PERMITTED macros.
> >
> > Signed-off-by: Oliver Glitta
> > ---
> > Changes since v2
> >
> > Use bit operation & instead of logical && as reported by kernel test
> > robot and Dan Carpenter
> >
> > Changes since v1
> >
> > Conversion from kselftest to KUnit test suggested by Marco Elver.
> > Error silencing.
> > Error counting improvements.
> >
> > include/linux/slab.h | 2 +
> > include/linux/slub_def.h | 2 +
> > lib/Kconfig.debug| 5 ++
> > lib/Makefile | 1 +
> > lib/test_slub.c | 124 +++
> > mm/slab.h| 7 ++-
> > mm/slab_common.c | 2 +-
> > mm/slub.c| 64 +---
> > 8 files changed, 184 insertions(+), 23 deletions(-)
> > create mode 100644 lib/test_slub.c
> >
> > diff --git a/include/linux/slab.h b/include/linux/slab.h
> > index 7ae604076767..ed1a5a64d028 100644
> > --- a/include/linux/slab.h
> > +++ b/include/linux/slab.h
> > @@ -25,6 +25,8 @@
> > */
> > /* DEBUG: Perform (expensive) checks on alloc/free */
> > #define SLAB_CONSISTENCY_CHECKS ((slab_flags_t __force)0x0100U)
> > +/* DEBUG: Silent bug reports */
> > +#define SLAB_SILENT_ERRORS ((slab_flags_t __force)0x0200U)
>
> This flag wouldn't be necessary if you do the design using
> kunit_resource (see below).
>
> (But perhaps I missed a conversation that said that this flag is
> generally useful, but if so, it should probably be in a separate patch
> justifying why it is required beyond the test.)
>
> > /* DEBUG: Red zone objs in a cache */
> > #define SLAB_RED_ZONE((slab_flags_t __force)0x0400U)
> > /* DEBUG: Poison objects */
> > diff --git a/include/linux/slub_def.h b/include/linux/slub_def.h
> > index dcde82a4434c..e4b51bb5bb83 100644
> > --- a/include/linux/slub_def.h
> > +++ b/include/linux/slub_def.h
> > @@ -133,6 +133,8 @@ struct kmem_cache {
> > unsigned int usersize; /* Usercopy region size */
> >
> > struct kmem_cache_node *node[MAX_NUMNODES];
> > +
> > + int errors; /* Number of errors in cache */
>
> So, I think it's bad design to add a new field 'errors', just for the
> test. This will increase kmem_cache size for all builds, which is
> unnecessary.
>
> Is there use to retrieve 'errors' elsewhere?
>
> While you could guard this with #ifdef CONFIG_SLUB_DEBUG or so, there's
> a better design option if this is just for the KUnit test's benefit: use
> kunit_resource.
>
> The way it'd work is that for each test (you can add a common init
> function) you add a named resource, in this case just an 'int' I guess,
> that slab would be able to retrieve if this test is being run.
>
> In the test somewhere, you could add something like this:
>
>
> static struct kunit_resource resource;
> static int slab_errors;
>
> ..
>
> static int test_init(struct kunit *test)
> {
> slab_errors = 0;
> kunit_add_named_resource(test, NULL, NULL, ,
> "slab_errors", _errors);
> return 0;
> }
>
> .. tests now check slab_errors .
>
> and then in slub.c you'd have:
>
> #if IS_ENABLED(CONFIG_KUNIT)
> static bool slab_add_kunit_errors(void)
> {
> struct kunit_resource *resource;
>
> if (likely(!current->kunit_test))
> return false;
>
Re: [PATCH v3 1/2] kunit: add a KUnit test for SLUB debugging functionality
[Note, if you'd like me to see future versions, please Cc me, otherwise
it's unlikely I see it in time. Also add kunit-...@googlegroups.com if
perhaps a KUnit dev should have another look, too.]
On Wed, Mar 31, 2021 at 10:51AM +0200, glit...@gmail.com wrote:
> From: Oliver Glitta
>
> SLUB has resiliency_test() function which is hidden behind #ifdef
> SLUB_RESILIENCY_TEST that is not part of Kconfig, so nobody
> runs it. KUnit should be a proper replacement for it.
>
> Try changing byte in redzone after allocation and changing
> pointer to next free node, first byte, 50th byte and redzone
> byte. Check if validation finds errors.
>
> There are several differences from the original resiliency test:
> Tests create own caches with known state instead of corrupting
> shared kmalloc caches.
>
> The corruption of freepointer uses correct offset, the original
> resiliency test got broken with freepointer changes.
>
> Scratch changing random byte test, because it does not have
> meaning in this form where we need deterministic results.
>
> Add new option CONFIG_SLUB_KUNIT_TEST in Kconfig.
>
> Add a counter field "errors" to struct kmem_cache to count number
> of errors detected in cache.
>
> Silence bug report in SLUB test. Add SLAB_SILENT_ERRORS debug flag.
> Add SLAB_SILENT_ERRORS flag to SLAB_NEVER_MERGE, SLAB_DEBUG_FLAGS,
> SLAB_FLAGS_PERMITTED macros.
>
> Signed-off-by: Oliver Glitta
> ---
> Changes since v2
>
> Use bit operation & instead of logical && as reported by kernel test
> robot and Dan Carpenter
>
> Changes since v1
>
> Conversion from kselftest to KUnit test suggested by Marco Elver.
> Error silencing.
> Error counting improvements.
>
> include/linux/slab.h | 2 +
> include/linux/slub_def.h | 2 +
> lib/Kconfig.debug| 5 ++
> lib/Makefile | 1 +
> lib/test_slub.c | 124 +++
> mm/slab.h| 7 ++-
> mm/slab_common.c | 2 +-
> mm/slub.c| 64 +---
> 8 files changed, 184 insertions(+), 23 deletions(-)
> create mode 100644 lib/test_slub.c
>
> diff --git a/include/linux/slab.h b/include/linux/slab.h
> index 7ae604076767..ed1a5a64d028 100644
> --- a/include/linux/slab.h
> +++ b/include/linux/slab.h
> @@ -25,6 +25,8 @@
> */
> /* DEBUG: Perform (expensive) checks on alloc/free */
> #define SLAB_CONSISTENCY_CHECKS ((slab_flags_t __force)0x0100U)
> +/* DEBUG: Silent bug reports */
> +#define SLAB_SILENT_ERRORS ((slab_flags_t __force)0x0200U)
This flag wouldn't be necessary if you do the design using
kunit_resource (see below).
(But perhaps I missed a conversation that said that this flag is
generally useful, but if so, it should probably be in a separate patch
justifying why it is required beyond the test.)
> /* DEBUG: Red zone objs in a cache */
> #define SLAB_RED_ZONE((slab_flags_t __force)0x0400U)
> /* DEBUG: Poison objects */
> diff --git a/include/linux/slub_def.h b/include/linux/slub_def.h
> index dcde82a4434c..e4b51bb5bb83 100644
> --- a/include/linux/slub_def.h
> +++ b/include/linux/slub_def.h
> @@ -133,6 +133,8 @@ struct kmem_cache {
> unsigned int usersize; /* Usercopy region size */
>
> struct kmem_cache_node *node[MAX_NUMNODES];
> +
> + int errors; /* Number of errors in cache */
So, I think it's bad design to add a new field 'errors', just for the
test. This will increase kmem_cache size for all builds, which is
unnecessary.
Is there use to retrieve 'errors' elsewhere?
While you could guard this with #ifdef CONFIG_SLUB_DEBUG or so, there's
a better design option if this is just for the KUnit test's benefit: use
kunit_resource.
The way it'd work is that for each test (you can add a common init
function) you add a named resource, in this case just an 'int' I guess,
that slab would be able to retrieve if this test is being run.
In the test somewhere, you could add something like this:
static struct kunit_resource resource;
static int slab_errors;
..
static int test_init(struct kunit *test)
{
slab_errors = 0;
kunit_add_named_resource(test, NULL, NULL, ,
"slab_errors", _errors);
return 0;
}
.. tests now check slab_errors .
and then in slub.c you'd have:
#if IS_ENABLED(CONFIG_KUNIT)
static bool slab_add_kunit_errors(void)
{
struct kunit_resource *resource;
if (likely(!current->kunit_test))
return false;
resource = kunit_find_named_resource(current->kunit_test,
"slab_errors");
if (!resource)
return false;
(*(int *)resource->data)++;
kunit_put_resource(resource);
}
#else
static inline bool
Re: [PATCH v3 1/2] kunit: add a KUnit test for SLUB debugging functionality
On 3/31/21 10:51 AM, glit...@gmail.com wrote: > From: Oliver Glitta > > SLUB has resiliency_test() function which is hidden behind #ifdef > SLUB_RESILIENCY_TEST that is not part of Kconfig, so nobody > runs it. KUnit should be a proper replacement for it. > > Try changing byte in redzone after allocation and changing > pointer to next free node, first byte, 50th byte and redzone > byte. Check if validation finds errors. > > There are several differences from the original resiliency test: > Tests create own caches with known state instead of corrupting > shared kmalloc caches. > > The corruption of freepointer uses correct offset, the original > resiliency test got broken with freepointer changes. > > Scratch changing random byte test, because it does not have > meaning in this form where we need deterministic results. > > Add new option CONFIG_SLUB_KUNIT_TEST in Kconfig. > > Add a counter field "errors" to struct kmem_cache to count number > of errors detected in cache. > > Silence bug report in SLUB test. Add SLAB_SILENT_ERRORS debug flag. > Add SLAB_SILENT_ERRORS flag to SLAB_NEVER_MERGE, SLAB_DEBUG_FLAGS, > SLAB_FLAGS_PERMITTED macros. > > Signed-off-by: Oliver Glitta Acked-by: Vlastimil Babka
[PATCH v3 1/2] kunit: add a KUnit test for SLUB debugging functionality
From: Oliver Glitta
SLUB has resiliency_test() function which is hidden behind #ifdef
SLUB_RESILIENCY_TEST that is not part of Kconfig, so nobody
runs it. KUnit should be a proper replacement for it.
Try changing byte in redzone after allocation and changing
pointer to next free node, first byte, 50th byte and redzone
byte. Check if validation finds errors.
There are several differences from the original resiliency test:
Tests create own caches with known state instead of corrupting
shared kmalloc caches.
The corruption of freepointer uses correct offset, the original
resiliency test got broken with freepointer changes.
Scratch changing random byte test, because it does not have
meaning in this form where we need deterministic results.
Add new option CONFIG_SLUB_KUNIT_TEST in Kconfig.
Add a counter field "errors" to struct kmem_cache to count number
of errors detected in cache.
Silence bug report in SLUB test. Add SLAB_SILENT_ERRORS debug flag.
Add SLAB_SILENT_ERRORS flag to SLAB_NEVER_MERGE, SLAB_DEBUG_FLAGS,
SLAB_FLAGS_PERMITTED macros.
Signed-off-by: Oliver Glitta
---
Changes since v2
Use bit operation & instead of logical && as reported by kernel test
robot and Dan Carpenter
Changes since v1
Conversion from kselftest to KUnit test suggested by Marco Elver.
Error silencing.
Error counting improvements.
include/linux/slab.h | 2 +
include/linux/slub_def.h | 2 +
lib/Kconfig.debug| 5 ++
lib/Makefile | 1 +
lib/test_slub.c | 124 +++
mm/slab.h| 7 ++-
mm/slab_common.c | 2 +-
mm/slub.c| 64 +---
8 files changed, 184 insertions(+), 23 deletions(-)
create mode 100644 lib/test_slub.c
diff --git a/include/linux/slab.h b/include/linux/slab.h
index 7ae604076767..ed1a5a64d028 100644
--- a/include/linux/slab.h
+++ b/include/linux/slab.h
@@ -25,6 +25,8 @@
*/
/* DEBUG: Perform (expensive) checks on alloc/free */
#define SLAB_CONSISTENCY_CHECKS((slab_flags_t __force)0x0100U)
+/* DEBUG: Silent bug reports */
+#define SLAB_SILENT_ERRORS ((slab_flags_t __force)0x0200U)
/* DEBUG: Red zone objs in a cache */
#define SLAB_RED_ZONE ((slab_flags_t __force)0x0400U)
/* DEBUG: Poison objects */
diff --git a/include/linux/slub_def.h b/include/linux/slub_def.h
index dcde82a4434c..e4b51bb5bb83 100644
--- a/include/linux/slub_def.h
+++ b/include/linux/slub_def.h
@@ -133,6 +133,8 @@ struct kmem_cache {
unsigned int usersize; /* Usercopy region size */
struct kmem_cache_node *node[MAX_NUMNODES];
+
+ int errors; /* Number of errors in cache */
};
#ifdef CONFIG_SLUB_CPU_PARTIAL
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 2779c29d9981..e0dec830c269 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -2371,6 +2371,11 @@ config BITS_TEST
If unsure, say N.
+config SLUB_KUNIT_TEST
+ tristate "KUnit Test for SLUB cache error detection" if !KUNIT_ALL_TESTS
+ depends on SLUB_DEBUG && KUNIT
+ default KUNIT_ALL_TESTS
+
config TEST_UDELAY
tristate "udelay test driver"
help
diff --git a/lib/Makefile b/lib/Makefile
index b5307d3eec1a..e1eb986c0e87 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -352,5 +352,6 @@ obj-$(CONFIG_LIST_KUNIT_TEST) += list-test.o
obj-$(CONFIG_LINEAR_RANGES_TEST) += test_linear_ranges.o
obj-$(CONFIG_BITS_TEST) += test_bits.o
obj-$(CONFIG_CMDLINE_KUNIT_TEST) += cmdline_kunit.o
+obj-$(CONFIG_SLUB_KUNIT_TEST) += test_slub.o
obj-$(CONFIG_GENERIC_LIB_DEVMEM_IS_ALLOWED) += devmem_is_allowed.o
diff --git a/lib/test_slub.c b/lib/test_slub.c
new file mode 100644
index ..4f8ea3c7d867
--- /dev/null
+++ b/lib/test_slub.c
@@ -0,0 +1,124 @@
+// SPDX-License-Identifier: GPL-2.0
+#include
+#include
+#include
+#include
+#include
+#include "../mm/slab.h"
+
+
+static void test_clobber_zone(struct kunit *test)
+{
+ struct kmem_cache *s = kmem_cache_create("TestSlub_RZ_alloc", 64, 0,
+ SLAB_RED_ZONE | SLAB_SILENT_ERRORS, NULL);
+ u8 *p = kmem_cache_alloc(s, GFP_KERNEL);
+
+ p[64] = 0x12;
+
+ validate_slab_cache(s);
+ KUNIT_EXPECT_EQ(test, 1, s->errors);
+
+ kmem_cache_free(s, p);
+ kmem_cache_destroy(s);
+}
+
+static void test_next_pointer(struct kunit *test)
+{
+ struct kmem_cache *s = kmem_cache_create("TestSlub_next_ptr_free", 64,
0,
+ SLAB_POISON | SLAB_SILENT_ERRORS, NULL);
+ u8 *p = kmem_cache_alloc(s, GFP_KERNEL);
+ unsigned long tmp;
+ unsigned long *ptr_addr;
+
+ kmem_cache_free(s, p);
+
+ ptr_addr = (unsigned long *)(p + s->offset);
+ tmp = *ptr_addr;
+ p[s->offset] = 0x12;
+
+ /*
+* Expecting two errors.
+* One for the corrupted freechain and the other one for the wrong
+* count of objects in use.
+*/
+
