"Benno Lossin" [off-list ref] writes:
On Mon Jun 23, 2025 at 4:31 PM CEST, Andreas Hindborg wrote:
quoted
"Benno Lossin" [off-list ref] writes:
quoted
On Mon Jun 23, 2025 at 11:44 AM CEST, Andreas Hindborg wrote:
quoted
"Benno Lossin" [off-list ref] writes:
quoted
On Fri Jun 20, 2025 at 1:29 PM CEST, Andreas Hindborg wrote:
quoted
"Benno Lossin" [off-list ref] writes:
quoted
On Thu Jun 12, 2025 at 3:40 PM CEST, Andreas Hindborg wrote:
quoted
+/// A wrapper for kernel parameters.
+///
+/// This type is instantiated by the [`module!`] macro when module parameters are
+/// defined. You should never need to instantiate this type directly.
+///
+/// Note: This type is `pub` because it is used by module crates to access
+/// parameter values.
+#[repr(transparent)]
+pub struct ModuleParamAccess<T> {
+ data: core::cell::UnsafeCell<T>,
+}
+
+// SAFETY: We only create shared references to the contents of this container,
+// so if `T` is `Sync`, so is `ModuleParamAccess`.
+unsafe impl<T: Sync> Sync for ModuleParamAccess<T> {}
+
+impl<T> ModuleParamAccess<T> {
+ #[doc(hidden)]
+ pub const fn new(value: T) -> Self {
+ Self {
+ data: core::cell::UnsafeCell::new(value),
+ }
+ }
+
+ /// Get a shared reference to the parameter value.
+ // Note: When sysfs access to parameters are enabled, we have to pass in a
+ // held lock guard here.
+ pub fn get(&self) -> &T {
+ // SAFETY: As we only support read only parameters with no sysfs
+ // exposure, the kernel will not touch the parameter data after module
+ // initialization.
This should be a type invariant. But I'm having difficulty defining one
that's actually correct: after parsing the parameter, this is written
to, but when is that actually?
For built-in modules it is during kernel initialization. For loadable
modules, it during module load. No code from the module will execute
before parameters are set.
Gotcha and there never ever will be custom code that is executed
before/during parameter setting (so code aside from code in `kernel`)?
quoted
quoted
Would we eventually execute other Rust
code during that time? (for example when we allow custom parameter
parsing)
I don't think we will need to synchronize because of custom parameter
parsing. Parameters are initialized sequentially. It is not a problem if
the custom parameter parsing code name other parameters, because they
are all initialized to valid values (as they are statics).
If you have `&'static i64`, then the value at that reference is never
allowed to change.
quoted
quoted
This function also must never be `const` because of the following:
module! {
// ...
params: {
my_param: i64 {
default: 0,
description: "",
},
},
}
static BAD: &'static i64 = module_parameters::my_param.get();
AFAIK, this static will be executed before loading module parameters and
thus it makes writing to the parameter UB.
As I understand, the static will be initialized by a constant expression
evaluated at compile time. I am not sure what happens when this is
evaluated in const context:
pub fn get(&self) -> &T {
// SAFETY: As we only support read only parameters with no sysfs
// exposure, the kernel will not touch the parameter data after module
// initialization.
unsafe { &*self.data.get() }
}
Why would that not be OK? I would assume the compiler builds a dependency graph
when initializing statics?
Yes it builds a dependency graph, but that is irrelevant? The problem is
that I can create a `'static` reference to the inner value *before* the
parameter is written-to (as the static is initialized before the
parameters).
I see, I did not consider this situation. Thanks for pointing this out.
Could we get around this without a lock maybe? If we change
`ModuleParamAccess::get` to take a closure instead:
/// Call `func` with a reference to the parameter value stored in `Self`.
pub fn read(&self, func: impl FnOnce(&T)) {
// SAFETY: As we only support read only parameters with no sysfs
// exposure, the kernel will not touch the parameter data after module
// initialization.
let data = unsafe { &*self.data.get() };
func(data)
}
I think this would bound the lifetime of the reference passed to the
closure to the duration of the call, right?
Yes that is correct. Now you can't assign the reference to a static.
However, this API is probably very clunky to use, since you always have
to create a closure etc.
Since you mentioned in the other reply that one could spin up a thread
and do something simultaneously, I don't think this is enough. You could
have a loop spin over the new `read` function and read the value and
then the write happens.
Yes you are right, we have to treat it as if it could be written at any
point in time.
quoted
One way to fix this issue would be to use atomics to read the value and
to not create a reference to it. So essentially have
pub fn read(&self) -> T {
unsafe { atomic_read_unsafe_cell(&self.data) }
}
That could work.
quoted
Another way would be to use a `Once`-like type (does that exist on the C
side?) so a type that can be initialized once and then never changes.
While it doesn't have a value set, we return some default value for the
param and print a warning, when it's set, we just return the value. But
this probably also requires atomics...
I think atomic bool is not that far away. Either that, or we can lock.
quoted
Is parameter accessing used that often in hot paths? Can't you just copy
the value into your `Module` struct?
I don't imagine this being read in a hot path. If so, the user could
make a copy.
That's good to know, then let's try to go for something simple.
I don't think that we can just use a `Mutex<T>`, because we don't have a
way to create it at const time... I guess we could have
impl<T> Mutex<T>
/// # Safety
///
/// The returned value needs to be pinned and then `init` needs
/// to be called before any other methods are called on this.
pub unsafe const fn const_new() -> Self;
pub unsafe fn init(&self);
}
But that seems like a bad idea, because where would we call the `init`
function? That also needs to be synchronized...
Ah, that is unfortunate. The init function will not run before this, so
we would need a `Once` or an atomic anyway to initialize the lock.
I am not sure if we are allowed to sleep during this, I would have to
check. But then we could use a spin lock.
We will need the locking anyway, when we want to enable sysfs write
access to the parameters.
Maybe we can just like you said use an atomic bool?
Sigh, I will have to check how far that series has come.
Best regards,
Andreas Hindborg