[PATCH] mm: provide interface for retrieving kmem_cache name

[PATCH] mm: provide interface for retrieving kmem_cache name

[Knut Omang]

With the restructuring done in commit 9adeaa226988
("mm, slab: move memcg_cache_params structure to mm/slab.h")

it is no longer possible for code external to mm to access
the name of a kmem_cache as struct kmem_cache has effectively become
opaque. Having access to the cache name is helpful to kernel testing
infrastructure.

Expose a new function kmem_cache_name to mitigate that.

Signed-off-by: Knut Omang <knut.omang@oracle.com>
---
 include/linux/slab.h | 1 +
 mm/slab_common.c     | 9 +++++++++
 2 files changed, 10 insertions(+)

diff --git a/include/linux/slab.h b/include/linux/slab.h
index 4d2a2fa55ed5..3298c9db6e46 100644
--- a/include/linux/slab.h
+++ b/include/linux/slab.h
@@ -702,6 +702,7 @@ static inline void *kzalloc_node(size_t size, gfp_t flags, int node)
 }
 
 unsigned int kmem_cache_size(struct kmem_cache *s);
+const char *kmem_cache_name(struct kmem_cache *s);
 void __init kmem_cache_init_late(void);
 
 #if defined(CONFIG_SMP) && defined(CONFIG_SLAB)
diff --git a/mm/slab_common.c b/mm/slab_common.c
index f9fb27b4c843..269a99dc8214 100644
--- a/mm/slab_common.c
+++ b/mm/slab_common.c
@@ -82,6 +82,15 @@ unsigned int kmem_cache_size(struct kmem_cache *s)
 }
 EXPORT_SYMBOL(kmem_cache_size);
 
