From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id A1525C433F5 for ; Wed, 18 May 2022 09:38:05 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 422936B0073; Wed, 18 May 2022 05:38:05 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 3D2D98D0001; Wed, 18 May 2022 05:38:05 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 273FA6B0075; Wed, 18 May 2022 05:38:05 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0010.hostedemail.com [216.40.44.10]) by kanga.kvack.org (Postfix) with ESMTP id 173316B0073 for ; Wed, 18 May 2022 05:38:05 -0400 (EDT) Received: from smtpin12.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay02.hostedemail.com (Postfix) with ESMTP id E102F30C6B for ; Wed, 18 May 2022 09:38:04 +0000 (UTC) X-FDA: 79478362488.12.2539136 Received: from mail-pl1-f173.google.com (mail-pl1-f173.google.com [209.85.214.173]) by imf14.hostedemail.com (Postfix) with ESMTP id CB0DE1000D3 for ; Wed, 18 May 2022 09:38:01 +0000 (UTC) Received: by mail-pl1-f173.google.com with SMTP id c2so1268760plh.2 for ; Wed, 18 May 2022 02:38:04 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=s5ocU6sZ59/8dYavSeNm47P+W9gYWl3NcTuJSVhqmts=; b=LapNiWLpFboVMUEpHLPuY8Apyk1JZh6YqIl6W3AQV5DRaBcrsKSzx6KZi50sa0pl02 E02XCW/XMMLEtO6yzKe75b3DbXP9VWRI0/IhiDinUVm9kaQ5oSUSpEnUqpOcoNaY8hGk O4fLzYEcHbsbNkXmtriJuBXRP+/jHB0a+06DoWyrkpfHPq9s9hgHgr3LWApp9V4JFaBe S/bZiWeqGauY7pACsuJr0/Tei20A1ybesaquZSv7KtU0/DWtJI52iCfPqp7G/1dxtCLb XNq8jMGKEwDX7NZOBM1fZ22OIHuOWdLFt8UyxzvwpI8iELumo1qUe8FPFzr/7l3rF0hs 5UuA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=s5ocU6sZ59/8dYavSeNm47P+W9gYWl3NcTuJSVhqmts=; b=JF7vnAu8Xlmwwn63kWTKbrqiMaa0UmGTRUvPw4KNL2IrOvyv8GtgpoBhGPBl5bI7ha 8iUlJpDkJmk52ohcXRoY3vfv2RTwDop9JBm/wYaFXu2cRt9aBBHVYz8w6rpFK3comZnn etXnKySDZbv62A46zkD1n8bbLUlZfaqjFpwPPtsKgi1d+iGj9TKXU0YmLMa4MpwKKSV5 /K7Z3k4Hpe7ybFKWQMbIOw6Z6MhIzecT3c9gEAjfR8cERS7QhyeAtd6EzPGGG0Ozbtjm jJnYySVOiugKC43jvIF+XVOE2eqj4ZMKVJQGGMEK7MVkOf36RCubJsCglfEDBYe8hIAU omxQ== X-Gm-Message-State: AOAM531ZihXypmWb8PYjLO/KtLOihgyFd1IiGYgcaLSqiOfgudJZ2oQo RApIfezSVjTfDAXIqs9rVos= X-Google-Smtp-Source: ABdhPJw6V3vMehx2F89UM8zUOAvrs1ngHnXhuJ6OvAgXGqgAOSiKoAL5nCvilouq02QXy7OJRpMemA== X-Received: by 2002:a17:902:f809:b0:161:44a5:3349 with SMTP id ix9-20020a170902f80900b0016144a53349mr21134746plb.140.1652866683373; Wed, 18 May 2022 02:38:03 -0700 (PDT) Received: from hyeyoo ([114.29.24.243]) by smtp.gmail.com with ESMTPSA id b1-20020a62a101000000b0050dc7628196sm1393215pff.112.2022.05.18.02.37.57 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 18 May 2022 02:38:02 -0700 (PDT) Date: Wed, 18 May 2022 18:37:54 +0900 From: Hyeonggon Yoo <42.hyeyoo@gmail.com> To: Muchun Song Cc: YoMccU66auLAPEHa@casper.infradead.org, Steven Rostedt , Shakeel Butt , Roman Gushchin , Vlastimil Babka , Matthew Wilcox , kernel@openvz.org, linux-kernel@vger.kernel.org, Ingo Molnar , Andrew Morton , linux-mm@kvack.org, Joonsoo Kim , David Rientjes , Pekka Enberg , Christoph Lameter , Michal Hocko Subject: Re: [PATCH v2] tracing: add ACCOUNT flag for allocations from marked slab caches Message-ID: References: <8ef9de6a-7497-07f7-852c-befcc3843771@openvz.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Rspamd-Server: rspam02 X-Rspamd-Queue-Id: CB0DE1000D3 X-Stat-Signature: zc61dwnc36gibrkh1exgzsxhdc3sfz98 X-Rspam-User: Authentication-Results: imf14.hostedemail.com; dkim=pass header.d=gmail.com header.s=20210112 header.b=LapNiWLp; spf=pass (imf14.hostedemail.com: domain of 42.hyeyoo@gmail.com designates 209.85.214.173 as permitted sender) smtp.mailfrom=42.hyeyoo@gmail.com; dmarc=pass (policy=none) header.from=gmail.com X-HE-Tag: 1652866681-918581 X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: 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 > > > --- > > > 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 > > > #include > > > > > > -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