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 67971C433EF for ; Mon, 28 Mar 2022 23:44:54 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id AF5E68D0002; Mon, 28 Mar 2022 19:44:53 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id AA4C28D0001; Mon, 28 Mar 2022 19:44:53 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 945478D0002; Mon, 28 Mar 2022 19:44:53 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (relay.a.hostedemail.com [64.99.140.24]) by kanga.kvack.org (Postfix) with ESMTP id 8182B8D0001 for ; Mon, 28 Mar 2022 19:44:53 -0400 (EDT) Received: from smtpin01.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay11.hostedemail.com (Postfix) with ESMTP id 4531D80272 for ; Mon, 28 Mar 2022 23:44:53 +0000 (UTC) X-FDA: 79295427666.01.9884296 Received: from out2.migadu.com (out2.migadu.com [188.165.223.204]) by imf10.hostedemail.com (Postfix) with ESMTP id 64854C0041 for ; Mon, 28 Mar 2022 23:44:52 +0000 (UTC) Date: Mon, 28 Mar 2022 16:44:45 -0700 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.dev; s=key1; t=1648511090; 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=a/CwHgY1OQH27D1lFMO1ZeHx5K6kGyMSqY6y7nOOFAE=; b=OtAGsM4wI8P38y+LX/EzDOHIeRGVdxEnG4+bxRWWO9b6gmBsR8lvwLOWDDzNYtF8zdvKTy u3Yx4YFiKFwjVddd+CeFXt1b2xVWAh8aY4VWve4ba2U32S82IerLXbUZbdBcq/eTK1kS5k sB61GcIJD7nmGIBQjT4nc1UCF/wrt00= X-Report-Abuse: Please report any abuse attempt to abuse@migadu.com and include these headers. From: Roman Gushchin To: Waiman Long Cc: Muchun Song , Andrew Morton , Linux Memory Management List , LKML Subject: Re: [PATCH-mm v3] mm/list_lru: Optimize memcg_reparent_list_lru_node() Message-ID: References: <20220309144000.1470138-1-longman@redhat.com> <2263666d-5eef-b1fe-d5e3-b166a3185263@redhat.com> <5aa687c4-2888-7977-8c1a-d51384e685aa@redhat.com> <9e184cff-263a-d83a-0fc9-0ac7d453aa2a@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <9e184cff-263a-d83a-0fc9-0ac7d453aa2a@redhat.com> X-Migadu-Flow: FLOW_OUT X-Migadu-Auth-User: linux.dev X-Stat-Signature: 6g54arh8xezjc677sba3jaz46kks6o4m Authentication-Results: imf10.hostedemail.com; dkim=pass header.d=linux.dev header.s=key1 header.b=OtAGsM4w; spf=pass (imf10.hostedemail.com: domain of roman.gushchin@linux.dev designates 188.165.223.204 as permitted sender) smtp.mailfrom=roman.gushchin@linux.dev; dmarc=pass (policy=none) header.from=linux.dev X-Rspam-User: X-Rspamd-Server: rspam02 X-Rspamd-Queue-Id: 64854C0041 X-HE-Tag: 1648511092-147007 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, Mar 28, 2022 at 05:20:16PM -0400, Waiman Long wrote: > On 3/28/22 17:12, Roman Gushchin wrote: > > On Mon, Mar 28, 2022 at 04:46:39PM -0400, Waiman Long wrote: > > > On 3/28/22 15:12, Roman Gushchin wrote: > > > > On Sun, Mar 27, 2022 at 08:57:15PM -0400, Waiman Long wrote: > > > > > On 3/22/22 22:12, Muchun Song wrote: > > > > > > On Wed, Mar 23, 2022 at 9:55 AM Waiman Long wrote: > > > > > > > On 3/22/22 21:06, Muchun Song wrote: > > > > > > > > On Wed, Mar 9, 2022 at 10:40 PM Waiman Long wrote: > > > > > > > > > Since commit 2c80cd57c743 ("mm/list_lru.c: fix list_lru_count_node() > > > > > > > > > to be race free"), we are tracking the total number of lru > > > > > > > > > entries in a list_lru_node in its nr_items field. In the case of > > > > > > > > > memcg_reparent_list_lru_node(), there is nothing to be done if nr_items > > > > > > > > > is 0. We don't even need to take the nlru->lock as no new lru entry > > > > > > > > > could be added by a racing list_lru_add() to the draining src_idx memcg > > > > > > > > > at this point. > > > > > > > > Hi Waiman, > > > > > > > > > > > > > > > > Sorry for the late reply. Quick question: what if there is an inflight > > > > > > > > list_lru_add()? How about the following race? > > > > > > > > > > > > > > > > CPU0: CPU1: > > > > > > > > list_lru_add() > > > > > > > > spin_lock(&nlru->lock) > > > > > > > > l = list_lru_from_kmem(memcg) > > > > > > > > memcg_reparent_objcgs(memcg) > > > > > > > > memcg_reparent_list_lrus(memcg) > > > > > > > > memcg_reparent_list_lru() > > > > > > > > memcg_reparent_list_lru_node() > > > > > > > > if (!READ_ONCE(nlru->nr_items)) > > > > > > > > // Miss reparenting > > > > > > > > return > > > > > > > > // Assume 0->1 > > > > > > > > l->nr_items++ > > > > > > > > // Assume 0->1 > > > > > > > > nlru->nr_items++ > > > > > > > > > > > > > > > > IIUC, we use nlru->lock to serialise this scenario. > > > > > > > I guess this race is theoretically possible but very unlikely since it > > > > > > > means a very long pause between list_lru_from_kmem() and the increment > > > > > > > of nr_items. > > > > > > It is more possible in a VM. > > > > > > > > > > > > > How about the following changes to make sure that this race can't happen? > > > > > > > > > > > > > > diff --git a/mm/list_lru.c b/mm/list_lru.c > > > > > > > index c669d87001a6..c31a0a8ad4e7 100644 > > > > > > > --- a/mm/list_lru.c > > > > > > > +++ b/mm/list_lru.c > > > > > > > @@ -395,9 +395,10 @@ static void memcg_reparent_list_lru_node(struct > > > > > > > list_lru *lru, int nid, > > > > > > > struct list_lru_one *src, *dst; > > > > > > > > > > > > > > /* > > > > > > > - * If there is no lru entry in this nlru, we can skip it > > > > > > > immediately. > > > > > > > + * If there is no lru entry in this nlru and the nlru->lock is free, > > > > > > > + * we can skip it immediately. > > > > > > > */ > > > > > > > - if (!READ_ONCE(nlru->nr_items)) > > > > > > > + if (!READ_ONCE(nlru->nr_items) && !spin_is_locked(&nlru->lock)) > > > > > > I think we also should insert a smp_rmb() between those two loads. > > > > > Thinking about this some more, I believe that adding spin_is_locked() check > > > > > will be enough for x86. However, that will likely not be enough for arches > > > > > with a more relaxed memory semantics. So the safest way to avoid this > > > > > possible race is to move the check to within the lock critical section, > > > > > though that comes with a slightly higher overhead for the 0 nr_items case. I > > > > > will send out a patch to correct that. Thanks for bring this possible race > > > > > to my attention. > > > > Yes, I think it's not enough: > > > > CPU0 CPU1 > > > > READ_ONCE(&nlru->nr_items) -> 0 > > > > spin_lock(&nlru->lock); > > > > nlru->nr_items++; > > > > spin_unlock(&nlru->lock); > > > > && !spin_is_locked(&nlru->lock) -> 0 > > > I have actually thought of that. I am even thinking about reading nr_items > > > again after spin_is_locked(). Still for arches with relaxed memory > > > semantics, when will a memory write by one cpu be propagated to another cpu > > > can be highly variable. It is very hard to prove that it is completely safe. > > > > > > x86 has a more strict memory semantics and it is the only architecture that > > > I have enough confidence that doing the check without taking a lock can be > > > safe. Perhaps we could use this optimization just for x86 and do it inside > > > locks for the rests. > > Hm, is this such a big problem in the real life? Can you describe the setup? > > I'm somewhat resistant to an idea of having arch-specific optimizations here > > without a HUGE reason. > > I am just throwing this idea out for discussion. It does not mean that I > want to do an arch specific patch unless there is performance data to > indicate a substantial gain in performance in some use cases. Got it! I mean it's not obvious from the original commit message if it's just a nice optimization or there was a real life problem. In the first case the best thing is probably to leave everything as it is now, in the latter we need to do something. I'm trying to understand which case it is. Thanks!