[PATCH RFC 0/3] rust: Add Per-CPU Variable API

[PATCH RFC 0/3] rust: Add Per-CPU Variable API

[Mitchell Levy]

This series adds an API for declaring an using per-CPU variables from
Rust, and it also adds support for Rust access to C per-CPU variables
(subject to some soundness requirements). It also adds a small test
module, lib/percpu_test_rust.rs, in the vein of lib/percpu_test.c.

---
I am sending this patch as an RFC to gather feedback on the API as well
as get some input on correctness. In particular, the unsafe getter macro
is somewhat cumbersome, though its safety requirements for pure-Rust
code aren't too difficult to verify (interoperation with C code is
certainly much more tricky).

It was suggested that I base my implementation on Rust's thread-local
storage API. However, this API requires that for a thread-local T, any
particular usage must be via a &T. Thus, many times it is required that
thread-local variables be in the form RefCell<T> or Cell<T>. This has
some pretty severe disadvantages in the context of per-CPU variables.
Namely, RefCell<T> may panic if .borrow_mut is called multiple times at
different points in the call stack. This is similar to the problem
inherent in my current implementation (that is, unsafe_get_per_cpu_ref
must be used carefully so as to not create aliasing mut references), but
this implementation flags this potential problem via an unsafe block and
the resulting PerCpuRef can be easily passed along and used fairly
easily. Cell<T> on the other hand requires a copy any time the
underlying value is used, which is entirely avoided here.

Signed-off-by: Mitchell Levy <levymitchell0@gmail.com>

---
Mitchell Levy (3):
      rust: percpu: introduce a rust API for per-CPU variables
      rust: rust-analyzer: add lib to dirs searched for crates
      rust: percpu: add a rust per-CPU variable test

 lib/Kconfig.debug                 |   9 +++
 lib/Makefile                      |   1 +
 lib/percpu_test_rust.rs           |  65 +++++++++++++++
 rust/helpers/helpers.c            |   1 +
 rust/helpers/preempt.c            |  14 ++++
 rust/kernel/lib.rs                |   3 +
 rust/kernel/percpu.rs             | 161 ++++++++++++++++++++++++++++++++++++++
 rust/kernel/percpu/cpu_guard.rs   |  29 +++++++
 scripts/generate_rust_analyzer.py |   2 +-
 9 files changed, 284 insertions(+), 1 deletion(-)
---
base-commit: 0c5928deada15a8d075516e6e0d9ee19011bb000
change-id: 20240813-rust-percpu-ea2f54b5da33

Best regards,
-- 
Mitchell Levy <levymitchell0@gmail.com>

[PATCH RFC 1/3] rust: percpu: introduce a rust API for per-CPU variables

[Mitchell Levy]

Add a CpuGuard type that disables preemption for its lifetime. Add a
PerCpuVariable type used for declaring per-CPU variables and a
declare_per_cpu! macro for its use. Add a PerCpuRef that allows actual
use of the per-CPU variable and an usafe_get_per_cpu_ref! helper macro
to convert PerCpuVariable to PerCpuRef.

Signed-off-by: Mitchell Levy <levymitchell0@gmail.com>
---
 rust/helpers/helpers.c          |   1 +
 rust/helpers/preempt.c          |  14 ++++
 rust/kernel/lib.rs              |   3 +
 rust/kernel/percpu.rs           | 161 ++++++++++++++++++++++++++++++++++++++++
 rust/kernel/percpu/cpu_guard.rs |  29 ++++++++
 5 files changed, 208 insertions(+)

diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
index dcf827a61b52..1db0dcd8f11f 100644
--- a/rust/helpers/helpers.c
+++ b/rust/helpers/helpers.c
@@ -19,6 +19,7 @@
 #include "mutex.c"
 #include "page.c"
 #include "pid_namespace.c"
+#include "preempt.c"
 #include "rbtree.c"
 #include "refcount.c"
 #include "security.c"
diff --git a/rust/helpers/preempt.c b/rust/helpers/preempt.c
new file mode 100644
index 000000000000..2c7529528ddd
--- /dev/null
+++ b/rust/helpers/preempt.c
@@ -0,0 +1,14 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/preempt.h>
+
+void rust_helper_preempt_disable(void)
+{
+	preempt_disable();
+}
+
+void rust_helper_preempt_enable(void)
+{
+	preempt_enable();
+}
+
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index e1065a7551a3..3634e1412d60 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -49,6 +49,9 @@
 #[cfg(CONFIG_NET)]
 pub mod net;
 pub mod page;
