Re: [PATCH] rust: add `module_params` macro

[Andreas Hindborg <nmi@metaspace.dk>]

"Benno Lossin" <benno.lossin@proton.me> writes:

> On 02.08.24 12:27, Andreas Hindborg wrote:
>> "Benno Lossin" <benno.lossin@proton.me> writes:
>>> On 01.08.24 17:11, Andreas Hindborg wrote:
>>>> "Benno Lossin" <benno.lossin@proton.me> writes:
>>>>> On 01.08.24 15:40, Andreas Hindborg wrote:
>>>>>> "Benno Lossin" <benno.lossin@proton.me> writes:
>>>>>>> On 01.08.24 13:29, Andreas Hindborg wrote:
>>>>>>>> "Benno Lossin" <benno.lossin@proton.me> writes:
>>>>>>>>> On 05.07.24 13:15, Andreas Hindborg wrote:
>>>>>>>>>> +
>>>>>>>>>> +/// Types that can be used for module parameters.
>>>>>>>>>> +///
>>>>>>>>>> +/// Note that displaying the type in `sysfs` will fail if
>>>>>>>>>> +/// [`core::str::from_utf8`] (as implemented through the [`core::fmt::Display`]
>>>>>>>>>> +/// trait) writes more than [`PAGE_SIZE`] bytes (including an additional null
>>>>>>>>>> +/// terminator).
>>>>>>>>>> +///
>>>>>>>>>> +/// [`PAGE_SIZE`]: `bindings::PAGE_SIZE`
>>>>>>>>>> +pub trait ModuleParam: core::fmt::Display + core::marker::Sized {
>>>>>>>>>> +    /// The `ModuleParam` will be used by the kernel module through this type.
>>>>>>>>>> +    ///
>>>>>>>>>> +    /// This may differ from `Self` if, for example, `Self` needs to track
>>>>>>>>>> +    /// ownership without exposing it or allocate extra space for other possible
>>>>>>>>>> +    /// parameter values. This is required to support string parameters in the
>>>>>>>>>> +    /// future.
>>>>>>>>>> +    type Value: ?Sized;
>>>>>>>>>> +
>>>>>>>>>> +    /// Whether the parameter is allowed to be set without an argument.
>>>>>>>>>> +    ///
>>>>>>>>>> +    /// Setting this to `true` allows the parameter to be passed without an
>>>>>>>>>> +    /// argument (e.g. just `module.param` instead of `module.param=foo`).
>>>>>>>>>> +    const NOARG_ALLOWED: bool;
>>>>>>>>>
>>>>>>>>> I think, there is a better way of doing this. Instead of this bool, we
>>>>>>>>> do the following:
>>>>>>>>> 1. have a `const DEFAULT: Option<Self>`
>>>>>>>>> 2. change the type of the argument of `try_from_param_arg` to
>>>>>>>>>    `&'static [u8]`
>>>>>>>>>
>>>>>>>>> That way we don't have the weird behavior of `try_from_param_arg` that
>>>>>>>>> for params that don't have a default value.
>>>>>>>>
>>>>>>>> Since we have no parameter types for which `NOARG_ALLOWED` is true in
>>>>>>>> this patch set, it is effectively dead code. I will remove it.
>>>>>>>
>>>>>>> Hmm what parameters actually are optional? I looked at the old rust
>>>>>>> branch and only `bool` is marked as optional. Are there others?
>>>>>>>
>>>>>>> If it is used commonly for custom parameters (I could imagine that Rust
>>>>>>> modules have enums as parameters and specifying nothing could mean the
>>>>>>> default value), then it might be a good idea to just include it now.
>>>>>>> (otherwise we might forget the design later)
>>>>>>
>>>>>> As far as I can tell from the C code, all parameters are able to have
>>>>>> the `NOARG` flag set. We get a null pointer in the callback in that
>>>>>> case.
>>>>>>
>>>>>> If we want to handle this now, we could drop the `default` field
>>>>>> in the Rust module macro. There is no equivalent in the C macros.
>>>>>> And then use an `Option<Option<_>>` to represent the value. `None` would
>>>>>> be an unset parameter. `Some(None)` would be a parameter without a
>>>>>> value. `Some(Some(_))` would be a set parameter with a value. We could
>>>>>> probably fix the types so that only parameters with the `NOARG` flag use
>>>>>> the double option, others use a single option.
>>>>>
>>>>> What did you think of my approach that I detailed above? I would like to
>>>>> avoid `Option<Option<_>>` if we can.
>>>>
>>>> How would you represent the case when the parameter is passed without a
>>>> value and a default is given in `module!`?
>>>
>>> I am a bit confused, there are two default values here:
>>> (1) the value returned by `try_from_param_arg(None)`.
>>> (2) the value given by the user to the `module!` macro.
>>>
>>> I am talking about changing the definition of ModuleParam, so (1). I get
>>> the feeling that you are talking about (2), is that correct?
>> 
>> I confused myself as well I think. I am talking about (1). Let me try
>> again.
>> 
>> If we support `NOARG_ALLOWED` (`KERNEL_PARAM_OPS_FL_NOARG` in C. I
>> should change the flag name in Rust), modules can optionally support
>> some parameters where users can pass parameters either as
>> `my_module.param=value` or `my_module.param`. Thus, at the level of
>> `try_from_param_arg`, we need to represent two cases: parameter passed
>> without value, and parameter passed with value. A third case, parameter
>> not passed at all, is equivalent to `try_from_param_arg` never being
>> called. In C this is undetectable for the predefined parameter types. I
>> wanted the double option to detect this. But I guess it does not make
>> sense.
>
> My idea was to have an `const DEFAULT: Option<Self>` to represent the
> following:
> (1) if `DEFAULT == None`, then `KERNEL_PARAM_OPS_FL_NOARG` is not set
>     and thus either the `module!` user-specified default value is used,
>     or it is specified via `my_module.param=value` and
>     `try_from_param_arg` is called.
> (2) if `DEFAULT == Some(d)`, then `KERNEL_PARAM_OPS_FL_NOARG` is set and
>     when `NULL` is given to `kernel_param_ops.set`, the parameter value
>     is set to `d`, otherwise `try_from_param_arg` is called.
>
> But I think I agree with you removing `NOARG_ALLOWED`, see below.

Great!

>
>> At a higher level where the bindings supply the parsing functions, we
>> can decide that passing an argument without a value yields a default
>> parameter value. C does this for the predefined `bool` type. The
>> predefined integer types does not support omitting the value.
>>
>> This patch only supports the higher level predefined parameter types,
>> and does not allow modules to supply their own parameter parsing
>> functions. None of the types we implement in this patch support passing
>> the argument without a value. This is intentional to mirror the C
>> implementation.
>> 
>> To that end, I removed `NOARG_ALLOWED`, and changed the parsing function
>> trait to:
>> 
>>     fn try_from_param_arg(arg: &'static [u8]) -> Result<Self>;
>> 
>> If/when we start supporting types like `bool` or custom parsing
>> functions provided by the module, we will have to update the signature
>> to take an `Option` to represent the case where the user passed an
>> argument without a value. However, to mimic C, the function must always
>> return a value if successful, even if the user did not supply a value to
>> the argument.
>> 
>> Two different default values are in flight here. 1) the value that the
>> parameter will have before the kernel calls `try_from_param_arg` via
>> `set_param` and 2) the value to return from `try_from_param_arg` if the
>> user did not pass a value with the argument.
>> 
>> For a `bool` 1) would usually be `false` and 2) would always be `true`.
>> 
>> For predefined types the module would not customize 2), but 1) is useful
>> to customize. For custom types where the module supplies the parsing
>> function, 2) would be implicitly given by the module in the parsing
>> function.
>> 
>> In this patch set, we only have 1 default value, namely 1). We do not
>> need 2) because we do not support parameters without values.
>
> I am not sure that putting the default value of `my_module.param` into
> the `ModuleParam` trait is a good idea. It feels more correct to me to
> add an optional field to the part in `module!` that can be set to denote
> this default value -- we might also want to change the name of
> `default`, what do you think of `default_inactive` and `default_active`?

