Re: [PATCH 2/5] rust: sync: add READ_ONCE and WRITE_ONCE

[Andreas Hindborg <a.hindborg@kernel.org>]

"Alice Ryhl" <aliceryhl@google.com> writes:

> There are currently a few places in the kernel where we use volatile
> reads when we really should be using `READ_ONCE`. To make it possible to
> replace these with proper `READ_ONCE` calls, introduce a Rust version of
> `READ_ONCE`.
>
> I've written the code to use Rust's volatile ops directly when possible.
> This results in a small amount of code duplication, but I think it makes
> sense for READ_ONCE and WRITE_ONCE to be implemented in pure Rust when
> possible. Otherwise they would unconditionally be a function call unless
> you have a system where you can perform cross-language inlining.
>
> I considered these functions in the bindings crate instead of kernel
> crate. I actually think it would make a lot of sense. But it implies
> some annoying complications on old compilers since the #![feature()]
> invocations in kernel/lib.rs do not apply in the bindings crate.
>
> For now, we do not support using READ_ONCE on compound types even if
> they have the right size. This can be added later.
>
> This fails checkpatch due to a misordered MAINTAINERS entry, but this is
> a pre-existing problem.
>
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> ---
>  MAINTAINERS                |   2 +
>  rust/helpers/helpers.c     |   1 +
>  rust/helpers/rwonce.c      |  34 ++++++++
>  rust/kernel/sync.rs        |   2 +
>  rust/kernel/sync/rwonce.rs | 188 +++++++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 227 insertions(+)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 12f49de7fe036c2439c00f9f4c67b2219d72a4c3..1d0cae158fe2cc7d99b6a64c11176b635e2d14e4 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -4117,9 +4117,11 @@ F:	arch/*/include/asm/atomic*.h
>  F:	include/*/atomic*.h
>  F:	include/linux/refcount.h
>  F:	scripts/atomic/
> +F:	rust/helpers/rwonce.c
>  F:	rust/kernel/sync/atomic.rs
>  F:	rust/kernel/sync/atomic/
>  F:	rust/kernel/sync/refcount.rs
> +F:	rust/kernel/sync/rwonce.rs
>
>  ATTO EXPRESSSAS SAS/SATA RAID SCSI DRIVER
>  M:	Bradley Grove <linuxdrivers@attotech.com>
> diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
> index 79c72762ad9c4b473971e6210c9577860d2e2b08..28b79ca7844fb744e5ad128238824921c055ec82 100644
> --- a/rust/helpers/helpers.c
> +++ b/rust/helpers/helpers.c
> @@ -48,6 +48,7 @@
>  #include "rcu.c"
>  #include "refcount.c"
>  #include "regulator.c"
> +#include "rwonce.c"
>  #include "scatterlist.c"
>  #include "security.c"
>  #include "signal.c"
> diff --git a/rust/helpers/rwonce.c b/rust/helpers/rwonce.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..55c621678cd632e728cb925b6a4a2e34e2fc4884
> --- /dev/null
> +++ b/rust/helpers/rwonce.c
> @@ -0,0 +1,34 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +/*
> + * Copyright (C) 2025 Google LLC.
> + */
> +
> +#ifdef CONFIG_ARCH_USE_CUSTOM_READ_ONCE
> +
> +__rust_helper u8 rust_helper_read_once_1(const u8 *ptr)
> +{
> +	return READ_ONCE(*ptr);
> +}
> +
> +__rust_helper u16 rust_helper_read_once_2(const u16 *ptr)
> +{
> +	return READ_ONCE(*ptr);
> +}
> +
> +__rust_helper u32 rust_helper_read_once_4(const u32 *ptr)
> +{
> +	return READ_ONCE(*ptr);
> +}
> +
> +__rust_helper u64 rust_helper_read_once_8(const u64 *ptr)
> +{
> +	return READ_ONCE(*ptr);
> +}
> +
> +__rust_helper void *rust_helper_read_once_ptr(void * const *ptr)
> +{
> +	return READ_ONCE(*ptr);
> +}
> +
> +#endif
> diff --git a/rust/kernel/sync.rs b/rust/kernel/sync.rs
> index 5df87e2bd212e192b8a67644bd99f05b9d4afd75..a5bf7bdc3fa8a044786eafae39fe8844aeeef057 100644
> --- a/rust/kernel/sync.rs
> +++ b/rust/kernel/sync.rs
> @@ -20,6 +20,7 @@
>  pub mod poll;
>  pub mod rcu;
>  mod refcount;
> +pub mod rwonce;
>  mod set_once;
>
>  pub use arc::{Arc, ArcBorrow, UniqueArc};
> @@ -30,6 +31,7 @@
>  pub use lock::spinlock::{new_spinlock, SpinLock, SpinLockGuard};
>  pub use locked_by::LockedBy;
>  pub use refcount::Refcount;
> +pub use rwonce::{READ_ONCE, WRITE_ONCE};
>  pub use set_once::SetOnce;
>
>  /// Represents a lockdep class. It's a wrapper around C's `lock_class_key`.
> diff --git a/rust/kernel/sync/rwonce.rs b/rust/kernel/sync/rwonce.rs
> new file mode 100644
> index 0000000000000000000000000000000000000000..a1660e43c9ef94011812d1816713cf031a73de1d
> --- /dev/null
> +++ b/rust/kernel/sync/rwonce.rs
> @@ -0,0 +1,188 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +// Copyright (C) 2025 Google LLC.
> +
> +//! Rust version of the raw `READ_ONCE`/`WRITE_ONCE` functions.
> +//!
> +//! C header: [`include/asm-generic/rwonce.h`](srctree/include/asm-generic/rwonce.h)
> +
> +/// Read the pointer once.
> +///
> +/// # Safety
> +///
> +/// It must be safe to `READ_ONCE` the `ptr` with this type.
> +#[inline(always)]
> +#[must_use]
> +#[track_caller]
> +#[expect(non_snake_case)]

I really do not think we need to have screaming snake case here. I
understand that this is what the macro looks like in C code, but we do
not need to carry that over.


Best regards,
Andreas Hindborg