+// Only x86_64 is supported by percpu for now
+#[cfg(CONFIG_X86_64)]
+pub mod percpu;
 pub mod pid_namespace;
 pub mod prelude;
 pub mod print;
diff --git a/rust/kernel/percpu.rs b/rust/kernel/percpu.rs
new file mode 100644
index 000000000000..08fcc8797078
--- /dev/null
+++ b/rust/kernel/percpu.rs
@@ -0,0 +1,161 @@
+// SPDX-License-Identifier: GPL-2.0
+//! This module contains abstractions for creating and using per-CPU variables from Rust. In
+//! particular, see the define_per_cpu! and unsafe_get_per_cpu_ref! macros.
+pub mod cpu_guard;
+
+use crate::percpu::cpu_guard::CpuGuard;
+use crate::unsafe_get_per_cpu_ref;
+
+use core::arch::asm;
+use core::marker::PhantomData;
+use core::ops::{Deref, DerefMut};
+
+/// A PerCpuRef is obtained by the unsafe_get_per_cpu_ref! macro used on a PerCpuVariable defined
+/// via the define_per_cpu! macro.
+///
+/// This type will transparently deref(mut) into a &(mut) T referencing this CPU's instance of the
+/// underlying variable.
+pub struct PerCpuRef<T> {
+    offset: usize,
+    deref_type: PhantomData<T>,
+    _guard: CpuGuard,
+}
+
+/// A wrapper used for declaring static per-cpu variables. These symbols are "virtual" in that the
+/// linker uses them to generate offsets into each cpu's per-cpu area, but shouldn't be read
+/// from/written to directly. The fact that the statics are immutable prevents them being written
+/// to (generally), this struct having _val be non-public prevents reading from them.
+///
+/// The end-user of the per-CPU API should make use of the define_per_cpu! macro instead of
+/// declaring variables of this type directly.
+#[repr(transparent)]
+pub struct PerCpuVariable<T> {
+    _val: T, // generate a correctly sized type
+}
+
+impl<T> PerCpuRef<T> {
+    /// You should be using the unsafe_get_per_cpu! macro instead
+    ///
+    /// # Safety
+    /// offset must be a valid offset into the per cpu area
+    pub unsafe fn new(offset: usize, guard: CpuGuard) -> Self {
+        PerCpuRef {
+            offset,
+            deref_type: PhantomData,
+            _guard: guard,
+        }
+    }
+
+    /// Computes this_cpu_ptr as a usize, ignoring issues of ownership and borrowing
+    fn this_cpu_ptr_usize(&self) -> usize {
+        // SAFETY: this_cpu_off is read only as soon as the per-CPU subsystem is initialized
+        let off: PerCpuRef<u64> = unsafe { unsafe_get_per_cpu_ref!(this_cpu_off, CpuGuard::new()) };
+        let mut this_cpu_area: usize;
+        // SAFETY: gs + off_val is guaranteed to be a valid pointer by the per-CPU subsystem and
+        // the invariants guaranteed by PerCpuRef (i.e., off.offset is valid)
+        unsafe {
+            asm!(
+                // For some reason, the asm! parser doesn't like
+                //     mov {out}, [gs:{off_val}]
+                // so we use the less intuitive prefix version instead
+                "gs mov {out}, [{off_val}]",
+                off_val = in(reg) off.offset,
+                out = out(reg) this_cpu_area,
+            )
+        };
+        this_cpu_area + self.offset
+    }
+
+    /// Returns a pointer to self's associated per-CPU variable. Logically equivalent to C's
+    /// this_cpu_ptr
+    pub fn this_cpu_ptr(&self) -> *const T {
+        self.this_cpu_ptr_usize() as *const T
+    }
+
+    /// Returns a mut pointer to self's associated per-CPU variable. Logically equivalent to C's
+    /// this_cpu_ptr
+    pub fn this_cpu_ptr_mut(&mut self) -> *mut T {
+        self.this_cpu_ptr_usize() as *mut T
+    }
+}
+
+impl<T> Deref for PerCpuRef<T> {
+    type Target = T;
+    fn deref(&self) -> &Self::Target {
+        // SAFETY: By the contract of unsafe_get_per_cpu_ref!, we know that self is the only
+        // PerCpuRef associated with the underlying per-CPU variable and that the underlying
+        // variable is not mutated outside of rust.
+        unsafe { &*(self.this_cpu_ptr()) }
+    }
+}
+
+impl<T> DerefMut for PerCpuRef<T> {
+    fn deref_mut(&mut self) -> &mut Self::Target {
+        // SAFETY: By the contract of unsafe_get_per_cpu_ref!, we know that self is the only
+        // PerCpuRef associated with the underlying per-CPU variable and that the underlying
+        // variable is not mutated outside of rust.
+        unsafe { &mut *(self.this_cpu_ptr_mut()) }
+    }
+}
+
+/// define_per_cpu! is analogous to the C DEFINE_PER_CPU macro in that it lets you create a
+/// statically allocated per-CPU variable.
+///
+/// # Example
+/// ```
+/// use kernel::define_per_cpu;
+/// use kernel::percpu::PerCpuVariable;
+///
+/// define_per_cpu!(pub MY_PERCPU: u64 = 0);
+/// ```
+#[macro_export]
+macro_rules! define_per_cpu {
+    ($vis:vis $id:ident: $ty:ty = $expr:expr) => {
+        $crate::macros::paste! {
+            // Expand $expr outside of the unsafe block to avoid silently allowing unsafe code to be
+            // used without a user-facing unsafe block
+            static [<__init_ $id>]: $ty = $expr;
+
+            // SAFETY: PerCpuVariable<T> is #[repr(transparent)], so we can freely convert from T
+            #[link_section = ".data..percpu"]
+            $vis static $id: PerCpuVariable<$ty> = unsafe {
+                core::mem::transmute::<$ty, PerCpuVariable<$ty>>([<__init_ $id>])
+            };
+        }
+    };
+}
+
+/// Goes from a PerCpuVariable to a usable PerCpuRef. $id is the identifier of the PerCpuVariable
+/// and $guard is an expression that evaluates to a CpuGuard.
+///
+/// # Safety
+/// Don't create two PerCpuRef that point at the same per-cpu variable, as this would allow you to
+/// accidentally break aliasing rules. Unless T is Sync, the returned PerCpuRef should not be used
+/// from interrupt contexts.
+///
+/// If $id is `extern "C"` (i.e., declared via declare_extern_per_cpu!) then the underlying per-CPU
+/// variable must not be written from C code while a PerCpuRef exists in Rust. That is, the
+/// underlying per-CPU variable must not be written in any IRQ context (unless the user ensures
+/// IRQs are disabled) and no FFI calls can be made to C functions that may write the per-CPU
+/// variable. The underlying PerCpuVariable created via declare_extern_per_cpu must also have the
+/// correct type.
+#[macro_export]
+macro_rules! unsafe_get_per_cpu_ref {
+    ($id:ident, $guard:expr) => {{
+        let off = core::ptr::addr_of!($id);
+        PerCpuRef::new(off as usize, $guard)
+    }};
+}
+
+/// Declares a PerCpuVariable corresponding to a per-CPU variable defined in C. Be sure to read the
+/// safety requirements of unsafe_get_per_cpu_ref!.
+#[macro_export]
+macro_rules! declare_extern_per_cpu {
+    ($id:ident: $ty:ty) => {
+        extern "C" {
+            static $id: PerCpuVariable<$ty>;
+        }
+    };
+}
+
+declare_extern_per_cpu!(this_cpu_off: u64);
diff --git a/rust/kernel/percpu/cpu_guard.rs b/rust/kernel/percpu/cpu_guard.rs
new file mode 100644
index 000000000000..6f7d320a5c9a
--- /dev/null
+++ b/rust/kernel/percpu/cpu_guard.rs
@@ -0,0 +1,29 @@
+// SPDX-License-Identifier: GPL-2.0
+//! Contains abstractions for disabling CPU preemption. See CpuGuard.
+
+/// A RAII guard for bindings::preempt_disable and bindings::preempt_enable. Guarantees preemption
+/// is disabled for as long as this object exists.
+pub struct CpuGuard {
+    // Don't make one without using new()
+    _phantom: (),
+}
+
+impl CpuGuard {
+    /// Create a new CpuGuard. Disables preemption for its lifetime.
+    pub fn new() -> Self {
+        // SAFETY: There are no preconditions required to call preempt_disable
+        unsafe {
+            bindings::preempt_disable();
+        }
+        CpuGuard { _phantom: () }
+    }
+}
+
+impl Drop for CpuGuard {
+    fn drop(&mut self) {
+        // SAFETY: There are no preconditions required to call preempt_enable
+        unsafe {
+            bindings::preempt_enable();
+        }
+    }
+}

