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 C2B12C433F5 for ; Mon, 21 Feb 2022 16:44:18 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 25AAB8D0002; Mon, 21 Feb 2022 11:44:18 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 20ABD8D0001; Mon, 21 Feb 2022 11:44:18 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 0AC778D0002; Mon, 21 Feb 2022 11:44:18 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0117.hostedemail.com [216.40.44.117]) by kanga.kvack.org (Postfix) with ESMTP id EC0FD8D0001 for ; Mon, 21 Feb 2022 11:44:17 -0500 (EST) Received: from smtpin22.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay01.hostedemail.com (Postfix) with ESMTP id ADB3B181C991C for ; Mon, 21 Feb 2022 16:44:17 +0000 (UTC) X-FDA: 79167359754.22.FBCEAD2 Received: from galois.linutronix.de (Galois.linutronix.de [193.142.43.55]) by imf12.hostedemail.com (Postfix) with ESMTP id 13DFE4000A for ; Mon, 21 Feb 2022 16:44:16 +0000 (UTC) Date: Mon, 21 Feb 2022 17:44:13 +0100 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linutronix.de; s=2020; t=1645461854; 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=FanYIpzuzO2OIQcmAbO6hr4IzLwYQWIb8ljFjp/SrAs=; b=nEVg+O/ZK98mEFQmhOseYs8lnNeH5TrEg8Jk3+iUr8PcEMutyi03fcN4iEX1GRo/t6b5kP M0/7n71mvbQcDqNA4E0EaAKsrrHsbFuXh/G1zUTGDUYIYTCjx7ieNkZMQCZe/ca9sPFUic FLWmNSrJanZEvxyyrbbSt4g9jc3EhbqYQXlbtdcjtwjIir3GCsRDkqJElKIK3LYp+LEhzz tSN+uyKedReEmpi29ETFV9cNizYc8UFGLtTSAIxgcqWf91lLoQKUFgmFiDozkQ9nBXptIb LjWHqYIUBuiCX7vEuXAZkGSfdVWuYdDm3o5kFLpF9QwOEvTDwBMRv4NSlpGluQ== DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=linutronix.de; s=2020e; t=1645461854; 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=FanYIpzuzO2OIQcmAbO6hr4IzLwYQWIb8ljFjp/SrAs=; b=9tF2cGidY7GBkRFBiGZEcrVfwFXWtCNGIFcUkRa6oYaw4qWA9vVi5q3/DKgNjJtrllkuIh lu9RvHESpOVbA/BA== From: Sebastian Andrzej Siewior To: Michal Hocko Cc: cgroups@vger.kernel.org, linux-mm@kvack.org, Andrew Morton , Johannes Weiner , Michal =?utf-8?Q?Koutn=C3=BD?= , Peter Zijlstra , Thomas Gleixner , Vladimir Davydov , Waiman Long , kernel test robot Subject: Re: [PATCH v3 5/5] mm/memcg: Protect memcg_stock with a local_lock_t Message-ID: References: <20220217094802.3644569-1-bigeasy@linutronix.de> <20220217094802.3644569-6-bigeasy@linutronix.de> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: X-Rspamd-Queue-Id: 13DFE4000A X-Stat-Signature: yufq8z1u3rr9ye3cha6gcjgqkjoz83jk Authentication-Results: imf12.hostedemail.com; dkim=pass header.d=linutronix.de header.s=2020 header.b="nEVg+O/Z"; dkim=pass header.d=linutronix.de header.s=2020e header.b=9tF2cGid; dmarc=pass (policy=none) header.from=linutronix.de; spf=pass (imf12.hostedemail.com: domain of bigeasy@linutronix.de designates 193.142.43.55 as permitted sender) smtp.mailfrom=bigeasy@linutronix.de X-Rspam-User: X-Rspamd-Server: rspam11 X-HE-Tag: 1645461856-412530 X-Bogosity: Ham, tests=bogofilter, spamicity=0.000010, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On 2022-02-21 17:24:41 [+0100], Michal Hocko wrote: > > > > @@ -2282,14 +2288,9 @@ static void drain_all_stock(struct mem_cgroup *root_memcg) > > > > rcu_read_unlock(); > > > > > > > > if (flush && > > > > - !test_and_set_bit(FLUSHING_CACHED_CHARGE, &stock->flags)) { > > > > - if (cpu == curcpu) > > > > - drain_local_stock(&stock->work); > > > > - else > > > > - schedule_work_on(cpu, &stock->work); > > > > - } > > > > + !test_and_set_bit(FLUSHING_CACHED_CHARGE, &stock->flags)) > > > > + schedule_work_on(cpu, &stock->work); > > > > > > Maybe I am missing but on !PREEMPT kernels there is nothing really > > > guaranteeing that the worker runs so there should be cond_resched after > > > the mutex is unlocked. I do not think we want to rely on callers to be > > > aware of this subtlety. > > > > There is no guarantee on PREEMPT kernels, too. The worker will be made > > running and will be put on the CPU when the scheduler sees it fit and > > there could be other worker which take precedence (queued earlier). > > But I was not aware that the worker _needs_ to run before we return. > > A lack of draining will not be a correctness problem (sorry I should > have made that clear). It is more about subtlety than anything. E.g. the > charging path could be forced to memory reclaim because of the cached > charges which are still waiting for their draining. Not really something > to lose sleep over from the runtime perspective. I was just wondering > that this makes things more complex than necessary. So it is no strictly wrong but it would be better if we could do drain_local_stock() on the local CPU. > > We > > might get migrated after put_cpu() so I wasn't aware that this is > > important. Should we attempt best effort and wait for the worker on the > > current CPU? > > > > > An alternative would be to split out __drain_local_stock which doesn't > > > do local_lock. > > > > but isn't the section in drain_local_stock() unprotected then? > > local_lock instead of {get,put}_cpu would handle that right? It took a while, but it clicked :) If we acquire the lock_lock_t, that we would otherwise acquire in drain_local_stock(), before the for_each_cpu loop (as you say get,pu_cpu) then we would indeed need __drain_local_stock() and things would work. But it looks like an abuse of the lock to avoid CPU migration since there is no need to have it acquired at this point. Also the whole section would run with disabled interrupts and there is no need for it. What about if replace get_cpu() with migrate_disable()? Sebastian