Date:   Thu, 2 Feb 2023 14:21:53 +0000
From:   Gary Guo <gary@garyguo.net>
To:     Peter Zijlstra <peterz@infradead.org>
Cc:     Boqun Feng <boqun.feng@gmail.com>, rust-for-linux@vger.kernel.org,
        linux-kernel@vger.kernel.org, Will Deacon <will@kernel.org>,
        Mark Rutland <mark.rutland@arm.com>,
        Miguel Ojeda <ojeda@kernel.org>,
        Alex Gaynor <alex.gaynor@gmail.com>,
        Wedson Almeida Filho <wedsonaf@gmail.com>,
        =?UTF-8?B?QmrDtnJu?= Roy Baron <bjorn3_gh@protonmail.com>,
        Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
Subject: Re: [RFC 2/5] rust: sync: Arc: Introduces ArcInner::count()
Message-ID: <20230202142153.352ba479.gary@garyguo.net>
In-Reply-To: <Y9t+3u+2UcAFQc+r@hirez.programming.kicks-ass.net>
References: <20230201232244.212908-1-boqun.feng@gmail.com>
        <20230201232244.212908-3-boqun.feng@gmail.com>
        <Y9t+3u+2UcAFQc+r@hirez.programming.kicks-ass.net>
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?AIdCuoPQ8bEIMHOdB5j+tJXIX/W63l6t8g/1KW07CZuNs0uUR6Ctw6mfok2a?=
 =?us-ascii?Q?tB5SusUFnkyK2oxDk8JQnEfHRniSAj2zD2iJW9EeE8o6J9cMaS7ArrkyK7vb?=
 =?us-ascii?Q?b2S4RyIAHvOCXnlJJ05Lhr27VwCYRQza+zqnDsRFIMwm/sWUL1Q45g4pZzO8?=
 =?us-ascii?Q?b5PE/Bx0w+SOYa2GPqhZaI/W51LlkgSFFtrRnojcE9e55ZeQlcyOvo2BYQ8Z?=
 =?us-ascii?Q?EISgJDckLY3AOC/FqcQ0SD0yCRbCIbkXso3/jR1gYveUYojTBX9ccG3VLNpk?=
 =?us-ascii?Q?b6d0GwnkqGI8q1qdjrwxM2vOPngYowbdekipUXAIX3tTYoKcfaVva3VNgivm?=
 =?us-ascii?Q?VjsoU1pQzcRgUhQ2vvD4ktvUaVDcjDrQ+Wiic6yzHSojBooOC+J/SckzjTvC?=
 =?us-ascii?Q?2E9fsK4MPLbMErkjllkk1QvBjpuC/cN1SaMZjXc56V9bmNZ9KcjzsrOOsRso?=
 =?us-ascii?Q?lJkxFwYh29qtzjQa+VbJyXaBjTlb80gO7xVmAGa1iift+ofKqC4seCiEIM3F?=
 =?us-ascii?Q?Cz81ZBEW15EgFP3zu0LhzpSUJ3CNGapvSqqApo66ofjhG5ImtTGJUeZ/Vy0g?=
 =?us-ascii?Q?kHA6jH5G4XVUHJ2E9VHJyCvCwMAtfmQHE0F6ABf11Mda39Ce/htWH8kXbnIy?=
 =?us-ascii?Q?rNC6WiRBnIl4Gbf0IcgzUGd6XfJt//7sVWU+V4ZOjIdXlwzziO5EUGeLhp+R?=
 =?us-ascii?Q?1JAB3+Qi3FnVFGB+ylbEsBzFV/svfgGfiiGX5sjdetjiBVur6vhwBlmapz7C?=
 =?us-ascii?Q?CXUzojFp7DonCI3mHvwQgFGou75T1/UxyNXl0PgKKpN3pe+qysPdDO1NIRcs?=
 =?us-ascii?Q?JaEMnATLIw1leQJ4qndEXV79h45euCOWoYlJ3bufal2eIYK/VrmZyKx/2U5D?=
 =?us-ascii?Q?ypTc1kCpfUMFnK/W7IVCimUUkr44tz5JBht0OyxwLUHUk839MamrsJx04m25?=
 =?us-ascii?Q?NhAwQtQ5QbrG8Msd03TlqyPB6Cn5yAYuzqlIh5VOVGx9xFHcoxKHQqmcU7D4?=
 =?us-ascii?Q?RCEqtd/TgTfXVGTssxFpFYUaq+OPf2zquGNvVj6FzG0MQrzIr6In7ejFQZ9X?=
 =?us-ascii?Q?dyHiiEQtBIGNDaLwSbD6CeSTi0K3IO+A7lseDpuI2kGADbAWbd40Tr5EEvIx?=
 =?us-ascii?Q?Hq7I7TVVXkQrLcUM+e2nReSImnzsmj2OW2uquBXccP6lNINqYbBA23aXfMHO?=
 =?us-ascii?Q?qY/yPD42zEeIFA0RVb6jjTyERqrixIspwurtoFQFukpUkA9HHOPOKZcRD/Pn?=
 =?us-ascii?Q?XQpnQ5R6YGfV4bU/W0kWR4F3QRK2KgOzIIlITYWMOZgYEwPxp8XnHX54wo4j?=
 =?us-ascii?Q?OlsA3LTwZw0G2YfZ6V1Mqp1aEtwBjFedVBx9r4vv9bS2Qsd9yp7wMZqnCM7k?=
 =?us-ascii?Q?m17U/93nCIKvhA9G+LLmgxg6+0p+rN+RLlWr40H6jonmMt63kxYeqGDC3vLn?=
 =?us-ascii?Q?iyx3EqLJKHke7s7b/QhCoHk4iwiUMPqRK/gNE5kw+umR1CjN0kmJ9mioJYsR?=
 =?us-ascii?Q?WbAfPKlhwk2iOlW5pa3+fFeeCE2kc0EXd6exQUCg2Aeo9FAoz+FjEudlb/6N?=
 =?us-ascii?Q?id0FcJ9g6CPGShgeZ8wb+JG6Wxzs70j8sj+lMdDT?=