-- 
2.34.1

[PATCH RFC 2/3] rust: rust-analyzer: add lib to dirs searched for crates

[Mitchell Levy]

When generating rust-project.json, also include crates in lib/

Signed-off-by: Mitchell Levy <levymitchell0@gmail.com>
---
 scripts/generate_rust_analyzer.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/generate_rust_analyzer.py b/scripts/generate_rust_analyzer.py
index 09e1d166d8d2..7d7ffb45fc0c 100755
--- a/scripts/generate_rust_analyzer.py
+++ b/scripts/generate_rust_analyzer.py
@@ -109,7 +109,7 @@ def generate_crates(srctree, objtree, sysroot_src, external_src, cfgs):
     # Then, the rest outside of `rust/`.
     #
     # We explicitly mention the top-level folders we want to cover.
-    extra_dirs = map(lambda dir: srctree / dir, ("samples", "drivers"))
+    extra_dirs = map(lambda dir: srctree / dir, ("samples", "drivers", "lib"))
     if external_src is not None:
         extra_dirs = [external_src]
     for folder in extra_dirs:

-- 
2.34.1

[PATCH RFC 3/3] rust: percpu: add a rust per-CPU variable test

[Mitchell Levy]

Add a short exercise for Rust's per-CPU variable API, modelled after
lib/percpu_test.c

