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 65A7AC433EF for ; Mon, 21 Feb 2022 12:13:01 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id BF15C8D0002; Mon, 21 Feb 2022 07:13:00 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id BA15F8D0001; Mon, 21 Feb 2022 07:13:00 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id A68488D0002; Mon, 21 Feb 2022 07:13:00 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0031.hostedemail.com [216.40.44.31]) by kanga.kvack.org (Postfix) with ESMTP id 9687D8D0001 for ; Mon, 21 Feb 2022 07:13:00 -0500 (EST) Received: from smtpin28.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay01.hostedemail.com (Postfix) with ESMTP id 54882181CC1C9 for ; Mon, 21 Feb 2022 12:13:00 +0000 (UTC) X-FDA: 79166676120.28.AA736A5 Received: from galois.linutronix.de (Galois.linutronix.de [193.142.43.55]) by imf27.hostedemail.com (Postfix) with ESMTP id 961FA40007 for ; Mon, 21 Feb 2022 12:12:59 +0000 (UTC) Date: Mon, 21 Feb 2022 13:12:55 +0100 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linutronix.de; s=2020; t=1645445577; 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=Yd/Pv+4yxzI82Y3+KCMMwqZi8uHiedOULtnxyEsK9t8=; b=m3j5uZqP2GnBz9hR9+ahcnj5LxQ2aWpUFQWTc7iK8aShO6qeDylvW3GDy0Y/y1umG/5PwK VxOLm+MUozqKrjqM0G04ESo/lBfFZ0h+ZMjVk4m88VhPuM48tw+0TYVWYDKEsdRpzXgEl5 p5dzpEZ/O9sRbmkp+V1hQVvocMXOAUZL7/jpatstowYBwKVAMrwiANNR0YUbXLIqIxJwsW hYruaD48dcrYGwsXMmRo2GfzlU6sxZDRcNesCoaa2IA3cwHTNJrTRUlW7MA8NXa+WueGle zkN2DcDejQNJq3/tXQN9Ll9CxWj46szVpZ0p/Qf6Hiwma8poJcFCpCe1ZLEQvQ== DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=linutronix.de; s=2020e; t=1645445577; 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=Yd/Pv+4yxzI82Y3+KCMMwqZi8uHiedOULtnxyEsK9t8=; b=6+LQBW68DRyoQknMvgtM57egwQzQtY3fDE2Hj5+0FOFcN17uowYcI+ITQiy/OpYjcowS1T upjLeYDCV0zU35Dw== From: Sebastian Andrzej Siewior To: Shakeel Butt Cc: Cgroups , Linux MM , Andrew Morton , Johannes Weiner , Michal Hocko , Michal =?utf-8?Q?Koutn=C3=BD?= , Peter Zijlstra , Thomas Gleixner , Vladimir Davydov , Waiman Long , Roman Gushchin Subject: Re: [PATCH v3 3/5] mm/memcg: Protect per-CPU counter by disabling preemption on PREEMPT_RT where needed. Message-ID: References: <20220217094802.3644569-1-bigeasy@linutronix.de> <20220217094802.3644569-4-bigeasy@linutronix.de> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: X-Rspamd-Server: rspam04 X-Rspamd-Queue-Id: 961FA40007 X-Stat-Signature: iyip3jarhg1scsjkopky1cmauj3f9amp Authentication-Results: imf27.hostedemail.com; dkim=pass header.d=linutronix.de header.s=2020 header.b=m3j5uZqP; dkim=pass header.d=linutronix.de header.s=2020e header.b=6+LQBW68; spf=pass (imf27.hostedemail.com: domain of bigeasy@linutronix.de designates 193.142.43.55 as permitted sender) smtp.mailfrom=bigeasy@linutronix.de; dmarc=pass (policy=none) header.from=linutronix.de X-Rspam-User: X-HE-Tag: 1645445579-305282 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-02-21 12:31:18 [+0100], To Shakeel Butt wrote: > > The call chains from rmap.c have not really disabled irqs. Actually > > there is a comment in do_page_add_anon_rmap() "We use the irq-unsafe > > __{inc|mod}_zone_page_stat because these counters are not modified in > > interrupt context, and pte lock(a spinlock) is held, which implies > > preemption disabled". > > > > VM_BUG_ON(!irqs_disabled()) within memcg_stats_lock() would be giving > > false error reports for CONFIG_PREEMPT_NONE kernels. > > So three caller, including do_page_add_anon_rmap(): > __mod_lruvec_page_state() -> __mod_lruvec_state() -> __mod_memcg_lruvec_state() > > is affected. Here we get false warnings because interrupts may not be > disabled and it is intended. Hmmm. > The odd part is that this only affects certain idx so any kind of > additional debugging would need to take this into account. > What about memcg_rstat_updated()? It does: > > | x = __this_cpu_add_return(stats_updates, abs(val)); > | if (x > MEMCG_CHARGE_BATCH) { > | atomic_add(x / MEMCG_CHARGE_BATCH, &stats_flush_threshold); > | __this_cpu_write(stats_updates, 0); > | } > > The writes to stats_updates can happen from IRQ-context and with > disabled preemption only. So this is not good, right? So I made the following to avoid the wrong assert. Still not sure how bad the hunk above. diff --git a/mm/memcontrol.c b/mm/memcontrol.c index 97a88b63ee983..1bac4798b72ba 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -645,6 +645,13 @@ static void memcg_stats_lock(void) #endif } +static void __memcg_stats_lock(void) +{ +#ifdef CONFIG_PREEMPT_RT + preempt_disable(); +#endif +} + static void memcg_stats_unlock(void) { #ifdef CONFIG_PREEMPT_RT @@ -728,7 +735,20 @@ void __mod_memcg_lruvec_state(struct lruvec *lruvec, enum node_stat_item idx, pn = container_of(lruvec, struct mem_cgroup_per_node, lruvec); memcg = pn->memcg; - memcg_stats_lock(); + /* + * The caller from rmap relay on disabled preemption becase they never + * update their counter from in-interrupt context. For these two + * counters we check that the update is never performed from an + * interrupt context while other caller need to have disabled interrupt. + */ + __memcg_stats_lock(); + if (IS_ENABLED(CONFIG_DEBUG_VM)) { + if (idx == NR_ANON_MAPPED || idx == NR_FILE_MAPPED) + WARN_ON_ONCE(!in_task()); + else + WARN_ON_ONCE(!irqs_disabled()); + } + /* Update memcg */ __this_cpu_add(memcg->vmstats_percpu->state[idx], val); Sebastian