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 753DDC25B78 for ; Tue, 28 May 2024 08:11:04 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id DFE626B008C; Tue, 28 May 2024 04:11:03 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id DAE946B0092; Tue, 28 May 2024 04:11:03 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id C9DBC6B0096; Tue, 28 May 2024 04:11:03 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0015.hostedemail.com [216.40.44.15]) by kanga.kvack.org (Postfix) with ESMTP id AE8286B008C for ; Tue, 28 May 2024 04:11:03 -0400 (EDT) Received: from smtpin28.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay04.hostedemail.com (Postfix) with ESMTP id D57701A03E6 for ; Tue, 28 May 2024 08:11:02 +0000 (UTC) X-FDA: 82167083964.28.5584FC6 Received: from sin.source.kernel.org (sin.source.kernel.org [145.40.73.55]) by imf17.hostedemail.com (Postfix) with ESMTP id 61CEF40021 for ; Tue, 28 May 2024 08:11:00 +0000 (UTC) Authentication-Results: imf17.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b="D/aObtLz"; dmarc=pass (policy=none) header.from=kernel.org; spf=pass (imf17.hostedemail.com: domain of vbabka@kernel.org designates 145.40.73.55 as permitted sender) smtp.mailfrom=vbabka@kernel.org ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1716883861; 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:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=JxZwzKhEvt/X+5nbC0B/X67sJLt87c6T8VkWqgipm4s=; b=vTJhCs0hOxQlW/3RFZmHzN21pebW+P6jb/n2+bqGg0Mg0VpJLwcCy31CmwpL9Ov4XTqUpD d99pttIhEjHF+rQoOQYgyYFZtkBEjM/EY5Qep3e61dMqR0ISLTUS2ky641oaQHGODC62D/ o5D7KAJJrU137wOhaeCFP7aqFL5ugIQ= ARC-Authentication-Results: i=1; imf17.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b="D/aObtLz"; dmarc=pass (policy=none) header.from=kernel.org; spf=pass (imf17.hostedemail.com: domain of vbabka@kernel.org designates 145.40.73.55 as permitted sender) smtp.mailfrom=vbabka@kernel.org ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1716883861; a=rsa-sha256; cv=none; b=bGYk3xg2Z58EtSK/0tkMGGYa2HY1bSFt+v8f/Vh6UtMDjrxs/oFp5t8LyGRPCf6FAK2ZsP SHTk/dvRJ/WIocC+QFP9fnUR1DNvpDPe2fpx76SGsAz0K+0q+/j+4H11K0j6nEGZpp103V /AzabQ/Vs/fNjh91BOkiIOn5qFi0FEQ= Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by sin.source.kernel.org (Postfix) with ESMTP id 63C4CCE0B26; Tue, 28 May 2024 08:10:56 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 8E49FC3277B; Tue, 28 May 2024 08:10:53 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1716883855; bh=3JWi8QLCnAiMD/3J4oBlHOEY+0eLHBNVPmIqYMw0BFM=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=D/aObtLzXz5XBzdB9HiGDNLSWr3F4gw8VeWwnYEqoD+wq0c6hA9VO+9e9Fb8rHYoE f2TtwvUDnt3wPiizZEMYux1tp74d2kSTWguRqMjQhb7aqkYzbmS0ApTWCw/D9Tn2B4 YsJo8Lb5+mrWLAM4f1Bsm0IVepQBpPbGRq7qaP2aXCgz8GWjxm/6qprnmuTeOYyxNQ NvM67O7uAY4rkXCm+UcGMjZzxETPnKHspSXMa3KT8Q0xOJe+46/yEby+OoYru9tO8u 4d/Nk2++LbMHUXtyZz12PZawAtyDLGqoAEG/83+LFw7tNZ9ozzqbu37VrJsnCInbHJ Gb/SW7kqRThKw== Message-ID: <69c0e008-ddde-4a38-b856-b765dc2b9745@kernel.org> Date: Tue, 28 May 2024 10:10:50 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v2] memcg: simple cleanup of stats update functions To: Sebastian Andrzej Siewior , Shakeel Butt Cc: Andrew Morton , Johannes Weiner , Michal Hocko , Roman Gushchin , Muchun Song , linux-mm@kvack.org, linux-kernel@vger.kernel.org, Thomas Gleixner References: <20240420232505.2768428-1-shakeel.butt@linux.dev> <20240527152200.P1rU7FaG@linutronix.de> <86006806-4ffc-4330-ab4b-29215ab2c98c@kernel.org> <20240528075623.oFcU1JLj@linutronix.de> Content-Language: en-US From: "Vlastimil Babka (SUSE)" In-Reply-To: <20240528075623.oFcU1JLj@linutronix.de> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Rspamd-Queue-Id: 61CEF40021 X-Stat-Signature: dr4cqzt7a357zuoo1oggayxwxxwnrkhh X-Rspam-User: X-Rspamd-Server: rspam11 X-HE-Tag: 1716883860-698971 X-HE-Meta: U2FsdGVkX18WdrI4IjrgRlRk5lY7+l2sK6gJkVYsGo+0E1Q+2GgRWBpj0CM2MBNLMzk+jE2nQpayOb2S7JAOTUsG9rp6t+dHi/D0Xd9+J4zraU/dOd2arWKdTvY1KFQFgY9Tvm60hk3gKMt/WvPPWqM6DHPaowyjX441GxYeWp5g/eb/nXknEtVolFNQflDUOVRRWLBmQTc99OtshwWQj2t1vSWE0eBFtLimGv9YzhFrD7QrR6phpSyOHXr6h4PgFc4WZy9vLIkqxpL4Pg2ZsK+JkmkIZJ+V2QZYkbJHp4A7roKtYOmxcVjyEnDfr6EYnLVEDo/GVZdusXvG0FThiFdAtuSgglR0+O5wXmHaXRFHpf0/5MlqqcaLhBHp4e6P0GpfsHMc9CXLeRz+2AjG1l4ZtnvvI7jUomd0T0jAQ6DQETiZxDnS/CjIj9mfIbgX14tTV+VxR0lfc143PCwYAUZ69rI6YBqCiOlnrXnuVRjJ0AKgElnUB0K7bW2N80kSGUwuEaezFoFkI4/K7oRCnglCTh1pA1wSIiKRupdOS4DgV/SF0uyZhiX+VcLzy5kQkowhL+iJb+T0feb5tNPw2YVnYtoe8puHEYDIs/37Az/5g8O3v05tMjJ+yDWNjdrz+edvz3hP7dcRI+nl1oYL14g9/ySYvuf/L/hzQwpFXf0Y+U4sNHr+i9jkbsO5Kaxf7iWxlE0S7Uvetcr0NjeAw9oKYpmUTgN5BqWms/ZVbvwpNESDK9oDRRbNyOTx1MCm6LZQI41OgvwFyPh02EVfWoRc58x6rwKdosTkNSQUsIxGyBXnKqI3TsiYff3tDY4BJinPK4G+pCcKNGTSHz4xqX3gpvqLwDOMnAzZ4TysaQXF38kdy9dVfkpOuZ/1fkdhBHXD8QpvLWl/Ex8ns9xaany5/z11tU0+qTOzOyI2tq8suq6MfZ295L1GkfCXf1sl8AjoToElW89kiXQuNjJ hXzK4mSh 1QiJ8tIGyb65lwpTbKcjyBAdjz5V3G7BPTz3HL46oOeKYK3kLBQzeofxsbP2N1Bkz7BrL3KqQPFdyId9bw+pMA3GtlJeMsoIXhHRLR/iGPEk4vmmo7+kT1PqWb8RH8QvETOj3w35FfCGBHz62k+WzieGfEgeVFsUv5kZAxTHo7Pwan4bqZ82GSJTBhODSsyV35PTEi3TIwOZ7bd1hzira0TlZd8ZnO4Dy6JH1wkMKibTj8VrRyFy+7Dc1/D0y2t0xTXR2 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 5/28/24 9:56 AM, Sebastian Andrzej Siewior wrote: > On 2024-05-27 22:16:41 [-0700], Shakeel Butt wrote: >> On Mon, May 27, 2024 at 06:34:24PM GMT, Vlastimil Babka (SUSE) wrote: >> > On 5/27/24 5:22 PM, Sebastian Andrzej Siewior wrote: >> > > On 2024-04-20 16:25:05 [-0700], Shakeel Butt wrote: >> > >> mod_memcg_lruvec_state() is never called from outside of memcontrol.c >> > >> and with always irq disabled. So, replace it with the irq disabled >> > >> version and add an assert that irq is disabled in the caller. >> > > >> > > unless PREEMPT_RT is enabled. In that case IRQs are not disabled as part >> > > of local_lock_irqsave(&memcg_stock.stock_lock, …) leading to: >> >> Sorry about that and thanks for the report. > > no worries. > >> > >> > But then the "interrupts are handled by a kernel thread that can sleep" part >> > of RT also means it's ok to just have the stock_lock taken with no >> > interrupts disabled as no actual raw interrupt handler will interrupt the >> > holder and deadlock, right? > > I *don't* know why the interrupts-disabled check is here. The > memcg_stock.stock_lock is acquired on RT with interrupts enabled and > never disables interrupts. The lock is never acquired in an hard > interrupt (not threaded interrupt) context so there is never a deadlock. > > Originally the interrupts were disabled in mod_memcg_lruvec_state() > because the counter, it operates on, is per-CPU and relies on disabled > interrupts because the operation is not atomic and the code can be run > in interrupts context (on !RT). The __mod_memcg_lruvec_state() variant > of it relied on interrupts being disabled by the caller. This "rely on" > was part of a spinlock_t lock (or invoked from an interrupt handler, the > memory is fading slowly away) which does not disable interrupts on > PREEMPT_RT. > So for that reason we ended up with __memcg_stats_lock() which disables > preemption only on PREEMPT_RT to achieve the same level of "atomic" > update. > >> Thanks Vlastimil for jolting my memory on RT reasoning. >> >> > > suggestions? >> > >> > So in that case the appropriate thing would be to replace the assert with >> > lockdep_assert_held(&memcg_stock.stock_lock); >> > ? >> > >> > It seems all the code paths leading here have that one. >> > >> >> Yeah this seems right and reasonable. Should I send a fix or you want to >> send it? > > I don't mind sending a patch. I'm just not sure if the lock is the right > thing to do. However it should ensure that interrupts are disabled on > !RT for the sake of the counter update (if observed in IRQ context). Looks like some places there use VM_WARN_ON_IRQS_ENABLED() that's turned off for PREEMPT_RT, so maybe that's what should replace the current lockdep_assert, perhaps together with lockdep_assert_held(this_cpu_ptr(&memcg_stock.stock_lock)); But also __mod_memcg_lruvec_state() already has that VM_WARN_ON. > Yeah, let me prepare something. > > Sebastian