Signed-off-by: Mitchell Levy <levymitchell0@gmail.com>
---
 lib/Kconfig.debug       |  9 +++++++
 lib/Makefile            |  1 +
 lib/percpu_test_rust.rs | 65 +++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 75 insertions(+)

diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index f3d723705879..75a91f1766ce 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -2404,6 +2404,15 @@ config PERCPU_TEST
 
 	  If unsure, say N.
 
+config PERCPU_TEST_RUST
+	tristate "Rust per cpu operations test"
+	depends on m && DEBUG_KERNEL && RUST
+	help
+	  Enable this option to build a test module which validates Rust per-cpu
+	  operations.
+
+	  If unsure, say N.
+
 config ATOMIC64_SELFTEST
 	tristate "Perform an atomic64_t self-test"
 	help
diff --git a/lib/Makefile b/lib/Makefile
index a8155c972f02..0ea8d414763c 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -301,6 +301,7 @@ obj-$(CONFIG_RBTREE_TEST) += rbtree_test.o
 obj-$(CONFIG_INTERVAL_TREE_TEST) += interval_tree_test.o
 
 obj-$(CONFIG_PERCPU_TEST) += percpu_test.o
+obj-$(CONFIG_PERCPU_TEST_RUST) += percpu_test_rust.o
 
 obj-$(CONFIG_ASN1) += asn1_decoder.o
 obj-$(CONFIG_ASN1_ENCODER) += asn1_encoder.o
diff --git a/lib/percpu_test_rust.rs b/lib/percpu_test_rust.rs
new file mode 100644
index 000000000000..60df44332d7a
--- /dev/null
+++ b/lib/percpu_test_rust.rs
@@ -0,0 +1,65 @@
+// SPDX-License-Identifier: GPL-2.0
+//! A simple self test for the rust per-CPU API.
+use kernel::{
+    define_per_cpu, percpu::cpu_guard::*, percpu::*, pr_info, prelude::*, unsafe_get_per_cpu_ref,
+};
+
+module! {
+    type: PerCpuTestModule,
+    name: "percpu_test_rust",
+    author: "Mitchell Levy",
+    description: "Test code to exercise the Rust Per CPU variable API",
+    license: "GPL v2",
+}
+
+struct PerCpuTestModule;
+
+define_per_cpu!(PERCPU: i64 = 0);
+define_per_cpu!(UPERCPU: u64 = 0);
+
+impl kernel::Module for PerCpuTestModule {
+    fn init(_module: &'static ThisModule) -> Result<Self, Error> {
+        pr_info!("rust percpu test start\n");
+
+        let mut native: i64 = 0;
+        let mut pcpu: PerCpuRef<i64> = unsafe { unsafe_get_per_cpu_ref!(PERCPU, CpuGuard::new()) };
+
+        native += -1;
+        *pcpu += -1;
+        assert!(native == *pcpu && native == -1);
+
+        native += 1;
+        *pcpu += 1;
+        assert!(native == *pcpu && native == 0);
+
+        let mut unative: u64 = 0;
+        let mut upcpu: PerCpuRef<u64> =
+            unsafe { unsafe_get_per_cpu_ref!(UPERCPU, CpuGuard::new()) };
+
+        unative += 1;
+        *upcpu += 1;
+        assert!(unative == *upcpu && unative == 1);
+
+        unative = unative.wrapping_add((-1i64) as u64);
+        *upcpu = upcpu.wrapping_add((-1i64) as u64);
+        assert!(unative == *upcpu && unative == 0);
+
+        unative = unative.wrapping_add((-1i64) as u64);
+        *upcpu = upcpu.wrapping_add((-1i64) as u64);
+        assert!(unative == *upcpu && unative == (-1i64) as u64);
+
+        unative = 0;
+        *upcpu = 0;
+
+        unative = unative.wrapping_sub(1);
+        *upcpu = upcpu.wrapping_sub(1);
+        assert!(unative == *upcpu && unative == (-1i64) as u64);
+        assert!(unative == *upcpu && unative == u64::MAX);
+
+        pr_info!("rust percpu test done\n");
+
+        // Return Err to unload the module
+        Result::Err(EINVAL)
+    }
+}
+

