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 15:06:57 +0800
From:   Feng Tang <feng.tang@intel.com>
To:     Vlastimil Babka <vbabka@suse.cz>
CC:     Hyeonggon Yoo <42.hyeyoo@gmail.com>,
        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: <YxWgEZTxyI/4ISHa@feng-clx>
References: <20220829075618.69069-1-feng.tang@intel.com>
 <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>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <8ff805f4-76ae-fc0f-424f-4d230c08285e@suse.cz>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?3d9X7eKvpqBrvVjZaK+5qwe/cYmbwU1vsZttHL8xXQuDPZi3DWQEPtr/bnbg?=
 =?us-ascii?Q?p5udPM5KE+k8gTgTyOy0h7U6jtgUpAfCku1N3rOyIY1AGPw26u+IyXc1arSo?=
 =?us-ascii?Q?euWS987SG9Gol2ZeoeLBscBNPKgUMEtkyutp0oHfnBL24PGyyhW1FE1YQB1H?=
 =?us-ascii?Q?NfN/tdgu7fbq2M1MTzZ0CIln/zsyYfYREPGK2QeQJXIgNZe0Io50neo8/4kq?=
 =?us-ascii?Q?+Q9jpfCR9aaHWC2VRJiWkky2usErmllDhkyifrn/NmkZF/eYpncbwGoG7wPv?=
 =?us-ascii?Q?5NGeuzPGOUYF+tteZ1vAzaVKewIR1+nZgAgGXeiBq2V0qLCwiq0wSWo/Ee0C?=
 =?us-ascii?Q?7mfRZe4NRhTEf9pehYg22tky6sMtgu6UpCu0IbwAo0myKc7z9nIbzF7xTGWK?=
 =?us-ascii?Q?ppw6841EEW8UTiBL98k8grcXrLs6tGD8P5T+J4Y9AAJoDXEkddqnT+jVzj5L?=
 =?us-ascii?Q?He+TvAt38zGzK+oZ4IQZ67dBwNU+DjeFZP4Mm/sSI48fel3zm037g4B8cPiW?=
 =?us-ascii?Q?9XZ0PwYc1Zb4AB7O1/2DZ5KqjJTsV2NndR8WXAx87cCkDEVve7a3LwhjJ7ls?=
 =?us-ascii?Q?pOOhGh1dMOAdBI1NYEpYtiNseqQ1U6RNY9JBoRVLHJBVcEXhv+7MXjFNWjpy?=
 =?us-ascii?Q?WC/rXAnFit7jAkHpvAoT6RBFA/HJ0krWWEMluiIgz8Grvnpip5wyrkK4logL?=
 =?us-ascii?Q?HwBsE/Vmf9+kerp7N0QzqhqHgN1tyZV331lYU2bC06ippURZ5Kc14k82FQqz?=
 =?us-ascii?Q?V4W63WxP/v0C8IkQbWso82vPjSYN9x2Rxnu9HBi8O8Db9D4CdHNGhfCvwcWD?=
 =?us-ascii?Q?JTPj6Jes5QXBUJ5hQgDI34774aP8uy8vIaxrg5dRivnjXMh2Pgoba7iSVyiF?=
 =?us-ascii?Q?aBTZ26NGoSUWqNJjzq3YBXAP+rhwLhRcVwajnWZvHOC8RB9eyovb8eIqDRAq?=
 =?us-ascii?Q?o7CZea3w6FsM+fPYeR73AAU9gX4XT4O0ZKQXnFDEg5Wz4eSlbfbCYCVGSpHr?=
 =?us-ascii?Q?X2LnrnQKoXCvvF2ZVqNeuIM4/ywRgT8Gpgm6c1iwlLEabw76qAGz4rP/xbC0?=
 =?us-ascii?Q?SdlJLX7VL3SrEEyShV82jk7CGQewMkbG9slErV7g94pRPX5bQ99lxRaJkQyk?=
 =?us-ascii?Q?M1b/5jue75M43KnpEEmIWE1VlcrmlZmcxnca1cQUMbJ7WzZX/NXl/gAuD5Op?=
 =?us-ascii?Q?/o7BWHpy6Yi1lkW9sVQaVhDYhmMSImDh6RSCKiLa/eeOhOYU6REs1/2f5Vw0?=
 =?us-ascii?Q?uJOkEHPT8E7Tyvz8pNRPyl3zxAO/HSH8aSxm88V7JWOFB0BUtmk3T59Scn1f?=
 =?us-ascii?Q?Y5a1KtDEY+msu7L04AkkcrM//O7hR9TgvoffeAbBwCY+dpHq8LhJljr8bTCr?=
 =?us-ascii?Q?Ay7Z4v9G5YWwKpb0ZvvYD43RzKNz5Pcvvh2LCoSNtjqx4VCDGHyziCWPzpFH?=
 =?us-ascii?Q?xBd/MpfTPZtdCVgq+0KLwnzr6a/EwQJFThKg46MzGimvMOREZqb1Y++NaDQe?=
 =?us-ascii?Q?B+pdXdfXpMjIe+lO4Frzq4nCiOU35l/Ah1Q14K/RDDtBNSob/xW+no8CMfEr?=
 =?us-ascii?Q?K1pXFCCOGZjIG+PQCGyQFR7YIuPIufWHzZSma8dS?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 1cafcc75-279b-439f-e124-08da8f0d4d7c
