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:   Mon, 5 Sep 2022 16:37:05 +0800
From:   Feng Tang <feng.tang@intel.com>
To:     Vlastimil Babka <vbabka@suse.cz>,
        Hyeonggon Yoo <42.hyeyoo@gmail.com>
CC:     Andrew Morton <akpm@linux-foundation.org>,
        Christoph Lameter <cl@linux.com>,
        Pekka Enberg <penberg@kernel.org>,
        David Rientjes <rientjes@google.com>,
        Joonsoo Kim <iamjoonsoo.kim@lge.com>,
        Roman Gushchin <roman.gushchin@linux.dev>,
        Dmitry Vyukov <dvyukov@google.com>,
        "Hansen, Dave" <dave.hansen@intel.com>,
        "linux-mm@kvack.org" <linux-mm@kvack.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Robin Murphy <robin.murphy@arm.com>,
        John Garry <john.garry@huawei.com>,
        Kefeng Wang <wangkefeng.wang@huawei.com>
Subject: Re: [PATCH v4 1/4] mm/slub: enable debugging memory wasting of
 kmalloc
Message-ID: <YxW1MUIKJ8B17xl1@feng-clx>
References: <20220829075618.69069-2-feng.tang@intel.com>
 <YxC7Kda5g/aEqarm@hyeyoo>
 <YxGfkdvJcNJma/88@feng-clx>
 <YxRp5uz9KSY0S9id@hyeyoo>
 <YxRzCX8WqOv3KESJ@feng-clx>
 <YxSE6XmHl3itv4JY@hyeyoo>
 <YxVlB/YRj7SNLqml@feng-clx>
 <8ff805f4-76ae-fc0f-424f-4d230c08285e@suse.cz>
 <YxWgEZTxyI/4ISHa@feng-clx>
 <f5057559-d4a7-552d-c5e8-1f1cd133a7f1@suse.cz>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <f5057559-d4a7-552d-c5e8-1f1cd133a7f1@suse.cz>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?W8xK9EWktj2Mw+MuppEOA1EB3XuKkElELukGoPguGbojGMCCgFlRLi0VTWmD?=
 =?us-ascii?Q?OROWbSH10wTcDkRSXn5r8knVw2TRSl2uTj0TDM7Or6bpufbijdWGQElf9olE?=
 =?us-ascii?Q?Sk3npmIlBr/iS2uEnc/YAKQB0bw07BDrh9jecNVmTPfdFPvgVUCEw6QJUXQp?=
 =?us-ascii?Q?yxReJgPjebKg7JItjpH456U4mYxyIjn9feLpW8xkFlBIjoar5iUvBbP/1prm?=
 =?us-ascii?Q?OakZFeuIi7BB2pCZv63zmMEibEir0BQ65e7TNOVO9nFWHLParpIVltvdZYOi?=
 =?us-ascii?Q?XQSD2UDdweIw3fe28CrpZinJhkvwaQ6UMdm2twGyGL7cDdSik6v3uYUYpdt5?=
 =?us-ascii?Q?t2GLX1zdoHq1Y73ctLDGE5Lnvee66CZJubMIWttl3a7lHqGo5inr7kn36SzN?=
 =?us-ascii?Q?N4qcMIliLqvLS15l+4sDp3UPM3WePnSJt741/hFI7By/crBuzZrjsjmzX37M?=
 =?us-ascii?Q?tydqG4XTndJED57uIvGal0NGb52Qyh1ev2JJrS8m8JVmVNhMmZt6KssFPGrX?=
 =?us-ascii?Q?WrgkLzLfnutHW0SvY5024TV6Qb2mSDkxrm2+aunSox9aNwJUFhTok4TJFzy5?=
 =?us-ascii?Q?CDSx3NhDzRxEQDYgL6aJQCcSMejuOq7DQ6+JKa5roUTQp1T6FrwTrDj3jtt4?=
 =?us-ascii?Q?/IzjWXuSa9e7omOSdKgzuPllJuu15DTfVcedloCiLcYvpyp9qgUo0b/jVG05?=
 =?us-ascii?Q?nZeD8C4JOiTtNAR1VJfK459xjnk25h2A3jYDyYE/9qm+Va7hq+LTkVy7eVJ1?=
 =?us-ascii?Q?na6k5W/owXYZ98QdnfQDpXNn9CwDZW4g5EdvYo90oluQc8RHbPdE2jkQ+0GT?=
 =?us-ascii?Q?YOBR920exyCy2o7uelXYGP0AbgzLfyDXYAZFAy0RR0sqbG0zCINwWdxQxJS4?=
 =?us-ascii?Q?ZVPd0vaaRhZX8vI9DVEQKuohU5i2wiAzHTl8pYXtF0LEveKcevVz6DdfrY7j?=
 =?us-ascii?Q?P+jTrHUQa8Cu/S0ynOqUHsErDx08F/7ypOg4Q1nuoIdo203V/N9owddsG8eW?=
 =?us-ascii?Q?sxBwg61tC/iCa380CMqYEiQbBTVTgIkPP9W6VRJU3tcHNQC/KwykCh/ux/DK?=
 =?us-ascii?Q?eFnNvg4H1+Z/Ev1h9Dj/f0OadwHhTcgQgG/ccLB+X4rp9VmfcuSYV5NNUkRc?=
 =?us-ascii?Q?bD2uyfTjivfrxpHmZMsqFPsClivrIDdJEKuujTUQYTxb+ge7JnWS/2lFeKjD?=
 =?us-ascii?Q?DGU5KbMsxnpe5YCs6hqNKpc5VMJak8evZwnpGdwQHkM3DzdvkVI19Sjtqq3O?=
 =?us-ascii?Q?GdomFZoHtEalHzpRRUkG96Hwu3vqJqxjI6BPFoQfJHEc1t8SsFTat94J6JL7?=
 =?us-ascii?Q?R8hm86dasbokOrtvIU2ObC2eDP9ybcPRY6UekR7H2HtzEvZrw9np/DZBberH?=
 =?us-ascii?Q?Waanqpl1k2C2CZtyb1Q/5rb6IS5zDKBQwd2N6eOuNXLul8WqiVB8b+5YzEKW?=
 =?us-ascii?Q?XA+2BX9c5pdbmlKOlEuLe3eWywVCYOsuJc4dR6Hzhch8YwukCpjmccq7pm3j?=
 =?us-ascii?Q?LprjaHQCqrq2WtKpJutcUqAHbEmV95pHE9J2aZgialfa9RVdFR/P124QSEmI?=
 =?us-ascii?Q?RmgByiAWb7MGyD/DC8AO2pCPZPoPrMDfGaqTOPir?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 7eea13f6-d46f-4756-bc80-08da8f19e44e
