| Message ID | 20200610163135.17364-8-vbabka@suse.cz (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | slub_debug fixes and improvements | expand |
On Wed, Jun 10, 2020 at 06:31:33PM +0200, Vlastimil Babka wrote: > There are few places that call kmem_cache_debug(s) (which tests if any of debug > flags are enabled for a cache) immediatelly followed by a test for a specific > flag. The compiler can probably eliminate the extra check, but we can make the > code nicer by introducing kmem_cache_debug_flags() that works like > kmem_cache_debug() (including the static key check) but tests for specifig > flag(s). The next patches will add more users. > > Signed-off-by: Vlastimil Babka <vbabka@suse.cz> > --- > mm/slub.c | 18 ++++++++++++++---- > 1 file changed, 14 insertions(+), 4 deletions(-) Acked-by: Roman Gushchin <guro@fb.com> One small nit below. > > diff --git a/mm/slub.c b/mm/slub.c > index 24d3e5f832aa..c8e8b4ae2451 100644 > --- a/mm/slub.c > +++ b/mm/slub.c > @@ -122,18 +122,28 @@ DEFINE_STATIC_KEY_FALSE(slub_debug_enabled); > #endif > #endif > > -static inline int kmem_cache_debug(struct kmem_cache *s) > +/* > + * Returns true if any of the specified slub_debug flags is enabled for the > + * cache. Use only for flags parsed by setup_slub_debug() as it also enables > + * the static key. Maybe adding VM_WARN_ON_ONCE(!(flags & SLAB_DEBUG_FLAGS)) to make sure the flags argument is valid? > + */ > +static inline int kmem_cache_debug_flags(struct kmem_cache *s, slab_flags_t flags) > { > #ifdef CONFIG_SLUB_DEBUG > if (static_branch_unlikely(&slub_debug_enabled)) > - return s->flags & SLAB_DEBUG_FLAGS; > + return s->flags & flags; > #endif > return 0; > } > > +static inline int kmem_cache_debug(struct kmem_cache *s) > +{ > + return kmem_cache_debug_flags(s, SLAB_DEBUG_FLAGS); > +} > + > void *fixup_red_left(struct kmem_cache *s, void *p) > { > - if (kmem_cache_debug(s) && s->flags & SLAB_RED_ZONE) > + if (kmem_cache_debug_flags(s, SLAB_RED_ZONE)) > p += s->red_left_pad; > > return p; > @@ -4076,7 +4086,7 @@ void __check_heap_object(const void *ptr, unsigned long n, struct page *page, > offset = (ptr - page_address(page)) % s->size; > > /* Adjust for redzone and reject if within the redzone. */ > - if (kmem_cache_debug(s) && s->flags & SLAB_RED_ZONE) { > + if (kmem_cache_debug_flags(s, SLAB_RED_ZONE)) { > if (offset < s->red_left_pad) > usercopy_abort("SLUB object in left red zone", > s->name, to_user, offset, n); > -- > 2.26.2 >
On Wed, Jun 10, 2020 at 06:31:33PM +0200, Vlastimil Babka wrote: > There are few places that call kmem_cache_debug(s) (which tests if any of debug > flags are enabled for a cache) immediatelly followed by a test for a specific > flag. The compiler can probably eliminate the extra check, but we can make the > code nicer by introducing kmem_cache_debug_flags() that works like > kmem_cache_debug() (including the static key check) but tests for specifig > flag(s). The next patches will add more users. > > Signed-off-by: Vlastimil Babka <vbabka@suse.cz> > --- > mm/slub.c | 18 ++++++++++++++---- > 1 file changed, 14 insertions(+), 4 deletions(-) > > diff --git a/mm/slub.c b/mm/slub.c > index 24d3e5f832aa..c8e8b4ae2451 100644 > --- a/mm/slub.c > +++ b/mm/slub.c > @@ -122,18 +122,28 @@ DEFINE_STATIC_KEY_FALSE(slub_debug_enabled); > #endif > #endif > > -static inline int kmem_cache_debug(struct kmem_cache *s) > +/* > + * Returns true if any of the specified slub_debug flags is enabled for the > + * cache. Use only for flags parsed by setup_slub_debug() as it also enables > + * the static key. > + */ > +static inline int kmem_cache_debug_flags(struct kmem_cache *s, slab_flags_t flags) This should return slab_flag_t, yes? > { > #ifdef CONFIG_SLUB_DEBUG > if (static_branch_unlikely(&slub_debug_enabled)) > - return s->flags & SLAB_DEBUG_FLAGS; > + return s->flags & flags; > #endif > return 0; > } > > +static inline int kmem_cache_debug(struct kmem_cache *s) And this too, as long as we're making changes here.
On 6/17/20 7:56 PM, Kees Cook wrote: > On Wed, Jun 10, 2020 at 06:31:33PM +0200, Vlastimil Babka wrote: >> There are few places that call kmem_cache_debug(s) (which tests if any of debug >> flags are enabled for a cache) immediatelly followed by a test for a specific >> flag. The compiler can probably eliminate the extra check, but we can make the >> code nicer by introducing kmem_cache_debug_flags() that works like >> kmem_cache_debug() (including the static key check) but tests for specifig >> flag(s). The next patches will add more users. >> >> Signed-off-by: Vlastimil Babka <vbabka@suse.cz> >> --- >> mm/slub.c | 18 ++++++++++++++---- >> 1 file changed, 14 insertions(+), 4 deletions(-) >> >> diff --git a/mm/slub.c b/mm/slub.c >> index 24d3e5f832aa..c8e8b4ae2451 100644 >> --- a/mm/slub.c >> +++ b/mm/slub.c >> @@ -122,18 +122,28 @@ DEFINE_STATIC_KEY_FALSE(slub_debug_enabled); >> #endif >> #endif >> >> -static inline int kmem_cache_debug(struct kmem_cache *s) >> +/* >> + * Returns true if any of the specified slub_debug flags is enabled for the >> + * cache. Use only for flags parsed by setup_slub_debug() as it also enables >> + * the static key. >> + */ >> +static inline int kmem_cache_debug_flags(struct kmem_cache *s, slab_flags_t flags) > > This should return slab_flag_t, yes? bool, actually >> { >> #ifdef CONFIG_SLUB_DEBUG >> if (static_branch_unlikely(&slub_debug_enabled)) >> - return s->flags & SLAB_DEBUG_FLAGS; >> + return s->flags & flags; >> #endif >> return 0; >> } >> >> +static inline int kmem_cache_debug(struct kmem_cache *s) > > And this too, as long as we're making changes here. OK
On 6/10/20 6:31 PM, Vlastimil Babka wrote: > There are few places that call kmem_cache_debug(s) (which tests if any of debug > flags are enabled for a cache) immediatelly followed by a test for a specific > flag. The compiler can probably eliminate the extra check, but we can make the > code nicer by introducing kmem_cache_debug_flags() that works like > kmem_cache_debug() (including the static key check) but tests for specifig > flag(s). The next patches will add more users. > > Signed-off-by: Vlastimil Babka <vbabka@suse.cz> Please add this fixup per reviews. ----8<---- From 25793252a31a36f8d1d4ccf0f27ed3e43fba54d8 Mon Sep 17 00:00:00 2001 From: Vlastimil Babka <vbabka@suse.cz> Date: Thu, 18 Jun 2020 10:34:53 +0200 Subject: [PATCH] mm, slub: introduce kmem_cache_debug_flags()-fix Change return from int to bool, per Kees. Add VM_WARN_ON_ONCE() for invalid flags, per Roman. Signed-off-by: Vlastimil Babka <vbabka@suse.cz> --- mm/slub.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/mm/slub.c b/mm/slub.c index c8e8b4ae2451..59d19c64069e 100644 --- a/mm/slub.c +++ b/mm/slub.c @@ -127,16 +127,17 @@ DEFINE_STATIC_KEY_FALSE(slub_debug_enabled); * cache. Use only for flags parsed by setup_slub_debug() as it also enables * the static key. */ -static inline int kmem_cache_debug_flags(struct kmem_cache *s, slab_flags_t flags) +static inline bool kmem_cache_debug_flags(struct kmem_cache *s, slab_flags_t flags) { + VM_WARN_ON_ONCE(!(flags & SLAB_DEBUG_FLAGS)); #ifdef CONFIG_SLUB_DEBUG if (static_branch_unlikely(&slub_debug_enabled)) return s->flags & flags; #endif - return 0; + return false; } -static inline int kmem_cache_debug(struct kmem_cache *s) +static inline bool kmem_cache_debug(struct kmem_cache *s) { return kmem_cache_debug_flags(s, SLAB_DEBUG_FLAGS); }
On Thu, Jun 18, 2020 at 10:37:07AM +0200, Vlastimil Babka wrote: > On 6/10/20 6:31 PM, Vlastimil Babka wrote: > > There are few places that call kmem_cache_debug(s) (which tests if any of debug > > flags are enabled for a cache) immediatelly followed by a test for a specific > > flag. The compiler can probably eliminate the extra check, but we can make the > > code nicer by introducing kmem_cache_debug_flags() that works like > > kmem_cache_debug() (including the static key check) but tests for specifig > > flag(s). The next patches will add more users. > > > > Signed-off-by: Vlastimil Babka <vbabka@suse.cz> > > Please add this fixup per reviews. > ----8<---- > From 25793252a31a36f8d1d4ccf0f27ed3e43fba54d8 Mon Sep 17 00:00:00 2001 > From: Vlastimil Babka <vbabka@suse.cz> > Date: Thu, 18 Jun 2020 10:34:53 +0200 > Subject: [PATCH] mm, slub: introduce kmem_cache_debug_flags()-fix > > Change return from int to bool, per Kees. > Add VM_WARN_ON_ONCE() for invalid flags, per Roman. > > Signed-off-by: Vlastimil Babka <vbabka@suse.cz> Acked-by: Roman Gushchin <guro@fb.com> Thanks! > --- > mm/slub.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/mm/slub.c b/mm/slub.c > index c8e8b4ae2451..59d19c64069e 100644 > --- a/mm/slub.c > +++ b/mm/slub.c > @@ -127,16 +127,17 @@ DEFINE_STATIC_KEY_FALSE(slub_debug_enabled); > * cache. Use only for flags parsed by setup_slub_debug() as it also enables > * the static key. > */ > -static inline int kmem_cache_debug_flags(struct kmem_cache *s, slab_flags_t flags) > +static inline bool kmem_cache_debug_flags(struct kmem_cache *s, slab_flags_t flags) > { > + VM_WARN_ON_ONCE(!(flags & SLAB_DEBUG_FLAGS)); > #ifdef CONFIG_SLUB_DEBUG > if (static_branch_unlikely(&slub_debug_enabled)) > return s->flags & flags; > #endif > - return 0; > + return false; > } > > -static inline int kmem_cache_debug(struct kmem_cache *s) > +static inline bool kmem_cache_debug(struct kmem_cache *s) > { > return kmem_cache_debug_flags(s, SLAB_DEBUG_FLAGS); > } > -- > 2.27.0 > >
On Thu, Jun 18, 2020 at 10:37:07AM +0200, Vlastimil Babka wrote: > On 6/10/20 6:31 PM, Vlastimil Babka wrote: > > There are few places that call kmem_cache_debug(s) (which tests if any of debug > > flags are enabled for a cache) immediatelly followed by a test for a specific > > flag. The compiler can probably eliminate the extra check, but we can make the > > code nicer by introducing kmem_cache_debug_flags() that works like > > kmem_cache_debug() (including the static key check) but tests for specifig > > flag(s). The next patches will add more users. > > > > Signed-off-by: Vlastimil Babka <vbabka@suse.cz> > > Please add this fixup per reviews. > ----8<---- > From 25793252a31a36f8d1d4ccf0f27ed3e43fba54d8 Mon Sep 17 00:00:00 2001 > From: Vlastimil Babka <vbabka@suse.cz> > Date: Thu, 18 Jun 2020 10:34:53 +0200 > Subject: [PATCH] mm, slub: introduce kmem_cache_debug_flags()-fix > > Change return from int to bool, per Kees. > Add VM_WARN_ON_ONCE() for invalid flags, per Roman. > > Signed-off-by: Vlastimil Babka <vbabka@suse.cz> Acked-by: Kees Cook <keescook@chromium.org>
diff --git a/mm/slub.c b/mm/slub.c index 24d3e5f832aa..c8e8b4ae2451 100644 --- a/mm/slub.c +++ b/mm/slub.c @@ -122,18 +122,28 @@ DEFINE_STATIC_KEY_FALSE(slub_debug_enabled); #endif #endif -static inline int kmem_cache_debug(struct kmem_cache *s) +/* + * Returns true if any of the specified slub_debug flags is enabled for the + * cache. Use only for flags parsed by setup_slub_debug() as it also enables + * the static key. + */ +static inline int kmem_cache_debug_flags(struct kmem_cache *s, slab_flags_t flags) { #ifdef CONFIG_SLUB_DEBUG if (static_branch_unlikely(&slub_debug_enabled)) - return s->flags & SLAB_DEBUG_FLAGS; + return s->flags & flags; #endif return 0; } +static inline int kmem_cache_debug(struct kmem_cache *s) +{ + return kmem_cache_debug_flags(s, SLAB_DEBUG_FLAGS); +} + void *fixup_red_left(struct kmem_cache *s, void *p) { - if (kmem_cache_debug(s) && s->flags & SLAB_RED_ZONE) + if (kmem_cache_debug_flags(s, SLAB_RED_ZONE)) p += s->red_left_pad; return p; @@ -4076,7 +4086,7 @@ void __check_heap_object(const void *ptr, unsigned long n, struct page *page, offset = (ptr - page_address(page)) % s->size; /* Adjust for redzone and reject if within the redzone. */ - if (kmem_cache_debug(s) && s->flags & SLAB_RED_ZONE) { + if (kmem_cache_debug_flags(s, SLAB_RED_ZONE)) { if (offset < s->red_left_pad) usercopy_abort("SLUB object in left red zone", s->name, to_user, offset, n);
There are few places that call kmem_cache_debug(s) (which tests if any of debug flags are enabled for a cache) immediatelly followed by a test for a specific flag. The compiler can probably eliminate the extra check, but we can make the code nicer by introducing kmem_cache_debug_flags() that works like kmem_cache_debug() (including the static key check) but tests for specifig flag(s). The next patches will add more users. Signed-off-by: Vlastimil Babka <vbabka@suse.cz> --- mm/slub.c | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-)