+/*
+ * Get the name of a slab object
+ */
+const char *kmem_cache_name(struct kmem_cache *s)
+{
+	return s->name;
+}
+EXPORT_SYMBOL(kmem_cache_name);
+
 #ifdef CONFIG_DEBUG_VM
 static int kmem_cache_sanity_check(const char *name, unsigned int size)
 {
-- 
2.20.1

Re: [PATCH] mm: provide interface for retrieving kmem_cache name

[Michal Hocko]

On Thu 07-11-19 12:54:04, Knut Omang wrote:
> With the restructuring done in commit 9adeaa226988
> ("mm, slab: move memcg_cache_params structure to mm/slab.h")
> 
> it is no longer possible for code external to mm to access
> the name of a kmem_cache as struct kmem_cache has effectively become
> opaque. Having access to the cache name is helpful to kernel testing
> infrastructure.
> 
> Expose a new function kmem_cache_name to mitigate that.

Who is going to use that symbol? It is preferred that a user is added in
the same patch as the newly added symbol.

> Signed-off-by: Knut Omang <knut.omang@oracle.com>
> ---
>  include/linux/slab.h | 1 +
>  mm/slab_common.c     | 9 +++++++++
>  2 files changed, 10 insertions(+)
> 
> diff --git a/include/linux/slab.h b/include/linux/slab.h
> index 4d2a2fa55ed5..3298c9db6e46 100644
> --- a/include/linux/slab.h
> +++ b/include/linux/slab.h
> @@ -702,6 +702,7 @@ static inline void *kzalloc_node(size_t size, gfp_t flags, int node)
>  }
>  
>  unsigned int kmem_cache_size(struct kmem_cache *s);
> +const char *kmem_cache_name(struct kmem_cache *s);
>  void __init kmem_cache_init_late(void);
>  
>  #if defined(CONFIG_SMP) && defined(CONFIG_SLAB)
> diff --git a/mm/slab_common.c b/mm/slab_common.c
> index f9fb27b4c843..269a99dc8214 100644
> --- a/mm/slab_common.c
> +++ b/mm/slab_common.c
> @@ -82,6 +82,15 @@ unsigned int kmem_cache_size(struct kmem_cache *s)
>  }
>  EXPORT_SYMBOL(kmem_cache_size);
>  
> +/*
> + * Get the name of a slab object
> + */
> +const char *kmem_cache_name(struct kmem_cache *s)
> +{
> +	return s->name;
> +}
> +EXPORT_SYMBOL(kmem_cache_name);
> +
>  #ifdef CONFIG_DEBUG_VM
>  static int kmem_cache_sanity_check(const char *name, unsigned int size)
>  {
> -- 
> 2.20.1

-- 
Michal Hocko
SUSE Labs

Re: [PATCH] mm: provide interface for retrieving kmem_cache name

[Knut Omang]

On Thu, 2019-11-07 at 12:58 +0100, Michal Hocko wrote:
> On Thu 07-11-19 12:54:04, Knut Omang wrote:
> > With the restructuring done in commit 9adeaa226988
> > ("mm, slab: move memcg_cache_params structure to mm/slab.h")
> > 
> > it is no longer possible for code external to mm to access
> > the name of a kmem_cache as struct kmem_cache has effectively become
> > opaque. Having access to the cache name is helpful to kernel testing
> > infrastructure.
> > 
> > Expose a new function kmem_cache_name to mitigate that.
> 
> Who is going to use that symbol? It is preferred that a user is added in
> the same patch as the newly added symbol.

Yes, I am aware that that's the normal practice, 
we're currently using cache->name directly in the kernel 
unit test framework KTF (https://github.com/oracle/ktf/)
which we are working (https://lkml.org/lkml/2019/8/13/111) to get 
into the kernel in one form or another.

To me this seems like a natural part of an API for the kmem_cache
data structure now that it has in effect become opaque, so it seemed 
appropriate to get it in close in time to the patch that no longer 
makes this possible, instead of someone else hitting this down the road.

Thanks,
Knut

> > Signed-off-by: Knut Omang <knut.omang@oracle.com>
> > ---
> >  include/linux/slab.h | 1 +
> >  mm/slab_common.c     | 9 +++++++++
> >  2 files changed, 10 insertions(+)
> > 
> > diff --git a/include/linux/slab.h b/include/linux/slab.h
> > index 4d2a2fa55ed5..3298c9db6e46 100644
> > --- a/include/linux/slab.h
> > +++ b/include/linux/slab.h
> > @@ -702,6 +702,7 @@ static inline void *kzalloc_node(size_t size, gfp_t flags, int node)
> >  }
> >  
> >  unsigned int kmem_cache_size(struct kmem_cache *s);
> > +const char *kmem_cache_name(struct kmem_cache *s);
> >  void __init kmem_cache_init_late(void);
> >  
> >  #if defined(CONFIG_SMP) && defined(CONFIG_SLAB)
> > diff --git a/mm/slab_common.c b/mm/slab_common.c
> > index f9fb27b4c843..269a99dc8214 100644
> > --- a/mm/slab_common.c
> > +++ b/mm/slab_common.c
> > @@ -82,6 +82,15 @@ unsigned int kmem_cache_size(struct kmem_cache *s)
> >  }
> >  EXPORT_SYMBOL(kmem_cache_size);
> >  
> > +/*
> > + * Get the name of a slab object
> > + */
> > +const char *kmem_cache_name(struct kmem_cache *s)
> > +{
> > +	return s->name;
> > +}
> > +EXPORT_SYMBOL(kmem_cache_name);
> > +
> >  #ifdef CONFIG_DEBUG_VM
> >  static int kmem_cache_sanity_check(const char *name, unsigned int size)
> >  {
> > -- 
> > 2.20.1

Re: [PATCH] mm: provide interface for retrieving kmem_cache name

[Michal Hocko]

On Thu 07-11-19 13:26:09, Knut Omang wrote:
> On Thu, 2019-11-07 at 12:58 +0100, Michal Hocko wrote:
> > On Thu 07-11-19 12:54:04, Knut Omang wrote:
> > > With the restructuring done in commit 9adeaa226988
> > > ("mm, slab: move memcg_cache_params structure to mm/slab.h")
> > > 
> > > it is no longer possible for code external to mm to access
> > > the name of a kmem_cache as struct kmem_cache has effectively become
> > > opaque. Having access to the cache name is helpful to kernel testing
> > > infrastructure.
> > > 
> > > Expose a new function kmem_cache_name to mitigate that.
> > 
> > Who is going to use that symbol? It is preferred that a user is added in
> > the same patch as the newly added symbol.
> 
> Yes, I am aware that that's the normal practice, 
> we're currently using cache->name directly in the kernel 
> unit test framework KTF (https://github.com/oracle/ktf/)
> which we are working (https://lkml.org/lkml/2019/8/13/111) to get 
> into the kernel in one form or another.

Please add the export with a patch that really needs it.

> To me this seems like a natural part of an API for the kmem_cache
> data structure now that it has in effect become opaque, so it seemed 
> appropriate to get it in close in time to the patch that no longer 
> makes this possible, instead of someone else hitting this down the road.

Well, this is something for SLAB maintainers but I do not really think
the name is something the in kernel code should care about. It is solely
for presenting reasonable statistics to the userspace and that code
workds just fine AFAIK.
-- 
Michal Hocko
SUSE Labs

Re: [PATCH] mm: provide interface for retrieving kmem_cache name

[David Rientjes]

On Thu, 7 Nov 2019, Michal Hocko wrote:

> On Thu 07-11-19 13:26:09, Knut Omang wrote:
> > On Thu, 2019-11-07 at 12:58 +0100, Michal Hocko wrote:
> > > On Thu 07-11-19 12:54:04, Knut Omang wrote:
> > > > With the restructuring done in commit 9adeaa226988
> > > > ("mm, slab: move memcg_cache_params structure to mm/slab.h")
> > > > 
> > > > it is no longer possible for code external to mm to access
> > > > the name of a kmem_cache as struct kmem_cache has effectively become
> > > > opaque. Having access to the cache name is helpful to kernel testing
> > > > infrastructure.
> > > > 
> > > > Expose a new function kmem_cache_name to mitigate that.
> > > 
> > > Who is going to use that symbol? It is preferred that a user is added in
> > > the same patch as the newly added symbol.
> > 
> > Yes, I am aware that that's the normal practice, 
> > we're currently using cache->name directly in the kernel 
> > unit test framework KTF (https://github.com/oracle/ktf/)
> > which we are working (https://lkml.org/lkml/2019/8/13/111) to get 
> > into the kernel in one form or another.
> 
> Please add the export with a patch that really needs it.
> 

Agree, this patch could be added as a predecessor to the series at
https://lkml.org/lkml/2019/8/13/111 which would use it.  If it's obvious 
why using the kmem cache name is useful in that series, acking this will 
be a no brainer.

Re: [PATCH] mm: provide interface for retrieving kmem_cache name

[Christopher Lameter]

On Thu, 7 Nov 2019, Michal Hocko wrote:

> On Thu 07-11-19 13:26:09, Knut Omang wrote:
> > On Thu, 2019-11-07 at 12:58 +0100, Michal Hocko wrote:
> > > On Thu 07-11-19 12:54:04, Knut Omang wrote:
> > > > With the restructuring done in commit 9adeaa226988
> > > > ("mm, slab: move memcg_cache_params structure to mm/slab.h")
> > > >
> > > > it is no longer possible for code external to mm to access

That patch only affected the memcg_cache_params structure and not
kmem_cache.

And I do not see any references to the memcg_cache_param?

The fields that all allocators need to expose are listed in
the struct kmme_cache definition in linux/mm/slab.h.

Re: [PATCH] mm: provide interface for retrieving kmem_cache name

[Knut Omang]

On Fri, 2019-11-08 at 15:37 +0000, Christopher Lameter wrote:
> On Thu, 7 Nov 2019, Michal Hocko wrote:
> 
> > On Thu 07-11-19 13:26:09, Knut Omang wrote:
> > > On Thu, 2019-11-07 at 12:58 +0100, Michal Hocko wrote:
> > > > On Thu 07-11-19 12:54:04, Knut Omang wrote:
> > > > > With the restructuring done in commit 9adeaa226988
> > > > > ("mm, slab: move memcg_cache_params structure to mm/slab.h")
> > > > >
> > > > > it is no longer possible for code external to mm to access
> 
> That patch only affected the memcg_cache_params structure and not
> kmem_cache.
> 
> And I do not see any references to the memcg_cache_param?

Good point, I should have made explicit reference to it. 

It gets inlined into kmem_cache with CONFIG_SLUB if CONFIG_MEMCG is set
(include/linux/slub_def.h, line 112)

> The fields that all allocators need to expose are listed in
> the struct kmme_cache definition in linux/mm/slab.h.

So I take that kmem_cache::name was still intended to be public, 
just that that broke due to the inlining of struct memcg_cache_param 
in slub_def.h?

Knut

Re: [PATCH] mm: provide interface for retrieving kmem_cache name

[Christopher Lameter]

On Fri, 8 Nov 2019, Knut Omang wrote:

> On Fri, 2019-11-08 at 15:37 +0000, Christopher Lameter wrote:
> > On Thu, 7 Nov 2019, Michal Hocko wrote:
> >
> > > On Thu 07-11-19 13:26:09, Knut Omang wrote:
> > > > On Thu, 2019-11-07 at 12:58 +0100, Michal Hocko wrote:
> > > > > On Thu 07-11-19 12:54:04, Knut Omang wrote:
> > > > > > With the restructuring done in commit 9adeaa226988
> > > > > > ("mm, slab: move memcg_cache_params structure to mm/slab.h")
> > > > > >
> > > > > > it is no longer possible for code external to mm to access
> >
> > That patch only affected the memcg_cache_params structure and not
> > kmem_cache.
> >
> > And I do not see any references to the memcg_cache_param?
>
> Good point, I should have made explicit reference to it.
>
> It gets inlined into kmem_cache with CONFIG_SLUB if CONFIG_MEMCG is set
> (include/linux/slub_def.h, line 112)

Yes but that does not affect the "name" field on line 105

> > The fields that all allocators need to expose are listed in
> > the struct kmme_cache definition in linux/mm/slab.h.
>
> So I take that kmem_cache::name was still intended to be public,
> just that that broke due to the inlining of struct memcg_cache_param
> in slub_def.h?

The patch did not change the name field. I am not sure what is broken?

Maybe you need memcg_params to compose a name of the memcg with the slab
name?

Re: [PATCH] mm: provide interface for retrieving kmem_cache name

[Knut Omang]

On Fri, 2019-11-08 at 16:49 +0000, Christopher Lameter wrote:
> On Fri, 8 Nov 2019, Knut Omang wrote:
> 
> > On Fri, 2019-11-08 at 15:37 +0000, Christopher Lameter wrote:
> > > On Thu, 7 Nov 2019, Michal Hocko wrote:
> > >
> > > > On Thu 07-11-19 13:26:09, Knut Omang wrote:
> > > > > On Thu, 2019-11-07 at 12:58 +0100, Michal Hocko wrote:
> > > > > > On Thu 07-11-19 12:54:04, Knut Omang wrote:
> > > > > > > With the restructuring done in commit 9adeaa226988
> > > > > > > ("mm, slab: move memcg_cache_params structure to mm/slab.h")
> > > > > > >
> > > > > > > it is no longer possible for code external to mm to access
> > >
> > > That patch only affected the memcg_cache_params structure and not
> > > kmem_cache.
> > >
> > > And I do not see any references to the memcg_cache_param?
> >
> > Good point, I should have made explicit reference to it.
> >
> > It gets inlined into kmem_cache with CONFIG_SLUB if CONFIG_MEMCG is set
> > (include/linux/slub_def.h, line 112)
> 
> Yes but that does not affect the "name" field on line 105
> 
> > > The fields that all allocators need to expose are listed in
> > > the struct kmme_cache definition in linux/mm/slab.h.
> >
> > So I take that kmem_cache::name was still intended to be public,
> > just that that broke due to the inlining of struct memcg_cache_param
> > in slub_def.h?
> 
> The patch did not change the name field. I am not sure what is broken?
> 
> Maybe you need memcg_params to compose a name of the memcg with the slab
> name?

No, it's just that the slub definition of struct kmem_cache won't compile because
the definition of struct memcg_cache_params is no longer available.

Knut