X-MS-Exchange-CrossTenant-AuthSource: MN0PR11MB6304.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 05 Sep 2022 08:37:39.5071
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: PHRLZKpMsOoHpKJ1dPsvM78HnmOfxKecd1B4BiqpUiItD9+VL84g2nxSFWmtlSibcE7nPRrj/K6zWEGmqEfT4A==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR11MB3574
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-7.1 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_HI,
        RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE,
        T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Mon, Sep 05, 2022 at 03:33:14PM +0800, Vlastimil Babka wrote:
> On 9/5/22 09:06, Feng Tang wrote:
> > On Mon, Sep 05, 2022 at 02:29:51PM +0800, Vlastimil Babka wrote:
> >> 
> >> How about get_partial() instantiates an on-stack structure that contains
> >> gfpflags, ret_slab, orig_size and passes pointer to that to all the nested
> >> functions.
> >> 
> >> Would be similar to "struct alloc_context" in page allocation.
> >> Something like "struct partial_context pc"?
> > 
> > Yep! This would make the parameters passing much tidier. Will try
> > this way. 
> > 
> > More aggressively is to also embed the 'kmem_cache' parameter into
> > it, but this may make the code look ambiguous.
> 
> That one is used a lot everywhere, so it would be tedious to dereference it
> from a struct, and also might be a bit better code if it's in a register.

Got it. Following is the incremental patch for 1/4, which uses the
'partial_context' to pass parameters. And actually the 4/4 patch will
benefit more from this refactoring, as the object initialization doesn't
need to be separated and has race issue.

Thanks,
Feng

---
diff --git a/mm/slub.c b/mm/slub.c
index 82e7bd3a3966..7497fb6ca8e2 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -194,6 +194,12 @@ DEFINE_STATIC_KEY_FALSE(slub_debug_enabled);
 #endif
 #endif		/* CONFIG_SLUB_DEBUG */
 
