Message-ID: <b95cc90a-46ae-44af-90af-0fc374cd381a@proton.me>
Date: Tue, 06 Aug 2024 08:09:09 +0000
From: Benno Lossin <benno.lossin@...ton.me>
To: Andreas Hindborg <nmi@...aspace.dk>
Cc: Luis Chamberlain <mcgrof@...nel.org>, Miguel Ojeda <ojeda@...nel.org>, rust-for-linux@...r.kernel.org, linux-modules@...r.kernel.org, linux-kernel@...r.kernel.org, Andreas Hindborg <a.hindborg@...sung.com>, Adam Bratschi-Kaye <ark.email@...il.com>
Subject: Re: [PATCH] rust: add `module_params` macro

On 05.08.24 12:55, Andreas Hindborg wrote:
> "Benno Lossin" <benno.lossin@...ton.me> writes:
>> On 02.08.24 12:27, Andreas Hindborg wrote:
>>> 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.

So if your module has an i32 parameter, you can't set a default value to
eg 1000?

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

Couldn't I just emulate the behavior from above by creating a
`struct MyI32(i32)` and having a default value?
In that case, why make it more difficult instead of providing a simple
way to do it?

>> 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.

Can't you also do the custom parameter parsing from above? ie have an
integer parameter with a default value?
Or is that frowned upon (ie not allowed through review)?

>> 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.

So we are only going to use it for `bool`?

>> 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.

Ah I see, I missed that.

>>>>> 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.

By that argument, we should also not permit custom implementations to
specify a default value.

---
Cheers,
Benno

Powered by blists - more mailing lists