Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Date:   Wed, 18 May 2022 18:37:54 +0900
From:   Hyeonggon Yoo <42.hyeyoo@gmail.com>
To:     Muchun Song <songmuchun@bytedance.com>
Cc:     YoMccU66auLAPEHa@casper.infradead.org,
        Steven Rostedt <rostedt@goodmis.org>,
        Shakeel Butt <shakeelb@google.com>,
        Roman Gushchin <roman.gushchin@linux.dev>,
        Vlastimil Babka <vbabka@suse.cz>,
        Matthew Wilcox <willy@infradead.org>, kernel@openvz.org,
        linux-kernel@vger.kernel.org, Ingo Molnar <mingo@redhat.com>,
        Andrew Morton <akpm@linux-foundation.org>, linux-mm@kvack.org,
        Joonsoo Kim <iamjoonsoo.kim@lge.com>,
        David Rientjes <rientjes@google.com>,
        Pekka Enberg <penberg@kernel.org>,
        Christoph Lameter <cl@linux.com>,
        Michal Hocko <mhocko@suse.com>
Subject: Re: [PATCH v2] tracing: add ACCOUNT flag for allocations from marked
 slab caches
Message-ID: <YoS+cjzcReZK47sn@hyeyoo>
References: <8ef9de6a-7497-07f7-852c-befcc3843771@openvz.org>
 <YoOOI1Gw2mX9c8qZ@hyeyoo>
 <YoOjHYWwN38qWjVI@FVFYT0MHHV2J.usts.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <YoOjHYWwN38qWjVI@FVFYT0MHHV2J.usts.net>
Precedence: bulk

On Tue, May 17, 2022 at 09:29:01PM +0800, Muchun Song wrote:
> On Tue, May 17, 2022 at 08:59:31PM +0900, Hyeonggon Yoo wrote:
> > On Tue, May 17, 2022 at 12:44:14PM +0300, Vasily Averin wrote:
> > > dSlab caches marked with SLAB_ACCOUNT force accounting for every
> > > allocation from this cache even if __GFP_ACCOUNT flag is not passed.
> > > Unfortunately, at the moment this flag is not visible in ftrace output,
> > > and this makes it difficult to analyze the accounted allocations.
> > > 
> > > This patch adds the __GFP_ACCOUNT flag for allocations from slab caches
> > > marked with SLAB_ACCOUNT to the ftrace output
> > > ---
> > > v2:
> > >  1) handle kmem_cache_alloc_node() too, thanks to Shakeel
> > >  2) rework kmem_cache_alloc* tracepoints to use cachep instead
> > >     of current cachep->*size parameters. Now kmalloc[_node] and
> > >     kmem_cache_alloc[_node] tracepoints do not use common template
> > > 
> > > NB: kmem_cache_alloc_node tracepoint in SLOB cannot be switched to cachep,
> > >     therefore it was replaced by kmalloc_node tracepoint.
> > > ---
> > > VvS: is this acceptable? Maybe I should split this patch?
> > > 
> > > Signed-off-by: Vasily Averin <vvs@openvz.org>
> > > ---
> > >  include/trace/events/kmem.h | 82 +++++++++++++++++++++++++++----------
> > >  mm/slab.c                   |  7 +---
> > >  mm/slab_common.c            |  7 ++--
> > >  mm/slob.c                   | 10 ++---
> > >  mm/slub.c                   |  6 +--
> > >  5 files changed, 71 insertions(+), 41 deletions(-)
> > > 
> > > diff --git a/include/trace/events/kmem.h b/include/trace/events/kmem.h
> > > index 71c141804222..3b4f96e4a607 100644
> > > --- a/include/trace/events/kmem.h
> > > +++ b/include/trace/events/kmem.h
> > > @@ -9,7 +9,7 @@
> > >  #include <linux/tracepoint.h>
> > >  #include <trace/events/mmflags.h>
> > >  
> > > -DECLARE_EVENT_CLASS(kmem_alloc,
> > > +TRACE_EVENT(kmalloc,
> > >  
> > >  	TP_PROTO(unsigned long call_site,
> > >  		 const void *ptr,
> > > @@ -43,23 +43,41 @@ DECLARE_EVENT_CLASS(kmem_alloc,
> > >  		show_gfp_flags(__entry->gfp_flags))
> > >  );
> > >  
> > > -DEFINE_EVENT(kmem_alloc, kmalloc,
> > > +TRACE_EVENT(kmem_cache_alloc,
> > >  
> > > -	TP_PROTO(unsigned long call_site, const void *ptr,
> > > -		 size_t bytes_req, size_t bytes_alloc, gfp_t gfp_flags),
> > > +	TP_PROTO(unsigned long call_site,
> > > +		 const void *ptr,
> > > +		 struct kmem_cache *s,
> > > +		 gfp_t gfp_flags),
> > >  
> > > -	TP_ARGS(call_site, ptr, bytes_req, bytes_alloc, gfp_flags)
> > > -);
> > > +	TP_ARGS(call_site, ptr, s, gfp_flags),
> > >  
> > > -DEFINE_EVENT(kmem_alloc, kmem_cache_alloc,
> > > +	TP_STRUCT__entry(
> > > +		__field(	unsigned long,	call_site	)
> > > +		__field(	const void *,	ptr		)
> > > +		__field(	size_t,		bytes_req	)
> > > +		__field(	size_t,		bytes_alloc	)
> > > +		__field(	unsigned long,	gfp_flags	)
> > > +	),
> > >  
> > > -	TP_PROTO(unsigned long call_site, const void *ptr,
> > > -		 size_t bytes_req, size_t bytes_alloc, gfp_t gfp_flags),
> > > +	TP_fast_assign(
> > > +		__entry->call_site	= call_site;
> > > +		__entry->ptr		= ptr;
> > > +		__entry->bytes_req	= s->object_size;
> > > +		__entry->bytes_alloc	= s->size;
> > > +		__entry->gfp_flags	= (__force unsigned long)gfp_flags |
> > > +				(s->flags & SLAB_ACCOUNT ? __GFP_ACCOUNT : 0);
> > > +	),
> > 
> > This is a bit of lie. SLAB_ACCOUNT is not a gfp flag.
> >
> 
> Maybe it is not a problem since the functionalities of SLAB_ACCOUNT and
> __GFP_ACCOUNT are similar.
>
> > IMO the problem here is that we don't know which cache kernel is allocating
> > from. What about just printing name of cache and remove bytes_req,
> > bytes_alloc?
> 
> Is it a problem? 

I thought so because SLAB_ACCOUNT is a characteristic of cache, not allocations
unlike GFP_KERNEL/GFP_ATOMIC.

There is more SLAB_* flags and I think it's better not to print
all of them in tracepoints. What if someone wants to track allocations
that are reclaimable?

> Because we have changed the behavior to users. Should
> we treat the tracepoint as a stable API to users? If so, I think we
> should not change the parameter of this tracepoint.  Maybe I am wrong,
> just some thoughts from me.

Hmm, yeah it may break userspace tools. but even changing name of functions
can break such tools... I too wonder we consider them as stable API.
Is there general agreement for this?

And If we cannot change tracepoint (toward breaking existing tools)
after release, We should think more about adding 'accounted' in tracepoints.

Apart from that - even if we're not going to remove bytes_req/bytes_alloc,
I think distinguishing caches is worth than adding something like 'accounted'.

> Thanks.
> 
> > 
> > And then you can check if the cache uses SLAB_ACCOUNT or not.
> > 

-- 
Thanks,
Hyeonggon