X-MS-Exchange-CrossTenant-AuthSource: MN0PR11MB6304.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 05 Sep 2022 07:07:32.7628
 (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: 7hzAXRYR5QtxUeQ56qtY3iDHggr+NqKVU7+YrdCLUggth9OwILlxmP0w+IxIue2PhqsgJgFSDZVskHfXMehI8Q==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM4PR11MB5263
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,
        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 02:29:51PM +0800, Vlastimil Babka wrote:
> On 9/5/22 04:55, Feng Tang wrote:
> > On Sun, Sep 04, 2022 at 06:58:49PM +0800, Hyeonggon Yoo wrote:
> >> On Sun, Sep 04, 2022 at 05:42:33PM +0800, Feng Tang wrote:
> >> > On Sun, Sep 04, 2022 at 05:03:34PM +0800, Hyeonggon Yoo wrote:
> >> > [...]
> >> > > > > 
> >> > > > > This patch is okay but with patch 4, init_object() initializes redzone/poison area
> >> > > > > using s->object_size, and init_kmalloc_object() fixes redzone/poison area using orig_size.
> >> > > > > Why not do it in init_object() in the first time?
> >> > > > > 
> >> > > > > Also, updating redzone/poison area after alloc_single_from_new_slab()
> >> > > > > (outside list_lock, after adding slab to list) will introduce races with validation.
> >> > > > > 
> >> > > > > So I think doing set_orig_size()/init_kmalloc_object() in alloc_debug_processing() would make more sense.
> >> > > > 
> >> > > > Yes, this makes sense, and in v3, kmalloc redzone/poison setup was
> >> > > > done in alloc_debug_processing() (through init_object()). When
> >> > > > rebasing to v4, I met the classical problem: how to pass 'orig_size'
> >> > > > parameter :)
> >> > > > 
> >> > > > In latest 'for-next' branch, one call path for alloc_debug_processing()
> >> > > > is
> >> > > >   ___slab_alloc
> >> > > >     get_partial
> >> > > >       get_any_partial
> >> > > >         get_partial_node
> >> > > >           alloc_debug_processing
> >> > > > 
> >> > > > Adding 'orig_size' paramter to all these function looks horrible, and
> >> > > > I couldn't figure out a good way and chosed to put those ops after
> >> > > > 'set_track()'
> >> > > 
> >> > > IMO adding a parameter to them isn't too horrible...
> >> > > I don't see better solution than adding a parameter with current implementation.
> >> > > (Yeah, the code is quite complicated...)
> >> > > 
> >> > > It won't affect performance to meaningful degree as most of
> >> > > allocations will be served from cpu slab or percpu partial list. 
> >> > 
> >> > Thanks for the suggestion! I'm fine with it and just afraid other
> >> > developers may dislike the extra parameter. 
> >> > 
> >> > The race condition you mentioned is a valid concern, and I have thought
> >> > about it, one way is moving the set_orig_size() after the redzone/poision
> >> > setup, and in 'check_object()' we can detect whether the 'orig_size' is
> >> > set, and skip that check if it's not set yet. As the manual validate_slab
> >> > triggered from sysfs interface is a rare debug activity, I think skipping
> >> > one object shouldn't hurt much.
> >> 
> >> That will require smp_wmb()/smp_rmb() pair to make sure that
> >> effects of set_orig_size() to be visible after redzone/poison setup.
> > 
> > Yes, synchronization is needed here.
> > 
> >> Isn't it simpler to add a parameter?
> > 
> > OK, I can go this way in v5 if other developers are fine. thanks
> 
> 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.

Thanks,
Feng