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 E5731C46CA1 for ; Mon, 18 Sep 2023 18:56:44 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 6EDAB6B0435; Mon, 18 Sep 2023 14:56:44 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 69D436B0437; Mon, 18 Sep 2023 14:56:44 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 58D516B0438; Mon, 18 Sep 2023 14:56:44 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0017.hostedemail.com [216.40.44.17]) by kanga.kvack.org (Postfix) with ESMTP id 4884D6B0435 for ; Mon, 18 Sep 2023 14:56:44 -0400 (EDT) Received: from smtpin19.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay10.hostedemail.com (Postfix) with ESMTP id 07E2AC0BBC for ; Mon, 18 Sep 2023 18:56:44 +0000 (UTC) X-FDA: 81250624728.19.889F1A6 Received: from mail-pf1-f176.google.com (mail-pf1-f176.google.com [209.85.210.176]) by imf26.hostedemail.com (Postfix) with ESMTP id 1560A140023 for ; Mon, 18 Sep 2023 18:56:41 +0000 (UTC) Authentication-Results: imf26.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=Cjz9jDqc; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf26.hostedemail.com: domain of yury.norov@gmail.com designates 209.85.210.176 as permitted sender) smtp.mailfrom=yury.norov@gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1695063402; 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: in-reply-to:in-reply-to:references:references:dkim-signature; bh=RIS1GFpqDB1c/94phXmILGCKn2z3UAnormHxhEj0fhM=; b=myUoLjBCBApq+9vh/4VJX2zK37AdjpG5Mt8txhkHpUMpfFKVPLPTurfQHDsQYssvRlE9+h jIA47XvSeoaYF7MlmK6yeWjL/AhGWjDHkVB4vCPjScIz1GOfG/4qcsXXIyQYNEtHBUr+by ku/tQi7rnJGrkn0+Ht/i4uUwdSORAJY= ARC-Authentication-Results: i=1; imf26.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=Cjz9jDqc; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf26.hostedemail.com: domain of yury.norov@gmail.com designates 209.85.210.176 as permitted sender) smtp.mailfrom=yury.norov@gmail.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1695063402; a=rsa-sha256; cv=none; b=cokfaYmb2xXjIPNuWtKre5YjoBTyi6rMTYh0zbUbkHe/+/hrGv9Uw+Q4i+8Qw0h2qEFHQ/ La7xV4bwPn3u2FZR8rVXzH7PZaHuY8AdjJ+j9N/Oq7lHDwUHiLCvWyHjETL9nKjGLgAiH4 czuFygT5VHfw0ja+phiINQZmOkDVh7s= Received: by mail-pf1-f176.google.com with SMTP id d2e1a72fcca58-690b7cb71aeso437271b3a.0 for ; Mon, 18 Sep 2023 11:56:41 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1695063401; x=1695668201; darn=kvack.org; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=RIS1GFpqDB1c/94phXmILGCKn2z3UAnormHxhEj0fhM=; b=Cjz9jDqcfSaMlOhzzS6W5PlZKgGijZ4GRaxdxVVzOmM9CkQCL76jdlpPKeYerEKBgq DOcrtCOtrr9YKk2f26HkX2UuOOt7tRDjcgqOdVbiP8rQ5dzu4/cAibEruoymKrIpiA+c WbW0lu+MHcUKQFlFaDTB18qevFplwj7jLOupDGrfmcRf6I4k1JfW3X12PaCKZ+LyDqi5 9tFG3XtSIu2WLwjc9izJJmX1Aa5uAWjkra1waTEuWYySqBaFJFzgd/PTn2XUsy5b/Ug2 nvz42zchEZQTYFs7NmIA6qaHywVRttDc++Et13Q+5mK9+XSRCI1vuk7liyt5peFOFppR EygA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1695063401; x=1695668201; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=RIS1GFpqDB1c/94phXmILGCKn2z3UAnormHxhEj0fhM=; b=NO/wYmkxEJDOwfLRtIldbDiWxeyM1pc3Aenq3M2Rrh3jKuPZWLO+zgusDHSvPj0GOY o39Xm0YgCRgLob+PC9QmENJANxzgdBn7lJd8bv+w154Dh39RyN7JIHO0Smap5gJKYbZP yJTGpdnf6jMb65Pc/6ImNQmfe4B93up8nLzx05jaJcC+ObVRhcrewa7z06bUyMqvdT9r sonp1p67JRuplwga7MqgELvTANK81TdE3HiFSUp62YElqvoui/hgyXGQkW7lvvghIAk4 XK3cZ/UI61NOH6V/JpnyQ+DRMNVHsugco6+xICn1LDwwOPuYAc3JJXgin5pX7NekGibN uH3g== X-Gm-Message-State: AOJu0Yz9yZ65VxZyeYMg5m4eHHsVDUe3T01vuPCh7wUGTt1UR23x6mBy XYIWD7HVHAc7P1YXbal1LfQ= X-Google-Smtp-Source: AGHT+IERmhl0QL4j7dG4JO6RXuiu5Kh432AbWN83ydAdAa9lkQ9nKzGmWkFwx7AatWzvyDE0rjGeTg== X-Received: by 2002:a05:6a00:309e:b0:68f:b769:9182 with SMTP id bh30-20020a056a00309e00b0068fb7699182mr483932pfb.9.1695063400803; Mon, 18 Sep 2023 11:56:40 -0700 (PDT) Received: from localhost ([216.228.127.130]) by smtp.gmail.com with ESMTPSA id y3-20020aa78043000000b0068c61848785sm7414772pfm.208.2023.09.18.11.56.38 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 18 Sep 2023 11:56:40 -0700 (PDT) Date: Mon, 18 Sep 2023 11:56:36 -0700 From: Yury Norov To: Mirsad Todorovac Cc: Jan Kara , Philipp Stanner , linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org, Matthew Wilcox , Chris Mason , Andrew Morton , Josef Bacik , David Sterba , linux-btrfs@vger.kernel.org, linux-mm@kvack.org Subject: Re: [PATCH v1 1/1] xarray: fix the data-race in xas_find_chunk() by using READ_ONCE() Message-ID: References: <20230918044739.29782-1-mirsad.todorovac@alu.unizg.hr> <20230918094116.2mgquyxhnxcawxfu@quack3> <22ca3ad4-42ef-43bc-51d0-78aaf274977b@alu.unizg.hr> <20230918113840.h3mmnuyer44e5bc5@quack3> <20230918155403.ylhfdbscgw6yek6p@quack3> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Rspamd-Queue-Id: 1560A140023 X-Rspam-User: X-Rspamd-Server: rspam05 X-Stat-Signature: a1bbnkua7ss3ffmsf7grsxdmojt3hcds X-HE-Tag: 1695063401-122150 X-HE-Meta: U2FsdGVkX1+FWxHUqSOrAjIm5h10cKoWaJckQ9QOEHftmGqFuSsuXnRUatNBZwW9EVHlNNihnSBhqznaCJ9ZM9PFVUlFaL9It0KF1E0ROgw1RHVNHYILf2IBb6Q4QVeqIy0MGOHGOrqdXUe0PUPeBlYg4zSDzXGJ96NEO3OTfea4XVbWNqxKQSBbLbmGGjB79r2nKuVzOKdb7x4f5A53pmRhPP3tmByQbJpJLSTvQ/iOd2IDUijPTl52Fm8bozzlNqswutTXuATPRqHvBej6MtkcOl3TVn0KhfmBJx8uVgy3yNLDEy6nYuLXpVAquuk+jfRtplWXNIuqPNd78DSUKkYtsBQENb4CRHEnkdvNMHJ4nFULVBDcwHJvE8V/cMZzPmNDlm/JHC0hLlkGsyHXiLjS6tdDY7brrrjWVtoQxTU1b8rf2Snge6JX+IvXM+L71SIJ8aqlQpV8FBrbniC4J5/iRaStV6vacbMPqmfUCJgO6f672Gn4V67SjtLYKDrtc413bLDkLxmk92ZaFjMLZghVJ/oPQqEu3Yh50jspSuDXjoaYbXO2/DthtFEfD/SyIIwa7veTw/ETugfj2RKPo8BfQL05SbtO0KfPOlCI3ab0Jelj93Fltkl5qwXIEAIbgmcMI2HHWcre5NwSTFVR895dYbTieiWw7kwWFf1Www4ARpZp5+QhgVFHhM9QajAd/wRHxlO7BZJA9mCaw396rhbHYPQXpZ3kZIWaop+WcGqSMHvoo6Vx+cMJ836/fcsQo0+HwV5Vims4rKL6lDyiomoQPdZAwEVi+GDK4AeMJRybz+pd85y6LPwU8+v3KARxTdy2fERBB4yfdcUmu71GP9rBeDhFi1AnB14J8i15wukGiCH2+xLaB7o2Vu676ReZPWXJN/GbcnQJ7PCtE4+NlkNGIC451G7UW/Ejy36CgwJa2LOJnRKKaNdL1PovhbgeMWql/7h9jPmVF0PbDXy VIeqmgR+ 9k3GeSCjcT3kZbjJUG5w1AGQQzc2qv5c4j5jOWHfQ7badx2bY9we5WINRIJtbUjrxFqlHzmArabCnAR8tcwFnvihb6iWw4vE3N0YvfyMdaqfkrPP0YDOw2SwM3ITLWDINNal8gSyIV+Fw1x4ym2slDHgpqoM96ieKg3KuzBTwU9xScEdhzFrwnhFJTMI8oF1SynFdZYZnSA93R8om4K8AgsBWFFP4fn592GJ4h6YE7n80+xjheBqaBO7O4M1HvXRxkAq6Wakm0guUBFEhrzXs+w/EnwcEHDEgCeYKmL5+dZRCsfTn/8c8wf6X1eJSJoTyCuqKt1ITFwbDKBTl6Fv6ZgGI91DJqnBsc+gWzwqq2SAJqchJBqn67s4C7BHvpvZXD3tSSDUt/Ui3Azf2XJrF55Y08UgfaVuRTk0lqKlisfptqyQwAyqcIy3zsB8v44dXB+TzPGATldDGr6IeYhg65B5P/R9GddIPBIsLQgY7Xk2y5FSWMTZaOEQr1TREtIUjt1VowqLM09wi2K9nMSb+6I3uO+YCxysMBsTMJAh/5Wq/60o= 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 Mon, Sep 18, 2023 at 06:28:07PM +0200, Mirsad Todorovac wrote: > > > On 9/18/23 17:54, Jan Kara wrote: > > On Mon 18-09-23 07:59:03, Yury Norov wrote: > > > On Mon, Sep 18, 2023 at 02:46:02PM +0200, Mirsad Todorovac wrote: > > > > -------------------------------------------------------- > > > > lib/find_bit.c | 33 +++++++++++++++++---------------- > > > > 1 file changed, 17 insertions(+), 16 deletions(-) > > > > > > > > diff --git a/lib/find_bit.c b/lib/find_bit.c > > > > index 32f99e9a670e..56244e4f744e 100644 > > > > --- a/lib/find_bit.c > > > > +++ b/lib/find_bit.c > > > > @@ -18,6 +18,7 @@ > > > > #include > > > > #include > > > > #include > > > > +#include > > > > /* > > > > * Common helper for find_bit() function family > > > > @@ -98,7 +99,7 @@ out: \ > > > > */ > > > > unsigned long _find_first_bit(const unsigned long *addr, unsigned long size) > > > > { > > > > - return FIND_FIRST_BIT(addr[idx], /* nop */, size); > > > > + return FIND_FIRST_BIT(READ_ONCE(addr[idx]), /* nop */, size); > > > > } > > > > EXPORT_SYMBOL(_find_first_bit); > > > > #endif > > > > > > ... > > > > > > That doesn't look correct. READ_ONCE() implies that there's another > > > thread modifying the bitmap concurrently. This is not the true for > > > vast majority of bitmap API users, and I expect that forcing > > > READ_ONCE() would affect performance for them. > > > > > > Bitmap functions, with a few rare exceptions like set_bit(), are not > > > thread-safe and require users to perform locking/synchronization where > > > needed. > > > > Well, for xarray the write side is synchronized with a spinlock but the read > > side is not (only RCU protected). > > > > > If you really need READ_ONCE, I think it's better to implement a new > > > flavor of the function(s) separately, like: > > > find_first_bit_read_once() > > > > So yes, xarray really needs READ_ONCE(). And I don't think READ_ONCE() > > imposes any real perfomance overhead in this particular case because for > > any sane compiler the generated assembly with & without READ_ONCE() will be > > exactly the same. For example I've checked disassembly of _find_next_bit() > > using READ_ONCE(). The main loop is: > > > > 0xffffffff815a2b6d <+77>: inc %r8 > > 0xffffffff815a2b70 <+80>: add $0x8,%rdx > > 0xffffffff815a2b74 <+84>: mov %r8,%rcx > > 0xffffffff815a2b77 <+87>: shl $0x6,%rcx > > 0xffffffff815a2b7b <+91>: cmp %rcx,%rax > > 0xffffffff815a2b7e <+94>: jbe 0xffffffff815a2b9b <_find_next_bit+123> > > 0xffffffff815a2b80 <+96>: mov (%rdx),%rcx > > 0xffffffff815a2b83 <+99>: test %rcx,%rcx > > 0xffffffff815a2b86 <+102>: je 0xffffffff815a2b6d <_find_next_bit+77> > > 0xffffffff815a2b88 <+104>: shl $0x6,%r8 > > 0xffffffff815a2b8c <+108>: tzcnt %rcx,%rcx > > > > So you can see the value we work with is copied from the address (rdx) into > > a register (rcx) and the test and __ffs() happens on a register value and > > thus READ_ONCE() has no practical effect. It just prevents the compiler > > from doing some stupid de-optimization. > > > > Honza > > If I may also add, centralised READ_ONCE() version had fixed a couple of hundred of > the instances of KCSAN data-races in dmesg. > > _find_*_bit() functions and/or macros cause quite a number of KCSAN BUG warnings: > > 95 _find_first_and_bit (lib/find_bit.c:114 (discriminator 10)) > 31 _find_first_zero_bit (lib/find_bit.c:125 (discriminator 10)) > 173 _find_next_and_bit (lib/find_bit.c:171 (discriminator 2)) > 655 _find_next_bit (lib/find_bit.c:133 (discriminator 2)) > 5 _find_next_zero_bit > > Finding each one find_bit_*() function and replacing it with find_bit_*_read_once() > could be time-consuming and challenging. > > However, I will do both versions so you could compare, if you'd like. > > Note, in the PoC version I have only implemented find_next_bit_read_once() ATM to see if > this works. > > Regards, > Mirsad Guys, I lost the track of the conversation. In the other email Mirsad said: Which was the basic reason in the first place for all this, because something changed data from underneath our fingers .. It sounds clearly to me that this is a bug in xarray, *revealed* by find_next_bit() function. But later in discussion you're trying to 'fix' find_*_bit(), like if find_bit() corrupted the bitmap, but it's not. In previous email Jan said: for any sane compiler the generated assembly with & without READ_ONCE() will be exactly the same. If the code generated with and without READ_ONCE() is the same, the behavior would be the same, right? If you see the difference, the code should differ. You say that READ_ONCE() in find_bit() 'fixes' 200 KCSAN BUG warnings. To me it sounds like hiding the problems instead of fixing. If there's a race between writing and reading bitmaps, it should be fixed properly by adding an appropriate serialization mechanism. Shutting Kcsan up with READ_ONCE() here and there is exactly the opposite path to the right direction. Every READ_ONCE must be paired with WRITE_ONCE, just like atomic reads/writes or spin locks/unlocks. Having that in mind, adding READ_ONCE() in find_bit() requires adding it to every bitmap function out there. And this is, as I said before, would be an overhead for most users.