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 D0820C54E5D for ; Tue, 12 Mar 2024 18:56:42 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 5446E8E000D; Tue, 12 Mar 2024 14:56:42 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 4F5188E0007; Tue, 12 Mar 2024 14:56:42 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 3BC8F8E000D; Tue, 12 Mar 2024 14:56:42 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0013.hostedemail.com [216.40.44.13]) by kanga.kvack.org (Postfix) with ESMTP id 2946F8E0007 for ; Tue, 12 Mar 2024 14:56:42 -0400 (EDT) Received: from smtpin14.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay07.hostedemail.com (Postfix) with ESMTP id CB3B8160C18 for ; Tue, 12 Mar 2024 18:56:41 +0000 (UTC) X-FDA: 81889293402.14.8C2B76B Received: from out-183.mta0.migadu.com (out-183.mta0.migadu.com [91.218.175.183]) by imf02.hostedemail.com (Postfix) with ESMTP id 281D38000D for ; Tue, 12 Mar 2024 18:56:39 +0000 (UTC) Authentication-Results: imf02.hostedemail.com; dkim=pass header.d=linux.dev header.s=key1 header.b=iCghAoUn; dmarc=pass (policy=none) header.from=linux.dev; spf=pass (imf02.hostedemail.com: domain of roman.gushchin@linux.dev designates 91.218.175.183 as permitted sender) smtp.mailfrom=roman.gushchin@linux.dev ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1710269800; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=/vbUFRT4v6z0E2DcirJO1q59nBkBCTNwTPtMtNe8Yiw=; b=5TWg0QphEe3UBNio9CqOSHtrtsjfUeCxXL0q9DOq5/7LlJGpX82XyN5YsYrYJ30npxpJUe EcMf8QrgS+3tCxAv0ui3wfLyFriFdXjygADgNhRCPkkRP4PzRyqHN03NsJr2kg9fGoubO2 D7UfkdHg77CT4Dz3AalTiZSzapvFiuo= ARC-Authentication-Results: i=1; imf02.hostedemail.com; dkim=pass header.d=linux.dev header.s=key1 header.b=iCghAoUn; dmarc=pass (policy=none) header.from=linux.dev; spf=pass (imf02.hostedemail.com: domain of roman.gushchin@linux.dev designates 91.218.175.183 as permitted sender) smtp.mailfrom=roman.gushchin@linux.dev ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1710269800; a=rsa-sha256; cv=none; b=bcxt0nj4eztTEeuN3OzOLDUZNLfDoM46Mloc+tGjkgdZjt/5ouNQ4PK3PuLMtuXCiQZNbk /nqPe5oZexnim2nZumN8aP2eREikia+xrgtYYml8SBMmXIwJuzFFkt9pH2j5nvvnG8Nz8G /zf2JlY6PQRt+xyX6XTqpmRSmvHlB5g= Date: Tue, 12 Mar 2024 11:56:31 -0700 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.dev; s=key1; t=1710269798; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=/vbUFRT4v6z0E2DcirJO1q59nBkBCTNwTPtMtNe8Yiw=; b=iCghAoUnCwH8kI0/NrUiK4kWEC5H+CYF9MrTs2MphCasYW5zPOZqV/0AwbJkBhJdTMrJbp uYZKm2lEO4GyrVq8YfopiXFWp3Rz/FVyiJ0068cXPWVWHj8fuH5hlkz9rQiELZHRNVvcic XyDJE5V0OuxJapfzQzXJTkB/ae49lTI= X-Report-Abuse: Please report any abuse attempt to abuse@migadu.com and include these headers. From: Roman Gushchin To: Vlastimil Babka Cc: Linus Torvalds , Josh Poimboeuf , Jeff Layton , Chuck Lever , Kees Cook , Christoph Lameter , Pekka Enberg , David Rientjes , Joonsoo Kim , Andrew Morton , Hyeonggon Yoo <42.hyeyoo@gmail.com>, Johannes Weiner , Michal Hocko , Shakeel Butt , Muchun Song , Alexander Viro , Christian Brauner , Jan Kara , linux-mm@kvack.org, linux-kernel@vger.kernel.org, cgroups@vger.kernel.org, linux-fsdevel@vger.kernel.org Subject: Re: [PATCH RFC 2/4] mm, slab: move slab_memcg hooks to mm/memcontrol.c Message-ID: References: <20240301-slab-memcg-v1-0-359328a46596@suse.cz> <20240301-slab-memcg-v1-2-359328a46596@suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20240301-slab-memcg-v1-2-359328a46596@suse.cz> X-Migadu-Flow: FLOW_OUT X-Rspamd-Queue-Id: 281D38000D X-Rspam-User: X-Rspamd-Server: rspam04 X-Stat-Signature: hdkmw3idkabp3ik1mw58q3sh1xqwgikw X-HE-Tag: 1710269799-8608 X-HE-Meta: U2FsdGVkX18o2rl5coz9girWSqhQEkKf96tNLR4IOl+VxbpJkno5Kt3a5T04Czh5pln9heTvcTxtUZmHBLRBL8EForkfWWDGqDu/e00RgqsWgGjrhf1lCJre0Y/26GaGb43e4VqpyPukeb8HZj7bw93FT3uKhLJjT9vGxom9RvLodMV4V4WtZPf/voF4NLlM9EP3QwfafQCN54ypcUhwhg6OqUTU5xVriCr64kzvaYzkf2eaMBgooamxLc8dnB0rSYGF4z1EddZ1jrQXU2LLaD3Ckjoqnnty8cNCqLf4m+bd3Yoq+s4MrzDiXpOphhtDKZ1dUK0sAEJFj+MECoCzEN7NCVR1vBlK9WLaMbtB3gMGu8wHAbT2jM7zIUkJLQqm/k6UsGFn7l8ZND2j7aFmOjfcAL2YWoZPeeDAcBnihRrNUpccSSm3Dfi4k2Htj0k/xGFvXm7Ap8lf55p2oXDGkbr5/6EIpklvUgkNsSkYiRq0qJrexWk0cAp/Q2k/1xXpe3QDzDpQdxRMVh+tCWBaD0/Ds/KQ/QuLPaW86KRSCIMjx1YnjvNTThqVIkXKcb1rOx/+LKoSMb+DNrcQxhE0Ekj7O3fQqxqAIABOrq52p5KoRH6Y1l5ADTiCq7lTUPIHVw9fxgf1MlhN0y4x/hljj2O69kzO82jfLWlEMMAaJ/dFD8r/xPfrwRr4zUynEoXY0P+otmiOzqjbWgRPoQm982SgLE+nZDxHu08y7b6tgCVt4nWBGIzfx4bQh1aUTwKneIgybdUwSgPu9NqHSwbnV+sgYjb4DsTAY0zgyu46G9gEenvNVM7Fk3ImWGckDsCcZP+2wzkD+OBhWwrYsHZkp0dU+okHfTxSRrZyuBWaBhkNuJfFICVgyI+1bwA6/nkidRXBuKJKUaQvkqkBUfalZNWil2yHkVZjR9SWQLt3k88= 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: List-Subscribe: List-Unsubscribe: On Fri, Mar 01, 2024 at 06:07:09PM +0100, Vlastimil Babka wrote: > The hooks make multiple calls to functions in mm/memcontrol.c, including > to th current_obj_cgroup() marked __always_inline. It might be faster to > make a single call to the hook in mm/memcontrol.c instead. The hooks > also don't use almost anything from mm/slub.c. obj_full_size() can move > with the hooks and cache_vmstat_idx() to the internal mm/slab.h > > Signed-off-by: Vlastimil Babka > --- > mm/memcontrol.c | 90 ++++++++++++++++++++++++++++++++++++++++++++++++++ > mm/slab.h | 10 ++++++ > mm/slub.c | 100 -------------------------------------------------------- > 3 files changed, 100 insertions(+), 100 deletions(-) Reviewed-by: Roman Gushchin Btw, even before your change: $ cat mm/memcontrol.c | wc -l 8318 so I wonder if soon we might want to split it into some smaller parts. Thanks! > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > index e4c8735e7c85..37ee9356a26c 100644 > --- a/mm/memcontrol.c > +++ b/mm/memcontrol.c > @@ -3575,6 +3575,96 @@ void obj_cgroup_uncharge(struct obj_cgroup *objcg, size_t size) > refill_obj_stock(objcg, size, true); > } > > +static inline size_t obj_full_size(struct kmem_cache *s) > +{ > + /* > + * For each accounted object there is an extra space which is used > + * to store obj_cgroup membership. Charge it too. > + */ > + return s->size + sizeof(struct obj_cgroup *); > +} > + > +bool __memcg_slab_post_alloc_hook(struct kmem_cache *s, struct list_lru *lru, > + gfp_t flags, size_t size, void **p) > +{ > + struct obj_cgroup *objcg; > + struct slab *slab; > + unsigned long off; > + size_t i; > + > + /* > + * The obtained objcg pointer is safe to use within the current scope, > + * defined by current task or set_active_memcg() pair. > + * obj_cgroup_get() is used to get a permanent reference. > + */ > + objcg = current_obj_cgroup(); > + if (!objcg) > + return true; > + > + /* > + * slab_alloc_node() avoids the NULL check, so we might be called with a > + * single NULL object. kmem_cache_alloc_bulk() aborts if it can't fill > + * the whole requested size. > + * return success as there's nothing to free back > + */ > + if (unlikely(*p == NULL)) > + return true; > + > + flags &= gfp_allowed_mask; > + > + if (lru) { > + int ret; > + struct mem_cgroup *memcg; > + > + memcg = get_mem_cgroup_from_objcg(objcg); > + ret = memcg_list_lru_alloc(memcg, lru, flags); > + css_put(&memcg->css); > + > + if (ret) > + return false; > + } > + > + if (obj_cgroup_charge(objcg, flags, size * obj_full_size(s))) > + return false; > + > + for (i = 0; i < size; i++) { > + slab = virt_to_slab(p[i]); > + > + if (!slab_objcgs(slab) && > + memcg_alloc_slab_cgroups(slab, s, flags, false)) { > + obj_cgroup_uncharge(objcg, obj_full_size(s)); > + continue; > + } > + > + off = obj_to_index(s, slab, p[i]); > + obj_cgroup_get(objcg); > + slab_objcgs(slab)[off] = objcg; > + mod_objcg_state(objcg, slab_pgdat(slab), > + cache_vmstat_idx(s), obj_full_size(s)); > + } > + > + return true; > +} > + > +void __memcg_slab_free_hook(struct kmem_cache *s, struct slab *slab, > + void **p, int objects, struct obj_cgroup **objcgs) > +{ > + for (int i = 0; i < objects; i++) { > + struct obj_cgroup *objcg; > + unsigned int off; > + > + off = obj_to_index(s, slab, p[i]); > + objcg = objcgs[off]; > + if (!objcg) > + continue; > + > + objcgs[off] = NULL; > + obj_cgroup_uncharge(objcg, obj_full_size(s)); > + mod_objcg_state(objcg, slab_pgdat(slab), cache_vmstat_idx(s), > + -obj_full_size(s)); > + obj_cgroup_put(objcg); > + } > +} > #endif /* CONFIG_MEMCG_KMEM */ > > /* > diff --git a/mm/slab.h b/mm/slab.h > index 54deeb0428c6..3f170673fa55 100644 > --- a/mm/slab.h > +++ b/mm/slab.h > @@ -541,6 +541,12 @@ static inline bool kmem_cache_debug_flags(struct kmem_cache *s, slab_flags_t fla > return false; > } > > +static inline enum node_stat_item cache_vmstat_idx(struct kmem_cache *s) > +{ > + return (s->flags & SLAB_RECLAIM_ACCOUNT) ? > + NR_SLAB_RECLAIMABLE_B : NR_SLAB_UNRECLAIMABLE_B; > +} > + > #ifdef CONFIG_MEMCG_KMEM > /* > * slab_objcgs - get the object cgroups vector associated with a slab > @@ -564,6 +570,10 @@ int memcg_alloc_slab_cgroups(struct slab *slab, struct kmem_cache *s, > gfp_t gfp, bool new_slab); > void mod_objcg_state(struct obj_cgroup *objcg, struct pglist_data *pgdat, > enum node_stat_item idx, int nr); > +bool __memcg_slab_post_alloc_hook(struct kmem_cache *s, struct list_lru *lru, > + gfp_t flags, size_t size, void **p); > +void __memcg_slab_free_hook(struct kmem_cache *s, struct slab *slab, > + void **p, int objects, struct obj_cgroup **objcgs); > #else /* CONFIG_MEMCG_KMEM */ > static inline struct obj_cgroup **slab_objcgs(struct slab *slab) > { > diff --git a/mm/slub.c b/mm/slub.c > index 7022a1246bab..64da169d672a 100644 > --- a/mm/slub.c > +++ b/mm/slub.c > @@ -1875,12 +1875,6 @@ static bool freelist_corrupted(struct kmem_cache *s, struct slab *slab, > #endif > #endif /* CONFIG_SLUB_DEBUG */ > > -static inline enum node_stat_item cache_vmstat_idx(struct kmem_cache *s) > -{ > - return (s->flags & SLAB_RECLAIM_ACCOUNT) ? > - NR_SLAB_RECLAIMABLE_B : NR_SLAB_UNRECLAIMABLE_B; > -} > - > #ifdef CONFIG_MEMCG_KMEM > static inline void memcg_free_slab_cgroups(struct slab *slab) > { > @@ -1888,79 +1882,6 @@ static inline void memcg_free_slab_cgroups(struct slab *slab) > slab->memcg_data = 0; > } > > -static inline size_t obj_full_size(struct kmem_cache *s) > -{ > - /* > - * For each accounted object there is an extra space which is used > - * to store obj_cgroup membership. Charge it too. > - */ > - return s->size + sizeof(struct obj_cgroup *); > -} > - > -static bool __memcg_slab_post_alloc_hook(struct kmem_cache *s, > - struct list_lru *lru, > - gfp_t flags, size_t size, > - void **p) > -{ > - struct obj_cgroup *objcg; > - struct slab *slab; > - unsigned long off; > - size_t i; > - > - /* > - * The obtained objcg pointer is safe to use within the current scope, > - * defined by current task or set_active_memcg() pair. > - * obj_cgroup_get() is used to get a permanent reference. > - */ > - objcg = current_obj_cgroup(); > - if (!objcg) > - return true; > - > - /* > - * slab_alloc_node() avoids the NULL check, so we might be called with a > - * single NULL object. kmem_cache_alloc_bulk() aborts if it can't fill > - * the whole requested size. > - * return success as there's nothing to free back > - */ > - if (unlikely(*p == NULL)) > - return true; > - > - flags &= gfp_allowed_mask; > - > - if (lru) { > - int ret; > - struct mem_cgroup *memcg; > - > - memcg = get_mem_cgroup_from_objcg(objcg); > - ret = memcg_list_lru_alloc(memcg, lru, flags); > - css_put(&memcg->css); > - > - if (ret) > - return false; > - } > - > - if (obj_cgroup_charge(objcg, flags, size * obj_full_size(s))) > - return false; > - > - for (i = 0; i < size; i++) { > - slab = virt_to_slab(p[i]); > - > - if (!slab_objcgs(slab) && > - memcg_alloc_slab_cgroups(slab, s, flags, false)) { > - obj_cgroup_uncharge(objcg, obj_full_size(s)); > - continue; > - } > - > - off = obj_to_index(s, slab, p[i]); > - obj_cgroup_get(objcg); > - slab_objcgs(slab)[off] = objcg; > - mod_objcg_state(objcg, slab_pgdat(slab), > - cache_vmstat_idx(s), obj_full_size(s)); > - } > - > - return true; > -} > - > static void memcg_alloc_abort_single(struct kmem_cache *s, void *object); > > static __fastpath_inline > @@ -1986,27 +1907,6 @@ bool memcg_slab_post_alloc_hook(struct kmem_cache *s, struct list_lru *lru, > return false; > } > > -static void __memcg_slab_free_hook(struct kmem_cache *s, struct slab *slab, > - void **p, int objects, > - struct obj_cgroup **objcgs) > -{ > - for (int i = 0; i < objects; i++) { > - struct obj_cgroup *objcg; > - unsigned int off; > - > - off = obj_to_index(s, slab, p[i]); > - objcg = objcgs[off]; > - if (!objcg) > - continue; > - > - objcgs[off] = NULL; > - obj_cgroup_uncharge(objcg, obj_full_size(s)); > - mod_objcg_state(objcg, slab_pgdat(slab), cache_vmstat_idx(s), > - -obj_full_size(s)); > - obj_cgroup_put(objcg); > - } > -} > - > static __fastpath_inline > void memcg_slab_free_hook(struct kmem_cache *s, struct slab *slab, void **p, > int objects) > > -- > 2.44.0 > >