On Tue, Apr 05, 2022 at 12:58:53PM +0200, Vlastimil Babka wrote:
> On 3/17/22 09:10, Hyeonggon Yoo wrote:
> > slub_kunit does not expect other debugging flags to be set when running
> > tests. When SLAB_RED_ZONE flag is set globally, test fails because the
> > flag affects number of errors reported.
> >
> > To make slub_kunit unaffected by global slub debugging flags, introduce
> > SLAB_NO_GLOBAL_FLAGS to ignore them. It's still allowed to specify
> > debugging flags by specifying cache name(s) in slub_debug parameter.
>
> Given how we support globbing, I think it would be safest to just ignore
> everything that comes from slub_debug parameter when the
> SLAB_NO_GLOBAL_FLAGS flag is specified, even if it involves a (partial)
> cache name match.
> Maybe name it SLAB_NO_USER_FLAGS then?
>
Seems reasonable. letting your specify debugging flags for self-test
cache is not useful for any case.
Let's make self-test caches completely isolated from user-specified flags.
I sent v2 with your comments in mind.
> > Suggested-by: Vlastimil Babka <vbabka@xxxxxxx>
> > Signed-off-by: Hyeonggon Yoo <42.hyeyoo@xxxxxxxxx>
> > ---
> > include/linux/slab.h | 3 +++
> > lib/slub_kunit.c | 10 +++++-----
> > mm/slab.h | 5 +++--
> > mm/slub.c | 3 +++
> > 4 files changed, 14 insertions(+), 7 deletions(-)
> >
> > diff --git a/include/linux/slab.h b/include/linux/slab.h
> > index 0381868e5118..11fe2c28422d 100644
> > --- a/include/linux/slab.h
> > +++ b/include/linux/slab.h
> > @@ -112,6 +112,9 @@
> > #define SLAB_KASAN 0
> > #endif
> >
> > +/* Ignore globally specified debugging flags */
>
> I'd add that this is intended for caches created for self-tests so they
> always have flags as specified in the code.
>
Did it in v2.
Thanks!
Hyeonggon
> > +#define SLAB_NO_GLOBAL_FLAGS ((slab_flags_t __force)0x10000000U)
> > +
> > /* The following flags affect the page allocator grouping pages by mobility */
> > /* Objects are reclaimable */
> > #define SLAB_RECLAIM_ACCOUNT ((slab_flags_t __force)0x00020000U)
> > diff --git a/lib/slub_kunit.c b/lib/slub_kunit.c
> > index 8662dc6cb509..acf061dc558d 100644
> > --- a/lib/slub_kunit.c
> > +++ b/lib/slub_kunit.c
> > @@ -12,7 +12,7 @@ static int slab_errors;
> > static void test_clobber_zone(struct kunit *test)
> > {
> > struct kmem_cache *s = kmem_cache_create("TestSlub_RZ_alloc", 64, 0,
> > - SLAB_RED_ZONE, NULL);
> > + SLAB_RED_ZONE|SLAB_NO_GLOBAL_FLAGS, NULL);
> > u8 *p = kmem_cache_alloc(s, GFP_KERNEL);
> >
> > kasan_disable_current();
> > @@ -30,7 +30,7 @@ static void test_clobber_zone(struct kunit *test)
> > static void test_next_pointer(struct kunit *test)
> > {
> > struct kmem_cache *s = kmem_cache_create("TestSlub_next_ptr_free", 64, 0,
> > - SLAB_POISON, NULL);
> > + SLAB_POISON|SLAB_NO_GLOBAL_FLAGS, NULL);
> > u8 *p = kmem_cache_alloc(s, GFP_KERNEL);
> > unsigned long tmp;
> > unsigned long *ptr_addr;
> > @@ -75,7 +75,7 @@ static void test_next_pointer(struct kunit *test)
> > static void test_first_word(struct kunit *test)
> > {
> > struct kmem_cache *s = kmem_cache_create("TestSlub_1th_word_free", 64, 0,
> > - SLAB_POISON, NULL);
> > + SLAB_POISON|SLAB_NO_GLOBAL_FLAGS, NULL);
> > u8 *p = kmem_cache_alloc(s, GFP_KERNEL);
> >
> > kmem_cache_free(s, p);
> > @@ -90,7 +90,7 @@ static void test_first_word(struct kunit *test)
> > static void test_clobber_50th_byte(struct kunit *test)
> > {
> > struct kmem_cache *s = kmem_cache_create("TestSlub_50th_word_free", 64, 0,
> > - SLAB_POISON, NULL);
> > + SLAB_POISON|SLAB_NO_GLOBAL_FLAGS, NULL);
> > u8 *p = kmem_cache_alloc(s, GFP_KERNEL);
> >
> > kmem_cache_free(s, p);
> > @@ -106,7 +106,7 @@ static void test_clobber_50th_byte(struct kunit *test)
> > static void test_clobber_redzone_free(struct kunit *test)
> > {
> > struct kmem_cache *s = kmem_cache_create("TestSlub_RZ_free", 64, 0,
> > - SLAB_RED_ZONE, NULL);
> > + SLAB_RED_ZONE|SLAB_NO_GLOBAL_FLAGS, NULL);
> > u8 *p = kmem_cache_alloc(s, GFP_KERNEL);
> >
> > kasan_disable_current();
> > diff --git a/mm/slab.h b/mm/slab.h
> > index c7f2abc2b154..69946131208a 100644
> > --- a/mm/slab.h
> > +++ b/mm/slab.h
> > @@ -330,7 +330,7 @@ static inline slab_flags_t kmem_cache_flags(unsigned int object_size,
> > SLAB_ACCOUNT)
> > #elif defined(CONFIG_SLUB)
> > #define SLAB_CACHE_FLAGS (SLAB_NOLEAKTRACE | SLAB_RECLAIM_ACCOUNT | \
> > - SLAB_TEMPORARY | SLAB_ACCOUNT)
> > + SLAB_TEMPORARY | SLAB_ACCOUNT | SLAB_NO_GLOBAL_FLAGS)
> > #else
> > #define SLAB_CACHE_FLAGS (SLAB_NOLEAKTRACE)
> > #endif
> > @@ -349,7 +349,8 @@ static inline slab_flags_t kmem_cache_flags(unsigned int object_size,
> > SLAB_NOLEAKTRACE | \
> > SLAB_RECLAIM_ACCOUNT | \
> > SLAB_TEMPORARY | \
> > - SLAB_ACCOUNT)
> > + SLAB_ACCOUNT | \
> > + SLAB_NO_GLOBAL_FLAGS)
> >
> > bool __kmem_cache_empty(struct kmem_cache *);
> > int __kmem_cache_shutdown(struct kmem_cache *);
> > diff --git a/mm/slub.c b/mm/slub.c
> > index 71e8663f6037..2a3cffd7b27f 100644
> > --- a/mm/slub.c
> > +++ b/mm/slub.c
> > @@ -1592,6 +1592,9 @@ slab_flags_t kmem_cache_flags(unsigned int object_size,
> > if (flags & SLAB_NOLEAKTRACE)
> > slub_debug_local &= ~SLAB_STORE_USER;
> >
> > + if (flags & SLAB_NO_GLOBAL_FLAGS)
> > + slub_debug_local = 0;
> > +
> > len = strlen(name);
> > next_block = slub_debug_string;
> > /* Go through all blocks of debug options, see if any matches our slab's name */
>
