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:   Sun, 4 Sep 2022 17:42:33 +0800
From:   Feng Tang <feng.tang@intel.com>
To:     Hyeonggon Yoo <42.hyeyoo@gmail.com>
CC:     Andrew Morton <akpm@linux-foundation.org>,
        Vlastimil Babka <vbabka@suse.cz>,
        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: <YxRzCX8WqOv3KESJ@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>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <YxRp5uz9KSY0S9id@hyeyoo>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?ATUYQBK8lNJBUeARI53CZMmnCu2DqYNE4pmTGk/2fShrfJf8w4S5rmuon8Ad?=
 =?us-ascii?Q?i2JH4C63VswgHMwdDSTyAg0SssQlxwtzQe9wRerFguyO5vbaD+1qPUoG6afK?=
 =?us-ascii?Q?jHZJAqyHEuX7ljQPJFDan9c3ZeE8nVeNnU8MCCvgQ1iSoq8n5HfN3T0D1bto?=
 =?us-ascii?Q?7YJrWc4G7hL8qYf1fZ4TUxDn9IFw+VpCplImUnIvR6rmIWJwpSRwNFh3G9B1?=
 =?us-ascii?Q?8ocr+hQBLNpumMtHtiEHzX5wBuGYJYRdRJJCZtQxVDoQ7QrC9z4ZJhe08Nm9?=
 =?us-ascii?Q?IdD/V9aO7xdv3wCiW553tQooKOSnzfLkuNF+w+RE8ipf7xdw0u5sJSbwXN39?=
 =?us-ascii?Q?1arpyaJNeoGLIYyGjyMgKCmSXOttLDoj4jIhccnBG6eOQgEJMjVRbFyqS5KS?=
 =?us-ascii?Q?VgbkBrD54uz9HMwucAfyIuWlN4+NuvhkVAxDWocuvJrInPrmaal/jm02ykYR?=
 =?us-ascii?Q?yu3DTppDm52IBcwcU2/vtLYnjNIi94/8nw6Hvpv8B6o7UTZGq3oRppoO8TxF?=
 =?us-ascii?Q?74LkRwas0Vz7FQ6IpB1cBldLayDLbbpMs8kV0Z4piucqkcDaD+93Cjp0LmTE?=
 =?us-ascii?Q?R2k99i0gfokCCmNkS/vuBl7HVLk9kXVwpMlXn2ROyFl9YcOZ/4jKxFckgrRj?=
 =?us-ascii?Q?QN8G9umkiYyOY/vaxKxaAQJ49TsbY2GT5LwnULIfIe6+ckOy2EEhPHOT0M0k?=
 =?us-ascii?Q?2LBocjOuDwmTkWJg8LBgjh7kdQU0LXzvg+MNQ1a3ojbU07lk6G8Qap0FiKpp?=
 =?us-ascii?Q?iCeqAttI2vQ40rHVWIIXSfNavWiwBpWh4C7DmYhMbGjMMLQaCsgo79JFSKtf?=
 =?us-ascii?Q?RQgb+idKggz5XJleivOVknLq2CV6qMcML06l+fZKJtLpwF0U138PrYbhMVZC?=
 =?us-ascii?Q?xS6W3V5tiyaVRZZDzI9s5wCCcuJn0mxBqqXZE5SlHMIT4xvcbraGqHWOGya9?=
 =?us-ascii?Q?fmJ9YQzyU/1Zyu9e96CEbIDuEi4NcgXJRqB3BXwgsiZXeb9otAscputdDyby?=
 =?us-ascii?Q?aZZ1qMHTXHXVrFJtR0XWT+fUKFaA6Yj21KtneP3uZo7Xt7ilyrcfBgwuN/FC?=
 =?us-ascii?Q?G7/kGjQsBFpLBQosBnqNf8IR0YafyVGXUhMFeHxh6SrxQPAUIPA+iojzKjqM?=
 =?us-ascii?Q?f73eRL+S8s1WNjLlSPJLJ3vfiqvT8yfip/jloPsx4j+fny4DOBsFdbsPClR9?=
 =?us-ascii?Q?/QhEO8CqIYUBUEWg1vkQnE+iJyOeNSvNkAINlIISKoor8t6cOves+BBnFN6q?=
 =?us-ascii?Q?qH+/UPx833quWNK3M6kOURsv4Od0x1zt9UkISlUXKYe9GEwY9kDyyX6GqvIh?=
 =?us-ascii?Q?dT8kUuqplM058O9GMvdfNeUkhXBoqoZt2PdR9HedcONZZT8cw2XB86SEiaeN?=
 =?us-ascii?Q?fLvJgCaRJuO9/GpGmDBy1pKC3vJrbJLm0mV+gMcXqz54sj0J9dQyiSZ6973p?=
 =?us-ascii?Q?g5K8/4BkhwXOB994RAkOS2fOYcb8kcdZByMa9HtSDgt3TFCV/8+DCeH+un9p?=
 =?us-ascii?Q?XkqEi+lJYuNwFl8CE3pnfIwwdUiuUCyz3NUxucrkP5bLcTvaJ7uwGaN/16PD?=
 =?us-ascii?Q?1B1EaptnLBSkYKf8o+5kiqOT6njc2doauXa19V4D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 6e5ebe9a-dc70-4770-f26c-08da8e59e1ae
X-MS-Exchange-CrossTenant-AuthSource: MN0PR11MB6304.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 04 Sep 2022 09:43:11.7481
 (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: NOXewKdUiODZF4OtPVVzhyBwId8MAbZK4nHjvczziZzrpgRgV5dkZG4nfMvizaW6ZmpP0TehDUdfL2e+DaK7Jg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH2PR11MB4406
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,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 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.

Thanks,
Feng

> -- 
> Thanks,
> Hyeonggon
>