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 8CD35C61DA3 for ; Tue, 21 Feb 2023 23:38:35 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 1A6766B0071; Tue, 21 Feb 2023 18:38:35 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 157386B0073; Tue, 21 Feb 2023 18:38:35 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id F13866B0074; Tue, 21 Feb 2023 18:38:34 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0011.hostedemail.com [216.40.44.11]) by kanga.kvack.org (Postfix) with ESMTP id DBC8E6B0071 for ; Tue, 21 Feb 2023 18:38:34 -0500 (EST) Received: from smtpin06.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay10.hostedemail.com (Postfix) with ESMTP id B0B3EC022A for ; Tue, 21 Feb 2023 23:38:34 +0000 (UTC) X-FDA: 80492915748.06.469529A Received: from sin.source.kernel.org (sin.source.kernel.org [145.40.73.55]) by imf02.hostedemail.com (Postfix) with ESMTP id 42F3580013 for ; Tue, 21 Feb 2023 23:38:30 +0000 (UTC) Authentication-Results: imf02.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=PXcWPlPj; spf=pass (imf02.hostedemail.com: domain of "SRS0=KnBo=6R=paulmck-ThinkPad-P17-Gen-1.home=paulmck@kernel.org" designates 145.40.73.55 as permitted sender) smtp.mailfrom="SRS0=KnBo=6R=paulmck-ThinkPad-P17-Gen-1.home=paulmck@kernel.org"; dmarc=pass (policy=none) header.from=kernel.org ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1677022712; h=from:from:sender:reply-to: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=8Zb5y8BpakA+TDx30M4rena+A+13vd10CKC3iUG8+VA=; b=d3EdwNhVqepfZ0aHxmNJ2baEzwpAl+yjJxhxalplOl9chcsyi5HarItHFzR+vnP6GyULYo 4R6+LwGxrmUrwrNW8opzK2ZTgt0Isu3xloALCRn+NHzaqC5boE0Oz0m6L6f4Jxb4aLwiyy 9UH16hMGRdu96J95ot41b6Kl8ZnW4Eg= ARC-Authentication-Results: i=1; imf02.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=PXcWPlPj; spf=pass (imf02.hostedemail.com: domain of "SRS0=KnBo=6R=paulmck-ThinkPad-P17-Gen-1.home=paulmck@kernel.org" designates 145.40.73.55 as permitted sender) smtp.mailfrom="SRS0=KnBo=6R=paulmck-ThinkPad-P17-Gen-1.home=paulmck@kernel.org"; dmarc=pass (policy=none) header.from=kernel.org ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1677022712; a=rsa-sha256; cv=none; b=IWGOevUKsWMzFsmjxqjGj4eQWd+fI4RxvN7B7DRCc7SviynLeWfqaKTbkeG4nSiNlM+1eN 0QQC5Aseez85w5flfhhXoBKS9Gd9/oSoxroSCHe9LQ9M/RABEMala4Saz9nYTTN1Hz5Adk g1Qqc3aWxDic1jX4HR6irOx937w256c= Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by sin.source.kernel.org (Postfix) with ESMTPS id DC968CE1C30; Tue, 21 Feb 2023 23:38:27 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 47446C4339B; Tue, 21 Feb 2023 23:38:25 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1677022705; bh=YC2cKPAIdiP72jz3kueKr+ivgSgpcT42R6071kn+pqk=; h=Date:From:To:Cc:Subject:Reply-To:References:In-Reply-To:From; b=PXcWPlPjCh1klB6LcXlRN66knyftqtpw2w/ZHa2HmJvDNuFjsMWNCTl5tHTxB27Hm ksSDOyf8f0femr7ntrXehQZ0u+8U7zE5CXPqMZ/H+S5/8HNC1eoGNAqEn9GL+KZdUX /JmBTFAqWWHQqKyYmN/COvP1RH6+bHao7R3Ge1dn8VPjPo9OH4i0ytVm8UWTRH0887 jV8CFZ/pPRatqpcvp8eLq3YTM2jaZWO6QRK/pXzpsgkYBwG1j3YqK/fLGo+Up6cQDd W2wdFKyDaPt0AzVQIPMd/U/kKzGsNibmedFSZRsabj7z+AmG+rOc2Leum5XbWKfPuy W0yyXJoynbOEQ== Received: by paulmck-ThinkPad-P17-Gen-1.home (Postfix, from userid 1000) id F22DD5C065D; Tue, 21 Feb 2023 15:38:24 -0800 (PST) Date: Tue, 21 Feb 2023 15:38:24 -0800 From: "Paul E. McKenney" To: Shakeel Butt Cc: Roman Gushchin , Matthew Wilcox , Marco Elver , Yue Zhao , linux-mm@kvack.org, akpm@linux-foundation.org, hannes@cmpxchg.org, mhocko@kernel.org, muchun.song@linux.dev, cgroups@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] mm: change memcg->oom_group access with atomic operations Message-ID: <20230221233824.GM2948950@paulmck-ThinkPad-P17-Gen-1> Reply-To: paulmck@kernel.org References: <20230220230624.lkobqeagycx7bi7p@google.com> <6563189C-7765-4FFA-A8F2-A5CC4860A1EF@linux.dev> <20230221182359.GJ2948950@paulmck-ThinkPad-P17-Gen-1> <20230221223811.GK2948950@paulmck-ThinkPad-P17-Gen-1> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: X-Rspamd-Server: rspam07 X-Rspamd-Queue-Id: 42F3580013 X-Rspam-User: X-Stat-Signature: i35itwpdukdjaucaq7eiyabcbqdu94mk X-HE-Tag: 1677022710-112570 X-HE-Meta: U2FsdGVkX18EXlnTJyEynlnmWSuFZArQxllyvfMRCtTp66qyXUTidGO9KTqxw5+OxuP6pkq9ZZnRKy1IVEvYTBqomWUOVdWtDL/Z5W264RQ/KjFkR0YYGsKRCdVZli5R1us7SK6AlfLWBfhbNJ/CwtIm5o2PLvFpJQK11ci2fOCeLs9oDk9nfCKcoL2TtfvU7OoZSbkQnx1po+7reSBOkI98VQufX3skA0/DpNJnKzcwnVANB7XYsKebULtlv/Gi4nVegZmgWmwxNtVj3qjK1YgirO9k/f0JoCU1b5ONlAFVZpq7sVRc/cFrZng2gwTEsdcraLA5Vq7CuGQRXojcTu03w6QYptxGyy/qu+HW+WIhvbefBwJxhahiB1UswygCvJggklAHlOlRnDP/nPRHlW4+ZsLXx1ZgDa8p0oVlviAOtfQXt+h0Q4jFs1YZT/TH4DXnZ951E1tZKvb1aiBGzRVyvFVfrbBIiW3j7tSWATwQ+8lJHcXg3d0dN95r96dVrXBWtWiGgt2IY2+OzHbBWZP4dysl1Nbmt7mtreukKlyw6gMcdd506ToUZQGkGLVU/IPfJRUX9m6ZFONOOPF1p4J60b1Z+5AXoZ6XVFmDVkvfK2BSy+zH+UP+BDpNvh55WpSiooO6f8pleBXuQsqoZbVFXPAA7s1N5G2FXjbczubHrUTOF4bX72UjZjAdArpmNCm8AokaZ9j9bS5nwHe4uAVCip1ubYbFrRXpXIMdHdxmDNfXUWh6dWZWeQVU6lLLoXwCtUTykgrB7a0j9Vk9aNjCllhco6egsZAw5vejPXrjlKL/U0Trmrf8N5zILX1IpdzAUa8QTzWXw0H0W7ni519vMKnxsGTOiFMlZbXFW9vSPG7h1+YPwv/JHrsfB/PDEdwDgQPU6z9ijxKCCx236/7OrUuQDhH4QLXTLd7Ed/odSEgrL/z5Lj+omGCvX0vriYF7CzHzBYpCuHKXV62 uBqYDmJU rXvSEXDIEZtnBvqzg1aPab/HyJXl0GIBIlrwXmGbzOdSPslqrOsAfsk18BO/svd/ek9/fTTRWMYDOIevbyAdmsX+GSY7mOGUC+/S66ZLHFpwU337FK9xh+Cp9MF6zSM78XDNVwBeDSgHa5isENTUuwuWaety308BQGXBfxuXj2RObn3vYYtfuPwUtnwBV7kVVc+iuI9dvCG9SdBSWfrRKajJfalqadUAH43KjLkkALN/qj0Hg+1JyrA9e1WgwJN1bXiP2/cos/AuBjK7+y0y3esRDXpeSpRcHeBQyTOzfHz+a+ErmAZeYeCYEe9/IwQNz9n9hG2r+uNznjHJVMGJyjajyZt1LHsVrzx0u8YW29LdGYp5EgL/ER9dWt7VuG7UxCVm+6Z/e+XhovnAcd8sMPzJ/RBY9rdzl6/SZa2ia+nOJOUCa23Zw4n1drRQq9NqGDvjcv75mnpNdr0c/H4Ge9zT4moNfSGu+0NERD5coIOB6xdLBxtTWPriggMQ550MbV6JT0svZ+/z9yyFhRdy6HzntzTLkjduFG+1vxdSxAgwhcFrwGsl1ue8t29gD+C0ZFnOFhY3DEwmRV9f2OLfSNwPEOwZeUw5upajKsrpf0OAsOdtwV67HaLBuFjDMGcVTQHNcVwxW2JExpYjtp+iSKYkxz9FwYnVxz2+yxIJgmPeAty/oU28lbenoN4081JW6RkWTu2Ox2uOZPLmXF9X+Y9YIbocJKgfW6uXbyzVSb48y29fabw+ET5Vtuqr3zLlygXuIhnH2+uMtDPMC9MAMvMBqvYCZHzYhnEBq17HWrEZP83NrAPG3mZqS7J3gwrHLdTaJsthM/R6A9FmO1qIkuzXdYgK9QoZ5kiTJfZRwDJvhqgLoIoz2RXYNKomgVeGsnk38epzasThBtooRHtj4U6c3kQ== 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 Tue, Feb 21, 2023 at 03:13:36PM -0800, Shakeel Butt wrote: > On Tue, Feb 21, 2023 at 2:38 PM Paul E. McKenney wrote: > > > > On Tue, Feb 21, 2023 at 02:23:31PM -0800, Roman Gushchin wrote: > > > On Tue, Feb 21, 2023 at 10:23:59AM -0800, Paul E. McKenney wrote: > > > > On Tue, Feb 21, 2023 at 08:56:59AM -0800, Shakeel Butt wrote: > > > > > +Paul & Marco > > > > > > > > > > On Tue, Feb 21, 2023 at 5:51 AM Matthew Wilcox wrote: > > > > > > > > > > > > On Mon, Feb 20, 2023 at 10:52:10PM -0800, Shakeel Butt wrote: > > > > > > > On Mon, Feb 20, 2023 at 9:17 PM Roman Gushchin wrote: > > > > > > > > > On Feb 20, 2023, at 3:06 PM, Shakeel Butt wrote: > > > > > > > > > > > > > > > > > > On Mon, Feb 20, 2023 at 01:09:44PM -0800, Roman Gushchin wrote: > > > > > > > > >>> On Mon, Feb 20, 2023 at 11:16:38PM +0800, Yue Zhao wrote: > > > > > > > > >>> The knob for cgroup v2 memory controller: memory.oom.group > > > > > > > > >>> will be read and written simultaneously by user space > > > > > > > > >>> programs, thus we'd better change memcg->oom_group access > > > > > > > > >>> with atomic operations to avoid concurrency problems. > > > > > > > > >>> > > > > > > > > >>> Signed-off-by: Yue Zhao > > > > > > > > >> > > > > > > > > >> Hi Yue! > > > > > > > > >> > > > > > > > > >> I'm curious, have any seen any real issues which your patch is solving? > > > > > > > > >> Can you, please, provide a bit more details. > > > > > > > > >> > > > > > > > > > > > > > > > > > > IMHO such details are not needed. oom_group is being accessed > > > > > > > > > concurrently and one of them can be a write access. At least > > > > > > > > > READ_ONCE/WRITE_ONCE is needed here. > > > > > > > > > > > > > > > > Needed for what? > > > > > > > > > > > > > > For this particular case, documenting such an access. Though I don't > > > > > > > think there are any architectures which may tear a one byte read/write > > > > > > > and merging/refetching is not an issue for this. > > > > > > > > > > > > Wouldn't a compiler be within its rights to implement a one byte store as: > > > > > > > > > > > > load-word > > > > > > modify-byte-in-word > > > > > > store-word > > > > > > > > > > > > and if this is a lockless store to a word which has an adjacent byte also > > > > > > being modified by another CPU, one of those CPUs can lose its store? > > > > > > And WRITE_ONCE would prevent the compiler from implementing the store > > > > > > in that way. > > > > > > > > > > Thanks Willy for pointing this out. If the compiler can really do this > > > > > then [READ|WRITE]_ONCE are required here. I always have big bad > > > > > compiler lwn article open in a tab. I couldn't map this transformation > > > > > to ones mentioned in that article. Do we have name of this one? > > > > > > > > No, recent compilers are absolutely forbidden from doing this sort of > > > > thing except under very special circumstances. > > > > > > > > Before C11, compilers could and in fact did do things like this. This is > > > > after all a great way to keep the CPU's vector unit from getting bored. > > > > Unfortunately for those who prize optimization above all else, doing > > > > this can introduce data races, for example: > > > > > > > > char a; > > > > char b; > > > > spin_lock la; > > > > spin_lock lb; > > > > > > > > void change_a(char new_a) > > > > { > > > > spin_lock(&la); > > > > a = new_a; > > > > spin_unlock(&la); > > > > } > > > > > > > > void change_b(char new_b) > > > > { > > > > spin_lock(&lb); > > > > b = new_b; > > > > spin_unlock(&lb); > > > > } > > > > > > > > If the compiler "optimized" that "a = new_a" so as to produce a non-atomic > > > > read-modify-write sequence, it would be introducing a data race. > > > > And since C11, the compiler is absolutely forbidden from introducing > > > > data races. So, again, no, the compiler cannot invent writes to > > > > variables. > > > > > > > > What are those very special circumstances? > > > > > > > > 1. The other variables were going to be written to anyway, and > > > > none of the writes was non-volatile and there was no ordering > > > > directive between any of those writes. > > > > > > > > 2. The other variables are dead, as in there are no subsequent > > > > reads from them anywhere in the program. Of course in that case, > > > > there is no need to read the prior values of those variables. > > > > > > > > 3. All accesses to all of the variables are visible to the compiler, > > > > and the compiler can prove that there are no concurrent accesses > > > > to any of them. For example, all of the variables are on-stack > > > > variables whose addresses are never taken. > > > > > > > > Does that help, or am I misunderstanding the question? > > > > > > Thank you, Paul! > > > > > > So it seems like READ_ONCE()/WRITE_ONCE() are totally useless here. > > > Or I still miss something? > > > > Yes, given that the compiler will already avoid inventing data-race-prone > > C-language accesses to shared variables, so if that was the only reason > > that you were using READ_ONCE() or WRITE_ONCE(), then READ_ONCE() and > > WRITE_ONCE() won't be helping you. > > > > Or perhaps better to put it a different way... The fact that the compiler > > is not permitted to invent data-racy reads and writes is exactly why > > you do not normally need READ_ONCE() and WRITE_ONCE() for accesses in > > lock-based critical sections. Instead, you only need READ_ONCE() and > > WRITE_ONCE() when you have lockless accesses to the same shared variables. > > This is lockless access to memcg->oom_group potentially from multiple > CPUs, so, READ_ONCE() and WRITE_ONCE() are needed, right? Agreed, lockless concurrent accesses should use READ_ONCE() and WRITE_ONCE(). And if either conflicting access is lockless, it is lockless. ;-) Thanx, Paul