-- 
2.34.1

Re: [PATCH RFC 3/3] rust: percpu: add a rust per-CPU variable test

[Christoph Lameter (Ampere)]

On Thu, 19 Dec 2024, Mitchell Levy wrote:

> +        let mut native: i64 = 0;
> +        let mut pcpu: PerCpuRef<i64> = unsafe { unsafe_get_per_cpu_ref!(PERCPU, CpuGuard::new()) };

A bit complex.

> +        native += -1;
> +        *pcpu += -1;
> +        assert!(native == *pcpu && native == -1);
> +
> +        native += 1;
> +        *pcpu += 1;
> +        assert!(native == *pcpu && native == 0);
> +

That's pretty straightforward..... But is there no symbolic access to the
per cpu namespace? How would you access the kernel per cpu variables
defined in C?

How do you go about using per cpu atomics like

this_cpu_inc(nr_dentry_unused);

Re: [PATCH RFC 3/3] rust: percpu: add a rust per-CPU variable test

[Boqun Feng]

On Fri, Dec 20, 2024 at 09:56:54AM -0800, Christoph Lameter (Ampere) wrote:
> On Thu, 19 Dec 2024, Mitchell Levy wrote:
> 
> > +        let mut native: i64 = 0;
> > +        let mut pcpu: PerCpuRef<i64> = unsafe { unsafe_get_per_cpu_ref!(PERCPU, CpuGuard::new()) };
> 
> A bit complex.
> 
> > +        native += -1;
> > +        *pcpu += -1;
> > +        assert!(native == *pcpu && native == -1);
> > +
> > +        native += 1;
> > +        *pcpu += 1;
> > +        assert!(native == *pcpu && native == 0);
> > +
> 
> That's pretty straightforward..... But is there no symbolic access to the
> per cpu namespace? How would you access the kernel per cpu variables
> defined in C?
> 

FWIW, there is a declare_extern_per_cpu!() marco in patch #1, and
Mitchell used it to access this_cpu_off (defined in C).

Maybe we want to see an example here?

Regards,
Boqun

> How do you go about using per cpu atomics like
> 
> this_cpu_inc(nr_dentry_unused);
> 
> 

Re: [PATCH RFC 0/3] rust: Add Per-CPU Variable API

[Dennis Zhou]

Hello,

On Thu, Dec 19, 2024 at 01:08:25PM -0800, Mitchell Levy wrote:
> This series adds an API for declaring an using per-CPU variables from
> Rust, and it also adds support for Rust access to C per-CPU variables
> (subject to some soundness requirements). It also adds a small test
> module, lib/percpu_test_rust.rs, in the vein of lib/percpu_test.c.
> 
> ---
> I am sending this patch as an RFC to gather feedback on the API as well
> as get some input on correctness. In particular, the unsafe getter macro
> is somewhat cumbersome, though its safety requirements for pure-Rust
> code aren't too difficult to verify (interoperation with C code is
> certainly much more tricky).
> 

