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 23807C54791 for ; Wed, 13 Mar 2024 17:34:16 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 9B10880054; Wed, 13 Mar 2024 13:34:15 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 95E76940010; Wed, 13 Mar 2024 13:34:15 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 826C380054; Wed, 13 Mar 2024 13:34:15 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0017.hostedemail.com [216.40.44.17]) by kanga.kvack.org (Postfix) with ESMTP id 706A3940010 for ; Wed, 13 Mar 2024 13:34:15 -0400 (EDT) Received: from smtpin05.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay01.hostedemail.com (Postfix) with ESMTP id 0C3111C0894 for ; Wed, 13 Mar 2024 17:34:15 +0000 (UTC) X-FDA: 81892714470.05.B7A2DEB Received: from out-181.mta1.migadu.com (out-181.mta1.migadu.com [95.215.58.181]) by imf10.hostedemail.com (Postfix) with ESMTP id 0A8EDC0028 for ; Wed, 13 Mar 2024 17:34:11 +0000 (UTC) Authentication-Results: imf10.hostedemail.com; dkim=pass header.d=linux.dev header.s=key1 header.b=i9O5fi+S; dmarc=pass (policy=none) header.from=linux.dev; spf=pass (imf10.hostedemail.com: domain of roman.gushchin@linux.dev designates 95.215.58.181 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=1710351252; 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=Wol+p/8LL2LDmxvbQ9tqtsHeVEpgewKE43vwTQIQciI=; b=gONxO5gBIfTGaQLHDkZPJDwVeBG2C0ZM+qT16ETc1ixT8eju3gJE0VBznh0It/aaO1OtFJ tEdB5E9WY8YVgY+Oeizx1DQGPkr5h9hKb16iaHHpHHbz1xZ1ywI3lhRGtIUAt/NdRcvAAW 6b7PUkbHisE8XM3YZ6G2iU4MIkSJ7aw= ARC-Authentication-Results: i=1; imf10.hostedemail.com; dkim=pass header.d=linux.dev header.s=key1 header.b=i9O5fi+S; dmarc=pass (policy=none) header.from=linux.dev; spf=pass (imf10.hostedemail.com: domain of roman.gushchin@linux.dev designates 95.215.58.181 as permitted sender) smtp.mailfrom=roman.gushchin@linux.dev ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1710351252; a=rsa-sha256; cv=none; b=Z7x46FGIY8GbCAbaVKKdt+uOB2dzRfWL9A4PTrBTteWMdLM/XOFUNQdQphqf97Ea8ihchR 2sWJ1pThb/5mcHRaKz2INLW4EqPUfTsCheuyHSFSDp/y343X47kw4WrUYOCicAbL+UrwZC UnndahljoVQKhUvfeSCKQw9zoB51bGg= Date: Wed, 13 Mar 2024 10:34:02 -0700 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.dev; s=key1; t=1710351249; 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=Wol+p/8LL2LDmxvbQ9tqtsHeVEpgewKE43vwTQIQciI=; b=i9O5fi+SXWvJdxW5Y+LAyDuRwOq8j5mxhExsiND2GKS/paYNKMzxa8ft6bhjUogbgbM/k6 rNA6LgBgasXbGfI1+VcTZUQH8VlOXLZs1sFopHRnBTidvxUOvmqdZUMEdvGbeuER3AyX9J 3g5ZwMx3uU/mQrxbBF1Fh7x7Jgyzo+0= 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 1/4] mm, slab: move memcg charging to post-alloc hook Message-ID: References: <20240301-slab-memcg-v1-0-359328a46596@suse.cz> <20240301-slab-memcg-v1-1-359328a46596@suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Migadu-Flow: FLOW_OUT X-Rspamd-Queue-Id: 0A8EDC0028 X-Rspam-User: X-Rspamd-Server: rspam05 X-Stat-Signature: b4b84x3upiuqfp3eo4gwyszyqthpu4e5 X-HE-Tag: 1710351251-411279 X-HE-Meta: U2FsdGVkX19naCHAYVET+x2BQCF6tyoIPWsDgnbl4h+5hmk8315H/c9g4NkJgtQNjyWHpalekGENHUrnQidMir4iILuDDBfi3rfFJ9AZY0UCQ2vfDDesuo0hDw75pinC+G0TJnV0kSjI7KnAM1LS2NYm/OHWTy773aXneLgo6vCWdGZwMIsjVM6zVBZiiMtGwlgjlDqMgEiU6nqexfIUEze1DEUrosTTr7eqNSV+s/T1SW+OqTiWnKMeYbsGckmOr696FPe5xlTFdyTRpSgSr4zNkON2mWExhoXxb2qFGnjJmI20jl2Rh1SZTFEUTVR/J9+SIsLLwLh739eb/PUgFoV/Tss+QBglzUZjte2t3EI6w42rRxsHr5boGs96M47bLjfY5sujFV2PRezhTPkJItOJUl4g1gSe8JL61d1UxXzTxCO0DHVVUUz1WltBV/vAmuZu2q08ngKaRS4LQvWQTDrX3mOEeX+M5coZRsliQ3IHu/e2QLHhYEP8IRwW7AkWHU/7SfTkVfAfVIu2aeGsN/widcBrKkfKX5p401gPidrX8Hi92smDfcWiY70/OKyPiNm2QPbYehA7Xujl2OOfJg4QmFyGTfdrv8T4ovw0IM+4XCPACKs7oNf78Z82DDAbbld1z4sBq9O8PVUyzjfoFiScmu60Dqv4Cni/l1FTZjW0h3SOAJfGj42U7sPkdCpbWiAkUvOc/PQpxFAyPiNgLcfERWAtKxWpLrZ43ChkN55nh95vnyEzZRelTYXCmn+iQld9Jx5gJs4XH2Xpts2+QmNDafDMGML13JNje1/1LDwzXCPSzIO0zNh9HoPZoD2yU+abYHqBpPpUgPb/+XubugjLpTTcHX+bvF/xsFitFeojAmcchtXR5wzqha5xzGxT70ppNjTEY16QT5NZNhXSalA0L9GjFKEPqRdBh/FkQcidRLkxJX3+wyQPBiOqvXY6fXiRMLhTpKvN1ePg2LR wbK3DO1L fjg1JWhdu7jYwdhrp7BmxzIK2pFHTwBrFjklK5nVOi1uq7GRnKZxW8JTZgBRUYGkc2ueaoBsSqUY0IEl6K/t1hOdv35eno8SFeFQCn5dkIVs/p7Gi6baEKCwCbHM5Z/CS+nqcGiiNSZnUSW3jNXvbPUZtOQ6KCVUSJNNNQeCK/RZCNRr7f1vTVfffT6pjJLnISBnwq4CADePaux8Z+8C95ywEa7gTzmSHch2+8NJkCWU1Iiq4KasJ72Edfa1r9gIz0bOUEMurvYBoRGJqKL1nbZftFx3SDOpNXc5CI5M0P1jkHo0= 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 Wed, Mar 13, 2024 at 11:55:04AM +0100, Vlastimil Babka wrote: > On 3/12/24 19:52, Roman Gushchin wrote: > > On Fri, Mar 01, 2024 at 06:07:08PM +0100, Vlastimil Babka wrote: > >> The MEMCG_KMEM integration with slab currently relies on two hooks > >> during allocation. memcg_slab_pre_alloc_hook() determines the objcg and > >> charges it, and memcg_slab_post_alloc_hook() assigns the objcg pointer > >> to the allocated object(s). > >> > >> As Linus pointed out, this is unnecessarily complex. Failing to charge > >> due to memcg limits should be rare, so we can optimistically allocate > >> the object(s) and do the charging together with assigning the objcg > >> pointer in a single post_alloc hook. In the rare case the charging > >> fails, we can free the object(s) back. > >> > >> This simplifies the code (no need to pass around the objcg pointer) and > >> potentially allows to separate charging from allocation in cases where > >> it's common that the allocation would be immediately freed, and the > >> memcg handling overhead could be saved. > >> > >> Suggested-by: Linus Torvalds > >> Link: https://lore.kernel.org/all/CAHk-=whYOOdM7jWy5jdrAm8LxcgCMFyk2bt8fYYvZzM4U-zAQA@mail.gmail.com/ > >> Signed-off-by: Vlastimil Babka > > > > Nice cleanup, Vlastimil! > > Couple of small nits below, but otherwise, please, add my > > > > Reviewed-by: Roman Gushchin > > Thanks! > > >> /* > >> * 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. > >> */ > >> - struct obj_cgroup *objcg = current_obj_cgroup(); > >> + 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; > > > > Probably better to move this check up? current_obj_cgroup() != NULL check is more > > expensive. > > It probably doesn't matter in practice anyway, but my thinking was that > *p == NULL is so rare (the object allocation failed) it shouldn't matter > that we did current_obj_cgroup() uselessly in case it happens. > OTOH current_obj_cgroup() returning NULL is not that rare (?) so it > could be useful to not check *p in those cases? I see... Hm, I'd generally expect a speculative execution of the second check anyway (especially with an unlikely() hint for the first one), and because as you said p == NULL is almost never true, the additional cost is zero. But the same is true otherwise, so it really doesn't matter that much. Thanks for explaining your logic, it wasn't obvious to me.