Received: by 2002:ab2:7a09:0:b0:1f8:46dc:890e with SMTP id k9csp290000lqo; Wed, 15 May 2024 14:51:14 -0700 (PDT) X-Forwarded-Encrypted: i=3; AJvYcCWStYeVhmvFvkMSLhm5cdWPwMzGLWT2FdkjxQm6dyVEer/ms1xa6uROVr6RLzPZt2Eh2cqFo8AxhGAaBLnMnSE/MZ1jfnuAViZOEkRrKg== X-Google-Smtp-Source: AGHT+IER/eI9RwhovhygKe8gdM5uMoc5x3MCGNvcjtGDQBMLOcdzBQ8K7bKpPFTUN47kxxBx54Rg X-Received: by 2002:aca:2412:0:b0:3c9:6d44:8210 with SMTP id 5614622812f47-3c99704c46amr18771708b6e.17.1715809874673; Wed, 15 May 2024 14:51:14 -0700 (PDT) ARC-Seal: i=2; a=rsa-sha256; t=1715809874; cv=pass; d=google.com; s=arc-20160816; b=v9PTlv6rVYpTPEdAPkfmNYg3mImipSYFiIl27fH+0/siJoGQbOICUQjNLQH4FB9Tjd FPZLfHLkM1dcu6UGicBxp/MQBTf9bNusHS8T2ClRNl9YfNQ3F4ACfcJNOsMvNHzMIPP1 KVedMUH9mrCqHerEgUn1yziTv3/ecIMjjOOoTRUPxyS6NouCWb2UMQKIy5rWnsmiYYpQ Mfi7B+HVrrCkBVDm1IYrgCpQgK4e9pb7a2h1P+hgabeiCQkF72lbbq/YkTFqllL7CLws bbu5bXPBnPeHIaQMWLaGnvgjp7y1z91+qx1ogjwhLEp38I/xSdEGlkn13I9GRbP+jTbj DmPA== ARC-Message-Signature: i=2; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=in-reply-to:content-disposition:mime-version:list-unsubscribe :list-subscribe:list-id:precedence:references:reply-to:message-id :subject:cc:to:from:date:dkim-signature; bh=qmyftwU1zX0hWej2dFW3XY3G8U2UbWxim7ydmRF5VXg=; fh=geeHGWzaZWX1ulQSwzfD/QqY6q7T1geIMMpKSDUZUzQ=; b=zQGT7lbjIyDa5E2edUKdniF21Q7h84hrlKQMtBYUNOeI6AggFLybcoVgwQEBFBcjx3 lvDK4sgUfPc2kaxcHaTbr50hgA0UUJs7/HO4twuy6b0AS8RCF638I7EIKoVbTMePoNB5 pK8mKC7ciTQTjhcFBkmua8cX656BdYfHeYLJSZXHHByWyj5VyUQCBL5nr1cQ/pOnoeXi i/6rUNkT0oO35MBIMuj7a0PFn+CJyHfDQqc6gF8WLVa3YWCUNAGbskIlkZMcPTA5rZTc Yyr5CMbc1s9k7HfQ/Lyj0U767JZ7/ehN0P5R0rb+wrqa209+dOaKlTA+J+7EzegUxT3y u1jg==; dara=google.com ARC-Authentication-Results: i=2; mx.google.com; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=ot3QPVvo; arc=pass (i=1 dkim=pass dkdomain=kernel.org); spf=pass (google.com: domain of linux-kernel+bounces-180406-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) smtp.mailfrom="linux-kernel+bounces-180406-linux.lists.archive=gmail.com@vger.kernel.org"; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Return-Path: Received: from ny.mirrors.kernel.org (ny.mirrors.kernel.org. [147.75.199.223]) by mx.google.com with ESMTPS id af79cd13be357-792bf277ddfsi1470855885a.55.2024.05.15.14.51.14 for (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 15 May 2024 14:51:14 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel+bounces-180406-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223; Authentication-Results: mx.google.com; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=ot3QPVvo; arc=pass (i=1 dkim=pass dkdomain=kernel.org); spf=pass (google.com: domain of linux-kernel+bounces-180406-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) smtp.mailfrom="linux-kernel+bounces-180406-linux.lists.archive=gmail.com@vger.kernel.org"; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Received: from smtp.subspace.kernel.org (wormhole.subspace.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ny.mirrors.kernel.org (Postfix) with ESMTPS id 5612F1C22207 for ; Wed, 15 May 2024 21:51:14 +0000 (UTC) Received: from localhost.localdomain (localhost.localdomain [127.0.0.1]) by smtp.subspace.kernel.org (Postfix) with ESMTP id 8601215B56F; Wed, 15 May 2024 21:51:04 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="ot3QPVvo" Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 56E9F15B54A; Wed, 15 May 2024 21:51:03 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1715809863; cv=none; b=nQ1nm73TAUU7gxZmzO/Lafh64vECHEBMNqjtv4on8EX4306Tcknr6VcUs2q6Vfh+/ePzmc/Wo6Lgh/y1f9j3IFiJIsYSWadmwFkPaGAqmmsfP7yGq/lHeoVSIYtnyPhZdmfWYIip8kYuylGhKk26Y15knOaiGoZ1AqRGL5pNHuE= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1715809863; c=relaxed/simple; bh=0DXBzG5HSOGPu4fQRLxlcEQFxW8zkEG7odaxH8d2qII=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=ZcdtF2cxqJTn053n4ZtIfDTVXILlc434QdYee570GZRrwIfdWRbMF8pr78TjvZHo8ImUuhks6Vg3Gq0YZpoatXUF6vjEs3phkuHmV210zABtcmrYc3cCDJmbZ+aLZVOzA2OOm1E/XLai4hNeQPSVdh0oJbK9BiC9zzFiHC+ut/4= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=ot3QPVvo; arc=none smtp.client-ip=10.30.226.201 Received: by smtp.kernel.org (Postfix) with ESMTPSA id DAD2FC116B1; Wed, 15 May 2024 21:51:02 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1715809862; bh=0DXBzG5HSOGPu4fQRLxlcEQFxW8zkEG7odaxH8d2qII=; h=Date:From:To:Cc:Subject:Reply-To:References:In-Reply-To:From; b=ot3QPVvoXLBcRKW7d9jOH3R8j7cAacPPDYyldX5dqeswn6MW/we1wkXIo5cuzNMoi iutcCb2oFNiqpfdivDd/OTuUg8O57U/Z5PRbOtc8b8rTFuMUX9DPAbhesqs8bnYtqV R9rgNQ/qpjmRGdu1U5TQ/Ak8a1yV4tH5918yL1qYv3V9Hf4oPYERgXIR1bUvnH9oSh eizYQrM+siWNWZtQsSneCKw9APMtsYCLkUcPxjOLCzn5KuJC8+Xy4R7dR/mUGvgjA2 GWqhNuFuKN89NVJJMWtXhnisI0Py/npkiRSh1wOXKbde8+x5FdOVHG4eF80TxEtb37 WakY5VW+QmsTQ== Received: by paulmck-ThinkPad-P17-Gen-1.home (Postfix, from userid 1000) id 2F22ACE0DEC; Wed, 15 May 2024 14:51:02 -0700 (PDT) Date: Wed, 15 May 2024 14:51:02 -0700 From: "Paul E. McKenney" To: Marco Elver Cc: Bart Van Assche , Breno Leitao , Jens Axboe , "open list:BLOCK LAYER" , open list Subject: Re: [PATCH] block: Annotate a racy read in blk_do_io_stat() Message-ID: Reply-To: paulmck@kernel.org References: <4d230bac-bdb0-4a01-8006-e95156965aa8@acm.org> <447ad732-3ff8-40bf-bd82-f7be66899cee@paulmck-laptop> <59ec96c2-52ce-4da1-92c3-9fe38053cd3d@paulmck-laptop> <75421237-4c5a-48bc-849e-87a216ee9d32@paulmck-laptop> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: On Wed, May 15, 2024 at 07:40:08PM +0200, Marco Elver wrote: > On Wed, 15 May 2024 at 17:57, Paul E. McKenney wrote: > > On Wed, May 15, 2024 at 09:58:35AM +0200, Marco Elver wrote: > > > On Wed, 15 May 2024 at 01:47, Paul E. McKenney wrote: > > > > On Mon, May 13, 2024 at 10:13:49AM +0200, Marco Elver wrote: > > > > > On Sat, 11 May 2024 at 02:41, Paul E. McKenney wrote: > > > > > [...] > > > > > > ------------------------------------------------------------------------ > > > > > > > > > > > > commit 930cb5f711443d8044e88080ee21b0a5edda33bd > > > > > > Author: Paul E. McKenney > > > > > > Date: Fri May 10 15:36:57 2024 -0700 > > > > > > > > > > > > kcsan: Add example to data_race() kerneldoc header > > > > > > > > > > > > Although the data_race() kerneldoc header accurately states what it does, > > > > > > some of the implications and usage patterns are non-obvious. Therefore, > > > > > > add a brief locking example and also state how to have KCSAN ignore > > > > > > accesses while also preventing the compiler from folding, spindling, > > > > > > or otherwise mutilating the access. > > > > > > > > > > > > [ paulmck: Apply Bart Van Assche feedback. ] > > > > > > > > > > > > Reported-by: Bart Van Assche > > > > > > Signed-off-by: Paul E. McKenney > > > > > > Cc: Marco Elver > > > > > > Cc: Breno Leitao > > > > > > Cc: Jens Axboe > > > > > > > > > > > > diff --git a/include/linux/compiler.h b/include/linux/compiler.h > > > > > > index c00cc6c0878a1..9249768ec7a26 100644 > > > > > > --- a/include/linux/compiler.h > > > > > > +++ b/include/linux/compiler.h > > > > > > @@ -194,9 +194,17 @@ void ftrace_likely_update(struct ftrace_likely_data *f, int val, > > > > > > * This data_race() macro is useful for situations in which data races > > > > > > * should be forgiven. One example is diagnostic code that accesses > > > > > > * shared variables but is not a part of the core synchronization design. > > > > > > + * For example, if accesses to a given variable are protected by a lock, > > > > > > + * except for diagnostic code, then the accesses under the lock should > > > > > > + * be plain C-language accesses and those in the diagnostic code should > > > > > > + * use data_race(). This way, KCSAN will complain if buggy lockless > > > > > > + * accesses to that variable are introduced, even if the buggy accesses > > > > > > + * are protected by READ_ONCE() or WRITE_ONCE(). > > > > > > * > > > > > > - * This macro *does not* affect normal code generation, but is a hint > > > > > > - * to tooling that data races here are to be ignored. > > > > > > + * This macro *does not* affect normal code generation, but is a hint to > > > > > > + * tooling that data races here are to be ignored. If code generation must > > > > > > + * be protected *and* KCSAN should ignore the access, use both data_race() > > > > > > > > > > "code generation must be protected" seems ambiguous, because > > > > > protecting code generation could mean a lot of different things to > > > > > different people. > > > > > > > > > > The more precise thing would be to write that "If the access must be > > > > > atomic *and* KCSAN should ignore the access, use both ...". > > > > > > > > Good point, and I took your wording, thank you. > > > > > > > > > I've also had trouble in the past referring to "miscompilation" or > > > > > similar, because that also entirely depends on the promised vs. > > > > > expected semantics: if the code in question assumes for the access to > > > > > be atomic, the compiler compiling the code in a way that the access is > > > > > no longer atomic would be a "miscompilation". Although is it still a > > > > > "miscompilation" if the compiler generated code according to specified > > > > > language semantics (say according to our own LKMM) - and that's where > > > > > opinions can diverge because it depends on which side we stand > > > > > (compiler vs. our code). > > > > > > > > Agreed, use of words like "miscompilation" can annoy people. What > > > > word would you suggest using instead? > > > > > > Not sure. As suggested above, I try to just stick to "atomic" vs > > > "non-atomic" because that's ultimately the functional end result of > > > such a miscompilation. Although I've also had people be confused as in > > > "what atomic?! as in atomic RMW?!", but I don't know how to remove > > > that kind of confusion. > > > > > > If, however, our intended model is the LKMM and e.g. a compiler breaks > > > a dependency-chain, then we could talk about miscompilation, because > > > the compiler violates our desired language semantics. Of course the > > > compiler writers then will say that we try to do things that are > > > outside any implemented language semantics the compiler is aware of, > > > so it's not a miscompilation again. So it all depends on which side > > > we're arguing for. Fun times. > > > > ;-) ;-) ;-) > > > > > > > > + * and READ_ONCE(), for example, data_race(READ_ONCE(x)). > > > > > > > > > > Having more documentation sounds good to me, thanks for adding! > > > > > > > > > > This extra bit of documentation also exists in a longer form in > > > > > access-marking.txt, correct? I wonder how it would be possible to > > > > > refer to it, in case the reader wants to learn even more. > > > > > > > > Good point, especially given that I had forgotten about it. > > > > > > > > I don't have any immediate ideas for calling attention to this file, > > > > but would the following update be helpful? > > > > > > Mentioning __data_racy along with data_race() could be helpful, thank > > > you. See comments below. > > > > I did add a mention of it in "Linux-Kernel RCU Shared-Variable Marking" > > [1], but just a mention, given that I do not expect that we will use it > > within RCU. > > > > > Thanks, > > > -- Marco > > > > > > > Thanx, Paul > > > > > > > > ------------------------------------------------------------------------ > > > > > > > > diff --git a/tools/memory-model/Documentation/access-marking.txt b/tools/memory-model/Documentation/access-marking.txt > > > > index 65778222183e3..690dd59b7ac59 100644 > > > > --- a/tools/memory-model/Documentation/access-marking.txt > > > > +++ b/tools/memory-model/Documentation/access-marking.txt > > > > @@ -24,6 +24,12 @@ The Linux kernel provides the following access-marking options: > > > > 4. WRITE_ONCE(), for example, "WRITE_ONCE(a, b);" > > > > The various forms of atomic_set() also fit in here. > > > > > > > > +5. ASSERT_EXCLUSIVE_ACCESS() and ASSERT_EXCLUSIVE_WRITER(). > > > > > > Perhaps worth mentioning, but they aren't strictly access-marking > > > options. In the interest of simplicity could leave it out. > > > > Interestingly enough, they can be said to be implicitly marking other > > concurrent accesses to the variable. ;-) > > The document starts with "guidelines for marking intentionally > concurrent normal accesses to shared memory". The ASSERT_EXCLUSIVE > macros do capture more of the concurrency rules, and perhaps they > could be seen as some kind of "negative marking" where concurrent > access should _not_ happen concurrently with these. But I'm still not > convinced it's the same kind of marking the document introduces. > > I always considered them in the realm of general assertions that we > can just use to tell the tool more than can be inferred from the bits > of C code required for the functional implementation of whatever we're > doing. > > > I believe that the do need to be mentioned more prominently, though. > > > > Maybe a second list following this one? In that case, what do we name > > the list? I suppose the other alternative would be to leave them in > > this list, and change the preceding sentence to say something like > > "The Linux kernel provides the following access-marking-related primitives" > > > > Thoughts? > > And I just checked the current access-marking.txt to see where we > might add more, and found the section "ACCESS-DOCUMENTATION OPTIONS" > already exists. I think that section is perfectly reasonable as is, > and it does explicitly talk about ASSERT_EXCLUSIVE* macros. > > Did you want to add it more prominently at the top? If so, maybe a > brief forward-reference to that section might be helpful. How about like this? ------------------------------------------------------------------------ The Linux kernel provides the following access-marking options: 1. Plain C-language accesses (unmarked), for example, "a = b;" 2. Data-race marking, for example, "data_race(a = b);" 3. READ_ONCE(), for example, "a = READ_ONCE(b);" The various forms of atomic_read() also fit in here. 4. WRITE_ONCE(), for example, "WRITE_ONCE(a, b);" The various forms of atomic_set() also fit in here. 5. __data_racy, for example "int __data_racy a;" 6. KCSAN's negative-marking assertions, ASSERT_EXCLUSIVE_ACCESS() and ASSERT_EXCLUSIVE_WRITER(), are desccribed in the "ACCESS-DOCUMENTATION OPTIONS" section below. ------------------------------------------------------------------------ Would that work? Thanx, Paul