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 E93E4ECAAD5 for ; Tue, 6 Sep 2022 13:39:53 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 61D4180287; Tue, 6 Sep 2022 09:39:53 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 5CE1180224; Tue, 6 Sep 2022 09:39:53 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 446D680287; Tue, 6 Sep 2022 09:39:53 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0016.hostedemail.com [216.40.44.16]) by kanga.kvack.org (Postfix) with ESMTP id 3235F80224 for ; Tue, 6 Sep 2022 09:39:53 -0400 (EDT) Received: from smtpin08.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay04.hostedemail.com (Postfix) with ESMTP id EDC9E1A0D45 for ; Tue, 6 Sep 2022 13:39:52 +0000 (UTC) X-FDA: 79881768624.08.19B9611 Received: from mail-pg1-f179.google.com (mail-pg1-f179.google.com [209.85.215.179]) by imf31.hostedemail.com (Postfix) with ESMTP id 89AC12008C for ; Tue, 6 Sep 2022 13:39:52 +0000 (UTC) Received: by mail-pg1-f179.google.com with SMTP id q63so10653281pga.9 for ; Tue, 06 Sep 2022 06:39:52 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date; bh=ELnFZ093Jt8a9F8ubwxNseh5G1zdBMWLaLtcD4FqOZE=; b=OuSYJdIuEX5whZd9SA3DSBwHK8x34qKwbq0nOoZlJGCHJ1GESAUvXRz8ZB+LfPMrhP f98H0sCXQaL9bgJMRFL9JPHRPuH3r+/g8Ru6RSrB6qRdHBviTEEHjayvXHg+rhJ4sYGs /N4doEfM7+Ry8XcX/QCvNz48IYcaoUiMh+IlQWwgWLqfsDYHY2gNuYModPm4Egz20TIm 2Kw1Qx4V+MzYo46yrnX/iCXNFSrA2TI7YN2GtwBl5kyb0Yg7oY2be2Nntcg3eQwotyaS wXTFcu9S96rBGyQwUwDIL4MwFphygBCw2no7O99nWJBSQ72slo9f1gYRT1vBo8bIVr6o p0gg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date; bh=ELnFZ093Jt8a9F8ubwxNseh5G1zdBMWLaLtcD4FqOZE=; b=uSM9IpQDL3cp7u1JlCyWnw0DJuPX9PXH7KekpqYmc9x7rvC0bbgtn9XrF7bGUj/5j1 svtwMFzqujkh5hoRYfL5nfkA5T17Q+T/qCOhMiVivKJVBoiT3GXkbBnxGJs1pXQxGzyy 3K2SYYh2+xW9/JbPuPlBg9/Di1sJPjZdl+jiWxpKRTulaLWBblBqydRl2Dl6FTq+hGdj yv08jXw+pIOsYuqkDETKocisZr+6083c9JccITSwtGBFlfBkyenxgmwPIr2wzQBGdxmm l/877/e9JMHywSZCikKxhRVQAhU+wmFY1lSGt26j1g6sifoiwnHFh9nsBMVh8zlGqyGS OpZg== X-Gm-Message-State: ACgBeo0oSkZLI5A2auosx5+q8EDrxN3dCaJ0pEj7kShv3dSYQ8uOMVGz hdcObzA4VkDjKOy7cqQuldM= X-Google-Smtp-Source: AA6agR4tUlXyL8oUwLgGVvyxDTE5fbbjRA02wDD/zxu+KXrP25F7GIT/yUnGMEPufJoUfcnByvcahA== X-Received: by 2002:a63:4d5b:0:b0:42c:299e:eecc with SMTP id n27-20020a634d5b000000b0042c299eeeccmr35534099pgl.41.1662471591352; Tue, 06 Sep 2022 06:39:51 -0700 (PDT) Received: from hyeyoo ([114.29.91.56]) by smtp.gmail.com with ESMTPSA id v6-20020a1709029a0600b00176a47e5840sm5276189plp.298.2022.09.06.06.39.46 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 06 Sep 2022 06:39:50 -0700 (PDT) Date: Tue, 6 Sep 2022 22:39:44 +0900 From: Hyeonggon Yoo <42.hyeyoo@gmail.com> To: Feng Tang Cc: Vlastimil Babka , Andrew Morton , Christoph Lameter , Pekka Enberg , David Rientjes , Joonsoo Kim , Roman Gushchin , Dmitry Vyukov , "Hansen, Dave" , "linux-mm@kvack.org" , "linux-kernel@vger.kernel.org" , Robin Murphy , John Garry , Kefeng Wang Subject: Re: [PATCH v4 1/4] mm/slub: enable debugging memory wasting of kmalloc Message-ID: References: <8ff805f4-76ae-fc0f-424f-4d230c08285e@suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1662471592; a=rsa-sha256; cv=none; b=Mn2e/2aL69BnLj+dYC53qJd192fjpCriA93SAnbvL/P2vrVgVXvAx2/YToRqD70WOQbdk2 iCbqkXE2HceRjrd5wjmdmyPy20fLXvBdPGzgh3r1Jxk0c+Aa5gTIVip/mJfo8kEfc8CUoh OijPySRvT/Uy153J+7tdzo40ER+iVvk= ARC-Authentication-Results: i=1; imf31.hostedemail.com; dkim=pass header.d=gmail.com header.s=20210112 header.b=OuSYJdIu; spf=pass (imf31.hostedemail.com: domain of 42.hyeyoo@gmail.com designates 209.85.215.179 as permitted sender) smtp.mailfrom=42.hyeyoo@gmail.com; dmarc=pass (policy=none) header.from=gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1662471592; 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=ELnFZ093Jt8a9F8ubwxNseh5G1zdBMWLaLtcD4FqOZE=; b=YPfyXY6HHbGBq5U0LzPt/ozi8q4J3neY0BZe5JQMNragTG5DxU5nw7TmXR2fdiGptMJ2u4 wySEXLmen2SYqYZC2YXDMWBxPVdahmmkrsWmqX9KMq1Ewd9JRML1hCz7mIu2rzkG5QzUDJ YfU5DukCQUAWWz0v2QQfYcdmQhU1fUc= X-Stat-Signature: anc7akgwkxxc3rqzctsbdamdxx4eku3e X-Rspamd-Queue-Id: 89AC12008C Authentication-Results: imf31.hostedemail.com; dkim=pass header.d=gmail.com header.s=20210112 header.b=OuSYJdIu; spf=pass (imf31.hostedemail.com: domain of 42.hyeyoo@gmail.com designates 209.85.215.179 as permitted sender) smtp.mailfrom=42.hyeyoo@gmail.com; dmarc=pass (policy=none) header.from=gmail.com X-Rspamd-Server: rspam11 X-Rspam-User: X-HE-Tag: 1662471592-499596 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 Mon, Sep 05, 2022 at 04:37:05PM +0800, Feng Tang wrote: > 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 Looks fine to me. will review when next version arrives :) > --- > 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; > } -- Thanks, Hyeonggon