Message-ID: <87ikw2rlbi.fsf@metaspace.dk>
Date: Thu, 15 Aug 2024 13:11:09 +0000
From: Andreas Hindborg <nmi@...aspace.dk>
To: Benno Lossin <benno.lossin@...ton.me>
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

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

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

You _would_ be able to set the `default_inactive` value, which is the
value assigned to the static at initialization time. It would make sense
to default this to 0 for integer types and make it overridable in the
`module!` macro.

You would not be to set the `default_active` value, which is the value
assigned to the parameter static variable, when the parameter is passed
without value. The reason being that we want to mirror C, so we prohibit
this for predefined integer parameter types.

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

The way parameter parsing works in C (that we try to replicate) is that
we are able to parse a set of predefined types. For each predefined type
we have a set of parser functions. If a module author want to parse
something outside of the predefined set, they can provide custom parser
functions.

I would advocate we do the same in Rust. This patch provides a set of
predefined types that can be parsed. For parsing to other types, we
would eventually expand the features to allow the module authors to
implement the parsing trait on their own types.

So to answer your question: No, not in this patch set. But let's plan to
add it in the future.

>
>>> 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)?

In C, you cannot, if you utilize the predefined parameter parsing
functions. Your can supply your own parser, and then you can do whatever
you want.

I would assume that it is best practice to reuse the predefined parsing
functions. For Rust code I would argue that we should have module
authors use the predefined parsers, unless they have very special
requirements.

>
>>> 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`?

I think so.

As far as I can tell, the predefined C parsers only allow this for bool.
Outside of the predefined parsers, I see only 4 custom argument parsers
that allow parameters without values.

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

If we provide a predefined set of parsing functions, most uses will use
those, and we will get a similar user experience across most of the
modules. If we allow said customization of the predefined parsers,
people will use it those customizations, and we will not get a good user
experience. If we omit the customizations, I think most will still stick
to predefined semantics, rather than implement their own parsers. For
those that actually need to have other semantics (or types), we will
have the option of user provided parsers, just as C.


Best regards,
Andreas

Powered by blists - more mailing lists