Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Sat, 30 Apr 2022 14:06:55 +0000
From:   Hyeonggon Yoo <42.hyeyoo@gmail.com>
To:     Vlastimil Babka <vbabka@suse.cz>
Cc:     Marco Elver <elver@google.com>,
        Matthew WilCox <willy@infradead.org>,
        Christoph Lameter <cl@linux.com>,
        Pekka Enberg <penberg@kernel.org>,
        David Rientjes <rientjes@google.com>,
        Joonsoo Kim <iamjoonsoo.kim@lge.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Roman Gushchin <roman.gushchin@linux.dev>, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 14/23] mm/slab_common: print cache name in tracepoints
Message-ID: <20220430140654.GB25819@ip-172-31-27-201.ap-northeast-1.compute.internal>
References: <20220414085727.643099-1-42.hyeyoo@gmail.com>
 <20220414085727.643099-15-42.hyeyoo@gmail.com>
 <bbb97e3c-e597-dd6e-e213-55bc1779d901@suse.cz>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <bbb97e3c-e597-dd6e-e213-55bc1779d901@suse.cz>
User-Agent: Mutt/1.5.21 (2010-09-15)
Precedence: bulk

On Fri, Apr 29, 2022 at 04:05:14PM +0200, Vlastimil Babka wrote:
> On 4/14/22 10:57, Hyeonggon Yoo wrote:
> > Print cache name in tracepoints. If there is no corresponding cache
> > (kmalloc in SLOB or kmalloc_large_node), use KMALLOC_{,LARGE_}NAME
> > macro.
> > 
> > Signed-off-by: Hyeonggon Yoo <42.hyeyoo@gmail.com>
> 
> Regarding tracepoints, I'm not sure it's a good idea to unify kmalloc and
> kmem_cache_alloc. I think the common use case is to trace kmalloc as there
> are many different callers, and then I'm not interested in kmem_cache_alloc
> callers much.

Make sense.

> What I would suggest instead is:
> - drop the _node versions, add node to normal versions
> - drop the kmem_alloc EVENT_CLASS, as kmalloc is different enough from
> kmem_cache_alloc (see next points), define separately as TRACE_EVENT().
> - printing cache_name makes sense to add for kmem_cache_alloc (also allows
> filtering events) but not for kmalloc.
> - kmem_cache_alloc with name can then drop the bytes_req, bytes_alloc as
> they are fixed for given name (and can be read from slabinfo).

Those suggestions makes sense to me.
kmalloc and kmem_cache_alloc are a bit different.

> Not using a common tracepoint will prevent some later unifications/cleanup
> (patch 21?), but hopefully not too much?

In v2 unification of __kmalloc_node()/kfree() somewhat depends on unified tracepoint.
I'll try to unify them with this approach in v3.