I've been thinking a little bit about this over the holidays and fwiw am
not really the right person to evaluate the rust side of this. I did
briefly discuss this with Hudson (cced) who is more familiar with rust.

In addition to Christoph's comments:
 1. This implementation seems targeted at static allocations and the
    safety of this api relies on the end user doing the right thing.

    There are many percpu variables created dynamically via
    alloc_percpu() and the lifetime of those variables are tied to a
    larger object. So access to those variables is safe via the
    understanding the larger object exists. I feel like there is room
    here for rust to do this checking somehow? Maybe in rust-only percpu
    variables, but something better nonetheless.

 2. As Christoph mentioned, the percpu api relies heavily on macros for
    architecture specific optimizations. Your implementation seems
    asm-generic correct, but we should find a way to export the
    architecture specific percpu macros.

    `this_cpu_off` is a percpu variable that helps x86 `arch_raw_cpu_ptr()`.

 3. Maybe a good use case to start off with would be percpu_refcount and
    how rust can use it or how it can make something like that safer?

Some of Hudson's direct comments about the Rust implementation:
 1. DerefMut for PerCpuRef is ultimately constructing a reference from a
    raw integer without preserving the provenance of the original
    pointer in any way (since this integer is being constructed from raw
    register reads). I believe this violates Rust's pointer provenance
    rules for unsafe code.

 2. The safety requirements of unsafe_get_per_cpu_ref seem like they
    would often not be possible to verify at the callsite of the macro
    without global knowledge -- especially for percpu variables defined
    in C code. Is this a misunderstanding? Can you show an example of
    where it would be straightforward to build a higher-level safe API
    on top of this unsafe one?

> It was suggested that I base my implementation on Rust's thread-local
> storage API. However, this API requires that for a thread-local T, any
> particular usage must be via a &T. Thus, many times it is required that
> thread-local variables be in the form RefCell<T> or Cell<T>. This has
> some pretty severe disadvantages in the context of per-CPU variables.
> Namely, RefCell<T> may panic if .borrow_mut is called multiple times at
> different points in the call stack. This is similar to the problem
> inherent in my current implementation (that is, unsafe_get_per_cpu_ref
> must be used carefully so as to not create aliasing mut references), but
> this implementation flags this potential problem via an unsafe block and
> the resulting PerCpuRef can be easily passed along and used fairly
> easily. Cell<T> on the other hand requires a copy any time the
> underlying value is used, which is entirely avoided here.
> 
> Signed-off-by: Mitchell Levy <levymitchell0@gmail.com>
> 

I think this is a cool topic and has helped inspire me to learn a bit
more rust. Hopefully I can be more helpful in the future.
 
Thanks,
Dennis

Re: [PATCH RFC 3/3] rust: percpu: add a rust per-CPU variable test

[Charalampos Mitrodimas]

"Christoph Lameter (Ampere)" <cl@gentwo.org> writes:

> On Thu, 19 Dec 2024, Mitchell Levy wrote:
>
>> +        let mut native: i64 = 0;
>> +        let mut pcpu: PerCpuRef<i64> = unsafe { unsafe_get_per_cpu_ref!(PERCPU, CpuGuard::new()) };
>
> A bit complex.

I agree with this, maybe a helper function would suffise? Something in
terms of,
  unsafe fn get_per_cpu<T>(var: &PerCpuVariable<T>) -> PerCpuRef<T> {
	  unsafe_get_per_cpu_ref!(var, CpuGuard::new())
  }

>
>> +        native += -1;
>> +        *pcpu += -1;
>> +        assert!(native == *pcpu && native == -1);
>> +
>> +        native += 1;
>> +        *pcpu += 1;
>> +        assert!(native == *pcpu && native == 0);
>> +
>
> That's pretty straightforward..... But is there no symbolic access to the
> per cpu namespace? How would you access the kernel per cpu variables
> defined in C?
>
> How do you go about using per cpu atomics like
>
> this_cpu_inc(nr_dentry_unused);

Re: [PATCH RFC 0/3] rust: Add Per-CPU Variable API

[Mitchell Levy]