For all the predefined parameter types, the module code would never set
the `default_active` value. It should be part of the data parsing
specification for the predefined argument types.

For custom parsing functions implemented in the module, it makes sense
specifying this value in the trait.

> Since one might want an option by default be `true` and when one writes
> `my_module.param`, it should be `false`.

I think this would be confusing, since the default C parameter types do
not have this semantic. Specifying a default true boolean as an argument
does not make it false in C land. TBH this also feels like the most sane
thing to do.

> Also as the C side shows, having default values for integer types is not
> really a good idea, since you might want a non-zero default value.
> If one does not specify the `default_active` value, then the
> `KERNEL_PARAM_OPS_FL_NOARG` is not set.

I would rather predefine `KERNEL_PARAM_OPS_FL_NOARG` in the trait
implementation like C does. We would set the flag for `bool` but not for
integer types. For custom implementations of the trait, the developer
can do whatever they want.

> If you don't want to implement this (which I can fully understand, since
> we might get `syn` before anyone needs params with default values), then
> we should write this idea down (maybe in an issue?). But regardless, I
> would like to know your opinion on this topic.

We can create he issue if this series is accepted without the feature.

>
>>>> I think we need to drop the default value if we adopt the arg without
>>>> value scheme.
>>>
>>> Yes definitely. I don't see anything in the code doing this currently,
>>> right?
>> 
>> The current patch uses the default value given in the `module!` macro to
>> initialize the parameter value.
>
> But what drops the default value, when an actual value is specified via
> `my_module.param=value`? (or is the default value only assigned when
> nothing is specified?)

Some more confusion maybe. When I wrote "drop the default value", I
meant remove it from the code, not specify it. I take it back though. We
would use the value given in the `module!` macro `default` field as
`default_inactive`. `default_active` is not required for integer types,
since they always require a value for the argument.

But the drop would happen in `set_param` when the return value of
`core::ptr::replace` is dropped.

>
>>> We could also only allow `Copy` values to be used as Parameters (but
>>> then `str` cannot be used as a parameter...), since they can't implement
>>> `Drop`.
>> 
>> We should plan on eventually supporting `String`.
>
> Yes.
>
>>>>>> Or we could just not adopt this feature in the Rust abstractions.
>>>>>
>>>>> Depends on how common this is and if people need to use it. I think that
>>>>> what I proposed above isn't that complex, so it should be easy to
>>>>> implement.
>>>>
>>>> Rust modules would just force people to add "my_module.param=1" instead
>>>> of just "my_module.param". I think that is reasonable.
>>>
>>> Eh, why do we want to give that up? I don't think it's difficult to do.
>> 
>> I just don't see the point. Just have the user pass `my_module.param=1`
>> instead of omitting the value. Having multiple ways of specifying for
>> instance a true boolean just leads to confusion. Some boolean parameters
>> have a default value of `true`, for instance `nvme.use_cmb_sqes`. In
>> this case specifying `nvme.use_cmb_sqes` has no effect, even though one
>> might think it has.
>
> This just shows to me that a "global" default in `ModuleParam` is wrong,
> since for `use_cmb_sqes` one could either have a negated flag, or no
> default value, forcing the user to write `nvme.use_cmb_sqes=false`.

I do not think it is a good idea to be able to override the value
assigned to a parameter when it is given without a value. The more
predictable a user interface is, the better.


Best regards,
Andreas