X-OriginatorOrg: garyguo.net
X-MS-Exchange-CrossTenant-Network-Message-Id: 6490bbc5-de16-41eb-d9cf-08db0528d5f9
X-MS-Exchange-CrossTenant-AuthSource: LO2P265MB5183.GBRP265.PROD.OUTLOOK.COM
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 02 Feb 2023 14:21:55.0902
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: bbc898ad-b10f-4e10-8552-d9377b823d45
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 2DYTZPa2WTkBaRIp+Uw0JHQJf8Udr2qCKDYcI8Amxs7CCKeNcWHnNRlyMcxYsDcKo7i8Dp1xTSa9wwd0cp4K0Q==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CWLP265MB6514
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Thu, 2 Feb 2023 10:14:06 +0100
Peter Zijlstra <peterz@infradead.org> wrote:

> On Wed, Feb 01, 2023 at 03:22:41PM -0800, Boqun Feng wrote:
> > This allows reading the current count of a refcount in an `ArcInner`.
> > 
> > Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> > ---
> >  rust/helpers.c          | 6 ++++++
> >  rust/kernel/sync/arc.rs | 9 +++++++++
> >  2 files changed, 15 insertions(+)
> > 
> > diff --git a/rust/helpers.c b/rust/helpers.c
> > index 09a4d93f9d62..afc5f1a39fef 100644
> > --- a/rust/helpers.c
> > +++ b/rust/helpers.c
> > @@ -46,6 +46,12 @@ bool rust_helper_refcount_dec_and_test(refcount_t *r)
> >  }
> >  EXPORT_SYMBOL_GPL(rust_helper_refcount_dec_and_test);
> >  
> > +unsigned int rust_helper_refcount_read(refcount_t *r)
> > +{
> > +	return refcount_read(r);
> > +}
> > +EXPORT_SYMBOL_GPL(rust_helper_refcount_read);
> > +
> >  /*
> >   * We use `bindgen`'s `--size_t-is-usize` option to bind the C `size_t` type
> >   * as the Rust `usize` type, so we can use it in contexts where Rust
> > diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
> > index fc680a4a795c..fbfceaa3096e 100644
> > --- a/rust/kernel/sync/arc.rs
> > +++ b/rust/kernel/sync/arc.rs
> > @@ -127,6 +127,15 @@ struct ArcInner<T: ?Sized> {
> >      data: T,
> >  }
> >  
> > +impl<T: ?Sized> ArcInner<T> {
> > +    /// Returns the current reference count of [`ArcInner`].
> > +    fn count(&self) -> u32 {
> > +        // SAFETY: `self.refcount.get()` is always a valid pointer, and `refcount_read()` is a
> > +        // normal atomic read (i.e. no data race) only requiring on the address is valid.
> > +        unsafe { bindings::refcount_read(self.refcount.get()) }
> > +    }
> > +}  
> 
> This is completely unsafe vs concurrency. In order to enable correct
> tracing of refcount manipulations we have the __refcount_*(.oldp) API.

Retrieving the reference count is safe. It's just that in many
scenarios it's very hard to use the retrieved reference count
correctly, because it might be concurrently changed.

But there are correct ways to use a refcount, e.g. if you own
`Arc` and `.count()` returns 1, then you know that you are the
exclusive owner of the `Arc` and nobody else is going to touch it.
In this case we could get a `&mut` of the data (Rust standard library's
`Arc` provides such an API, although we don't have it yet).

The Rust standard library's `Arc` also expose a `strong_count`
function, with this doc:

```
Gets the number of strong (Arc) pointers to this allocation.

# Safety
This method by itself is safe, but using it correctly requires extra
care. Another thread can change the strong count at any time, including
potentially between calling this method and acting on the result.
```

Best,
Gary