On Sat, Jan 04, 2025 at 02:45:43PM -0800, Dennis Zhou wrote:
> Hello,
> 
> On Thu, Dec 19, 2024 at 01:08:25PM -0800, Mitchell Levy wrote:
> > This series adds an API for declaring an using per-CPU variables from
> > Rust, and it also adds support for Rust access to C per-CPU variables
> > (subject to some soundness requirements). It also adds a small test
> > module, lib/percpu_test_rust.rs, in the vein of lib/percpu_test.c.
> > 
> > ---
> > I am sending this patch as an RFC to gather feedback on the API as well
> > as get some input on correctness. In particular, the unsafe getter macro
> > is somewhat cumbersome, though its safety requirements for pure-Rust
> > code aren't too difficult to verify (interoperation with C code is
> > certainly much more tricky).
> > 
> 
> I've been thinking a little bit about this over the holidays and fwiw am
> not really the right person to evaluate the rust side of this. I did
> briefly discuss this with Hudson (cced) who is more familiar with rust.
> 
> In addition to Christoph's comments:
>  1. This implementation seems targeted at static allocations and the
>     safety of this api relies on the end user doing the right thing.
> 
>     There are many percpu variables created dynamically via
>     alloc_percpu() and the lifetime of those variables are tied to a
>     larger object. So access to those variables is safe via the
>     understanding the larger object exists. I feel like there is room
>     here for rust to do this checking somehow? Maybe in rust-only percpu
>     variables, but something better nonetheless.

You're correct; my hope is that adding dynamic allocations on top of
this API should be straightforward. Because we don't have to worry
about aliasing concerns (since we're in control of the memory that gets
set aside), we can just directly spit out a PerCpuRef. The one wrinkle
would be adding a lifetime parameter to PerCpuRef and making everything
line up.

>  2. As Christoph mentioned, the percpu api relies heavily on macros for
>     architecture specific optimizations. Your implementation seems
>     asm-generic correct, but we should find a way to export the
>     architecture specific percpu macros.
> 
>     `this_cpu_off` is a percpu variable that helps x86 `arch_raw_cpu_ptr()`.

Definitely! I'm hoping to add some operator overloading to make using
the optimized operations really smooth. I didn't want to bite off too
much for this RFC, though.

>  3. Maybe a good use case to start off with would be percpu_refcount and
>     how rust can use it or how it can make something like that safer?

This looks like a great idea, thank you!

> Some of Hudson's direct comments about the Rust implementation:
>  1. DerefMut for PerCpuRef is ultimately constructing a reference from a
>     raw integer without preserving the provenance of the original
>     pointer in any way (since this integer is being constructed from raw
>     register reads). I believe this violates Rust's pointer provenance
>     rules for unsafe code.

Caveat: I'm not super familiar with Rust's provenance model.

My understanding is that there are no "provenance issues" in a loose
sense. What I mean by this is that, if everything is treated as the
pointer it "should be" the process looks like:

per-cpu subsystem owns per-cpu area -> pointer is stored in
this_cpu_area -> we create a new pointer (via an in-bounds offset +
shrinking to sizeof(T)) to the contents of the per-cpu variable in
question

It looks like asm! actually supports reading into pointer types (I had
misread the table in the Rust Reference, and thought it only supported
reading into integer types) --- this might help avoid discarding
provenance during a cast to usize? I'm definitely interested in hearing more
on this point.

>  2. The safety requirements of unsafe_get_per_cpu_ref seem like they
>     would often not be possible to verify at the callsite of the macro
>     without global knowledge -- especially for percpu variables defined
>     in C code. Is this a misunderstanding? Can you show an example of
>     where it would be straightforward to build a higher-level safe API
>     on top of this unsafe one?

Yeah interoperation with C per-cpu variables is certainly very
difficult. Unfortunately, I think this problem crops up any time you're
sharing a piece of memory between Rust and C since the existence of &T
that points at some piece of memory is a guarantee to the compiler that
the pointed-to memory will not change. This facility is intended more as
a trap-door for when there's no better option than as an API building
block.

> > It was suggested that I base my implementation on Rust's thread-local
> > storage API. However, this API requires that for a thread-local T, any
> > particular usage must be via a &T. Thus, many times it is required that
> > thread-local variables be in the form RefCell<T> or Cell<T>. This has
> > some pretty severe disadvantages in the context of per-CPU variables.
> > Namely, RefCell<T> may panic if .borrow_mut is called multiple times at
> > different points in the call stack. This is similar to the problem
> > inherent in my current implementation (that is, unsafe_get_per_cpu_ref
> > must be used carefully so as to not create aliasing mut references), but
> > this implementation flags this potential problem via an unsafe block and
> > the resulting PerCpuRef can be easily passed along and used fairly
> > easily. Cell<T> on the other hand requires a copy any time the
> > underlying value is used, which is entirely avoided here.
> > 
> > Signed-off-by: Mitchell Levy <levymitchell0@gmail.com>
> > 
> 
> I think this is a cool topic and has helped inspire me to learn a bit
> more rust. Hopefully I can be more helpful in the future.

