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 D7D6EC433EF for ; Mon, 31 Jan 2022 15:06:36 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 4C4DD6B00AF; Mon, 31 Jan 2022 10:06:36 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 473A36B00B0; Mon, 31 Jan 2022 10:06:36 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 33B6B6B00B1; Mon, 31 Jan 2022 10:06:36 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0216.hostedemail.com [216.40.44.216]) by kanga.kvack.org (Postfix) with ESMTP id 21B146B00AF for ; Mon, 31 Jan 2022 10:06:36 -0500 (EST) Received: from smtpin29.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay04.hostedemail.com (Postfix) with ESMTP id CC9F432619 for ; Mon, 31 Jan 2022 15:06:35 +0000 (UTC) X-FDA: 79090908750.29.7C9FF53 Received: from galois.linutronix.de (Galois.linutronix.de [193.142.43.55]) by imf06.hostedemail.com (Postfix) with ESMTP id 9F2F5180015 for ; Mon, 31 Jan 2022 15:06:34 +0000 (UTC) Date: Mon, 31 Jan 2022 16:06:30 +0100 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linutronix.de; s=2020; t=1643641592; 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=w0NFiv/w/Cs5H22tLULVbOiZiCXLk9Q8Uppqq+mmVWE=; b=ITwPnNVNsBPallxs5zUMi9DOOpAhk6imEl29XzDT9MGobpamlZEVxC0uwSPXiwyoAqtOuN Hrwl3I5fISii+Y/OSbWmGCc6EQERKZdXnhx/vPX4nTQqUQk4qyit5lxFOo5iWifIhWj1ph WoTtowcpcyOyip8EKhw5RpLPnvAtJ1tjClZFVuHC9xg3yqt47DMJ+dI4GPCqXOlOkTiB5+ HP2Nt28QdqUkdNgDhBG/SV77OyQyT1jkxb6VmdKdZ/JMkNIXqV14SVHCfEDhSZcAbv3Xuf YohzS0CvFocKMugUENcXzjz+0oMSMD0gLABmWKAfDXO06HABuUjiymEQtKZHfg== DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=linutronix.de; s=2020e; t=1643641592; 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=w0NFiv/w/Cs5H22tLULVbOiZiCXLk9Q8Uppqq+mmVWE=; b=kDAzeZihk9torF7OFmhGe5sb/Dan/+Ie9U+8pJm4pG3edO3Y116sFbfMFGf++mfgG7JzSV x53bPvamwbn6AGCg== From: Sebastian Andrzej Siewior To: Vlastimil Babka Cc: cgroups@vger.kernel.org, linux-mm@kvack.org, Andrew Morton , Johannes Weiner , Michal Hocko , Michal =?utf-8?Q?Koutn=C3=BD?= , Peter Zijlstra , Thomas Gleixner , Vladimir Davydov , Waiman Long Subject: Re: [PATCH 3/4] mm/memcg: Add a local_lock_t for IRQ and TASK object. Message-ID: References: <20220125164337.2071854-1-bigeasy@linutronix.de> <20220125164337.2071854-4-bigeasy@linutronix.de> <7f4928b8-16e2-88b3-2688-1519a19653a9@suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <7f4928b8-16e2-88b3-2688-1519a19653a9@suse.cz> X-Rspamd-Server: rspam06 X-Rspamd-Queue-Id: 9F2F5180015 X-Stat-Signature: 1fm4uxfoxcz9g8j3n6potdm4km6u1eqj Authentication-Results: imf06.hostedemail.com; dkim=pass header.d=linutronix.de header.s=2020 header.b=ITwPnNVN; dkim=pass header.d=linutronix.de header.s=2020e header.b=kDAzeZih; dmarc=pass (policy=none) header.from=linutronix.de; spf=pass (imf06.hostedemail.com: domain of bigeasy@linutronix.de designates 193.142.43.55 as permitted sender) smtp.mailfrom=bigeasy@linutronix.de X-Rspam-User: nil X-HE-Tag: 1643641594-802234 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 2022-01-26 17:57:14 [+0100], Vlastimil Babka wrote: > > - drain_obj_stock() gets a memcg_stock_pcp passed if the stock_lock has been > > acquired (instead of the task_obj_lock) to avoid recursive locking later > > in refill_stock(). > > Looks like this was maybe true in some previous version but now > drain_obj_stock() gets a bool parameter that is passed to > obj_cgroup_uncharge_pages(). But drain_local_stock() uses a NULL or > stock_pcp for that bool parameter which is weird. buh. Sorry, that is a left over and should have been true/false instead. > > - drain_all_stock() disables preemption via get_cpu() and then invokes > > drain_local_stock() if it is the local CPU to avoid scheduling a worker > > (which invokes the same function). Disabling preemption here is > > problematic due to the sleeping locks in drain_local_stock(). > > This can be avoided by always scheduling a worker, even for the local > > CPU. Using cpus_read_lock() stabilizes cpu_online_mask which ensures > > that no worker is scheduled for an offline CPU. Since there is no > > flush_work(), it is still possible that a worker is invoked on the wrong > > CPU but it is okay since it operates always on the local-CPU data. > > > > - drain_local_stock() is always invoked as a worker so it can be optimized > > by removing in_task() (it is always true) and avoiding the "irq_save" > > variant because interrupts are always enabled here. Operating on > > task_obj first allows to acquire the lock_lock_t without lockdep > > complains. > > > > Signed-off-by: Sebastian Andrzej Siewior > > The problem is that this pattern where get_obj_stock() sets a > stock_lock_acquried bool and this is passed down and acted upon elsewhere, > is a well known massive red flag for Linus :/ > Maybe we should indeed just revert 559271146efc, as Michal noted there were > no hard numbers to justify it, and in previous discussion it seemed to > surface that the costs of irq disable/enable are not that bad on recent cpus > as assumed? I added some number, fell free re-run. Let me know if a revert is preferred or you want to keep that so that I can prepare the patches accordingly before posting. > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > > --- a/mm/memcontrol.c > > +++ b/mm/memcontrol.c > > @@ -260,8 +260,10 @@ bool mem_cgroup_kmem_disabled(void) > > return cgroup_memory_nokmem; > > } > > > > +struct memcg_stock_pcp; > > Seems this forward declaration is unused. you, thanks. Sebastian