+struct partial_context {
+	struct slab **slab;
+	gfp_t flags;
+	int orig_size;
+};
+
 static inline bool kmem_cache_debug(struct kmem_cache *s)
 {
 	return kmem_cache_debug_flags(s, SLAB_DEBUG_FLAGS);
@@ -1333,7 +1339,7 @@ static inline int alloc_consistency_checks(struct kmem_cache *s,
 }
 
 static noinline int alloc_debug_processing(struct kmem_cache *s,
-					struct slab *slab, void *object)
+			struct slab *slab, void *object, int orig_size)
 {
 	if (s->flags & SLAB_CONSISTENCY_CHECKS) {
 		if (!alloc_consistency_checks(s, slab, object))
@@ -1342,6 +1348,7 @@ static noinline int alloc_debug_processing(struct kmem_cache *s,
 
 	/* Success. Perform special debug activities for allocs */
 	trace(s, slab, object, 1);
+	set_orig_size(s, object, orig_size);
 	init_object(s, object, SLUB_RED_ACTIVE);
 	return 1;
 
@@ -1610,7 +1617,7 @@ static inline
 void setup_slab_debug(struct kmem_cache *s, struct slab *slab, void *addr) {}
 
 static inline int alloc_debug_processing(struct kmem_cache *s,
-	struct slab *slab, void *object) { return 0; }
+	struct slab *slab, void *object, int orig_size) { return 0; }
 
 static inline void set_orig_size(struct kmem_cache *s,
 	void *object, unsigned int orig_size) {}
@@ -2042,7 +2049,7 @@ static inline void remove_partial(struct kmem_cache_node *n,
  * it to full list if it was the last free object.
  */
 static void *alloc_single_from_partial(struct kmem_cache *s,
-		struct kmem_cache_node *n, struct slab *slab)
+		struct kmem_cache_node *n, struct slab *slab, int orig_size)
 {
 	void *object;
 
@@ -2052,7 +2059,7 @@ static void *alloc_single_from_partial(struct kmem_cache *s,
 	slab->freelist = get_freepointer(s, object);
 	slab->inuse++;
 
-	if (!alloc_debug_processing(s, slab, object)) {
+	if (!alloc_debug_processing(s, slab, object, orig_size)) {
 		remove_partial(n, slab);
 		return NULL;
 	}
@@ -2071,7 +2078,7 @@ static void *alloc_single_from_partial(struct kmem_cache *s,
  * and put the slab to the partial (or full) list.
  */
 static void *alloc_single_from_new_slab(struct kmem_cache *s,
-					struct slab *slab)
+					struct slab *slab, int orig_size)
 {
 	int nid = slab_nid(slab);
 	struct kmem_cache_node *n = get_node(s, nid);
@@ -2083,7 +2090,7 @@ static void *alloc_single_from_new_slab(struct kmem_cache *s,
 	slab->freelist = get_freepointer(s, object);
 	slab->inuse = 1;
 
-	if (!alloc_debug_processing(s, slab, object))
+	if (!alloc_debug_processing(s, slab, object, orig_size))
 		/*
 		 * It's not really expected that this would fail on a
 		 * freshly allocated slab, but a concurrent memory
@@ -2161,7 +2168,7 @@ static inline bool pfmemalloc_match(struct slab *slab, gfp_t gfpflags);
  * Try to allocate a partial slab from a specific node.
  */
 static void *get_partial_node(struct kmem_cache *s, struct kmem_cache_node *n,
-			      struct slab **ret_slab, gfp_t gfpflags)
+			      struct partial_context *pc)
 {
 	struct slab *slab, *slab2;
 	void *object = NULL;
@@ -2181,11 +2188,11 @@ static void *get_partial_node(struct kmem_cache *s, struct kmem_cache_node *n,
 	list_for_each_entry_safe(slab, slab2, &n->partial, slab_list) {
 		void *t;
 
-		if (!pfmemalloc_match(slab, gfpflags))
+		if (!pfmemalloc_match(slab, pc->flags))
 			continue;
 
 		if (kmem_cache_debug(s)) {
-			object = alloc_single_from_partial(s, n, slab);
+			object = alloc_single_from_partial(s, n, slab, pc->orig_size);
 			if (object)
 				break;
 			continue;
@@ -2196,7 +2203,7 @@ static void *get_partial_node(struct kmem_cache *s, struct kmem_cache_node *n,
 			break;
 
 		if (!object) {
-			*ret_slab = slab;
+			*pc->slab = slab;
 			stat(s, ALLOC_FROM_PARTIAL);
 			object = t;
 		} else {
@@ -2220,14 +2227,13 @@ static void *get_partial_node(struct kmem_cache *s, struct kmem_cache_node *n,
 /*
  * Get a slab from somewhere. Search in increasing NUMA distances.
  */
-static void *get_any_partial(struct kmem_cache *s, gfp_t flags,
-			     struct slab **ret_slab)
+static void *get_any_partial(struct kmem_cache *s, struct partial_context *pc)
 {
 #ifdef CONFIG_NUMA
 	struct zonelist *zonelist;
 	struct zoneref *z;
 	struct zone *zone;
-	enum zone_type highest_zoneidx = gfp_zone(flags);
+	enum zone_type highest_zoneidx = gfp_zone(pc->flags);
 	void *object;
 	unsigned int cpuset_mems_cookie;
 
@@ -2255,15 +2261,15 @@ static void *get_any_partial(struct kmem_cache *s, gfp_t flags,
 
 	do {
 		cpuset_mems_cookie = read_mems_allowed_begin();
-		zonelist = node_zonelist(mempolicy_slab_node(), flags);
+		zonelist = node_zonelist(mempolicy_slab_node(), pc->flags);
 		for_each_zone_zonelist(zone, z, zonelist, highest_zoneidx) {
 			struct kmem_cache_node *n;
 
 			n = get_node(s, zone_to_nid(zone));
 
-			if (n && cpuset_zone_allowed(zone, flags) &&
+			if (n && cpuset_zone_allowed(zone, pc->flags) &&
 					n->nr_partial > s->min_partial) {
-				object = get_partial_node(s, n, ret_slab, flags);
+				object = get_partial_node(s, n, pc);
 				if (object) {
 					/*
 					 * Don't check read_mems_allowed_retry()
@@ -2284,8 +2290,7 @@ static void *get_any_partial(struct kmem_cache *s, gfp_t flags,
 /*
  * Get a partial slab, lock it and return it.
  */
-static void *get_partial(struct kmem_cache *s, gfp_t flags, int node,
-			 struct slab **ret_slab)
+static void *get_partial(struct kmem_cache *s, int node, struct partial_context *pc)
 {
 	void *object;
 	int searchnode = node;
@@ -2293,11 +2298,11 @@ static void *get_partial(struct kmem_cache *s, gfp_t flags, int node,
 	if (node == NUMA_NO_NODE)
 		searchnode = numa_mem_id();
 
-	object = get_partial_node(s, get_node(s, searchnode), ret_slab, flags);
+	object = get_partial_node(s, get_node(s, searchnode), pc);
 	if (object || node != NUMA_NO_NODE)
 		return object;
 
-	return get_any_partial(s, flags, ret_slab);
+	return get_any_partial(s, pc);
 }
 
 #ifdef CONFIG_PREEMPTION
@@ -3022,6 +3027,7 @@ static void *___slab_alloc(struct kmem_cache *s, gfp_t gfpflags, int node,
 	void *freelist;
 	struct slab *slab;
 	unsigned long flags;
+	struct partial_context pc;
 
 	stat(s, ALLOC_SLOWPATH);
 
@@ -3135,7 +3141,10 @@ static void *___slab_alloc(struct kmem_cache *s, gfp_t gfpflags, int node,
 
 new_objects:
 
-	freelist = get_partial(s, gfpflags, node, &slab);
+	pc.flags = gfpflags;
+	pc.slab = &slab;
+	pc.orig_size = orig_size;
+	freelist = get_partial(s, node, &pc);
 	if (freelist)
 		goto check_new_slab;
 
@@ -3151,14 +3160,13 @@ static void *___slab_alloc(struct kmem_cache *s, gfp_t gfpflags, int node,
 	stat(s, ALLOC_SLAB);
 
 	if (kmem_cache_debug(s)) {
-		freelist = alloc_single_from_new_slab(s, slab);
+		freelist = alloc_single_from_new_slab(s, slab, orig_size);
 
 		if (unlikely(!freelist))
 			goto new_objects;
 
 		if (s->flags & SLAB_STORE_USER)
 			set_track(s, freelist, TRACK_ALLOC, addr);
-		set_orig_size(s, freelist, orig_size);
 
 		return freelist;
 	}
@@ -3184,7 +3192,6 @@ static void *___slab_alloc(struct kmem_cache *s, gfp_t gfpflags, int node,
 		 */
 		if (s->flags & SLAB_STORE_USER)
 			set_track(s, freelist, TRACK_ALLOC, addr);
-		set_orig_size(s, freelist, orig_size);
 
 		return freelist;
 	}