I appreciate it; thanks to you and Hudson for your help!

> Thanks,
> Dennis

Re: [PATCH RFC 3/3] rust: percpu: add a rust per-CPU variable test

[Mitchell Levy]

On Sun, Jan 05, 2025 at 01:01:43PM +0000, Charalampos Mitrodimas wrote:
> "Christoph Lameter (Ampere)" <cl@gentwo.org> writes:
> 
> > On Thu, 19 Dec 2024, Mitchell Levy wrote:
> >
> >> +        let mut native: i64 = 0;
> >> +        let mut pcpu: PerCpuRef<i64> = unsafe { unsafe_get_per_cpu_ref!(PERCPU, CpuGuard::new()) };
> >
> > A bit complex.
> 
> I agree with this, maybe a helper function would suffise? Something in
> terms of,
>   unsafe fn get_per_cpu<T>(var: &PerCpuVariable<T>) -> PerCpuRef<T> {
> 	  unsafe_get_per_cpu_ref!(var, CpuGuard::new())
>   }

I'm certainly open to adding such a helper. Is the main concern here the
unwieldy name? Generally, I prefer to keep modifications to global state
(disabling preemption via CpuGuard::new()) as explicit as possible, but
if there's consensus to the contrary, I'm happy to roll it into the
macro/a helper function.

> >
> >> +        native += -1;
> >> +        *pcpu += -1;
> >> +        assert!(native == *pcpu && native == -1);
> >> +
> >> +        native += 1;
> >> +        *pcpu += 1;
> >> +        assert!(native == *pcpu && native == 0);
> >> +
> >
> > That's pretty straightforward..... But is there no symbolic access to the
> > per cpu namespace? How would you access the kernel per cpu variables
> > defined in C?
> >
> > How do you go about using per cpu atomics like
> >
> > this_cpu_inc(nr_dentry_unused);

Re: [PATCH RFC 3/3] rust: percpu: add a rust per-CPU variable test

[Charalampos Mitrodimas]

Mitchell Levy <levymitchell0@gmail.com> writes:

> On Sun, Jan 05, 2025 at 01:01:43PM +0000, Charalampos Mitrodimas wrote:
>> "Christoph Lameter (Ampere)" <cl@gentwo.org> writes:
>>
>> > On Thu, 19 Dec 2024, Mitchell Levy wrote:
>> >
>> >> +        let mut native: i64 = 0;
>> >> +        let mut pcpu: PerCpuRef<i64> = unsafe { unsafe_get_per_cpu_ref!(PERCPU, CpuGuard::new()) };
>> >
>> > A bit complex.
>>
>> I agree with this, maybe a helper function would suffise? Something in
>> terms of,
>>   unsafe fn get_per_cpu<T>(var: &PerCpuVariable<T>) -> PerCpuRef<T> {
>> 	  unsafe_get_per_cpu_ref!(var, CpuGuard::new())
>>   }
>
> I'm certainly open to adding such a helper. Is the main concern here the
> unwieldy name? Generally, I prefer to keep modifications to global state
> (disabling preemption via CpuGuard::new()) as explicit as possible, but
> if there's consensus to the contrary, I'm happy to roll it into the
> macro/a helper function.

Yes, in my opinion, the macro name is indeed complex. You're right about
keeping modifications as explicit as possible. A helper wouldn’t be
necessary if the macro name were simpler.

Is adding "unsafe_" to a macro that is unsafe a standard practice? Maybe
we can remove it from the macro name. Documentation is enough IMO.

>
>> >
>> >> +        native += -1;
>> >> +        *pcpu += -1;
>> >> +        assert!(native == *pcpu && native == -1);
>> >> +
>> >> +        native += 1;
>> >> +        *pcpu += 1;
>> >> +        assert!(native == *pcpu && native == 0);
>> >> +
>> >
>> > That's pretty straightforward..... But is there no symbolic access to the
>> > per cpu namespace? How would you access the kernel per cpu variables
>> > defined in C?
>> >
>> > How do you go about using per cpu atomics like
>> >
>> > this_cpu_inc(nr_dentry_unused);