| Message ID | 20241213-module-params-v3-v3-0-485a015ac2cf@kernel.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | rust: extend `module!` macro with integer parameter support | expand |
On Fri, Dec 13, 2024 at 12:30:45PM +0100, Andreas Hindborg wrote:
> This series extends the `module!` macro with support module parameters.
Eeek, why?
Module parameters are from the 1990's, back when we had no idea what we
were doing and thought that a simple "one variable for a driver that
controls multiple devices" was somehow a valid solution :)
Please only really add module parameters if you can prove that you
actually need a module parameter.
thanks,
greg k-h
"Greg KH" <gregkh@linuxfoundation.org> writes: > On Fri, Dec 13, 2024 at 12:30:45PM +0100, Andreas Hindborg wrote: >> This series extends the `module!` macro with support module parameters. > > Eeek, why? > > Module parameters are from the 1990's, back when we had no idea what we > were doing and thought that a simple "one variable for a driver that > controls multiple devices" was somehow a valid solution :) > > Please only really add module parameters if you can prove that you > actually need a module parameter. I really need module parameters to make rust null block feature compatible with C null block. Let's not block interfacing parts of the kernel because we decided that the way we (well not me, I was not around) did things in the 80's was less than stellar. I mean, we would get nowhere. Best regards, Andreas Hindborg
Hi Luis, Petr, Sami, Dani, I just noticed that I failed in email and forgot to add you to the recipient list of this series. Do you want a resend, or is this sufficient? Best regards, Andreas Hindborg "Andreas Hindborg" <a.hindborg@kernel.org> writes: > This series extends the `module!` macro with support module parameters. It > also adds some string to integer parsing functions and updates `BStr` with > a method to strip a string prefix. > > This series stated out as code by Adam Bratschi-Kaye lifted from the original > `rust` branch [1]. > > Link: https://github.com/Rust-for-Linux/linux/tree/bc22545f38d74473cfef3e9fd65432733435b79f [1] > Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org> > --- > Changes since v2 [1]: > - use `SyncUnsafeCell` rather than `static mut` and simplify parameter access > - remove `Display` bound from `ModuleParam` > - automatically generate documentation for `PARAM_OPS_.*` > - remove `as *const _ as *mut_` phrasing > - inline parameter name in struct instantiation in `emit_params` > - move `RacyKernelParam` out of macro template > - use C string literals rather than byte string literals with explicit null > - template out `__{name}_{param_name}` in `emit_param` > - indent template in `emit_params` > - use let-else expression in `emit_params` to get rid of an indentation level > - document `expect_string_field` > - move invication of `impl_int_module_param` to be closer to macro def > - move attributes after docs in `make_param_ops` > - rename `impl_module_param` to impl_int_module_param` > - use `ty` instead of `ident` in `impl_parse_int` > - use `BStr` instead of `&str` for string manipulation > - move string parsing functions to seperate patch and add examples, fix bugs > - degrade comment about future support from doc comment to regular comment > - remove std lib path from `Sized` marker > - update documentation for `trait ModuleParam` > > Changes since v1 [2]: > - Remove support for params without values (`NOARG_ALLOWED`). > - Improve documentation for `try_from_param_arg`. > - Use prelude import. > - Refactor `try_from_param_arg` to return `Result`. > - Refactor `ParseInt::from_str` to return `Result`. > - Move C callable functions out of `ModuleParam` trait. > - Rename literal string field parser to `expect_string_field`. > - Move parameter parsing from generation to parsing stage. > - Use absolute type paths in macro code. > - Inline `kparam`and `read_func` values. > - Resolve TODO regarding alignment attributes. > - Remove unnecessary unsafe blocks in macro code. > - Improve error message for unrecognized parameter types. > - Do not use `self` receiver when reading parameter value. > - Add parameter documentation to `module!` macro. > - Use empty `enum` for parameter type. > - Use `addr_of_mut` to get address of parameter value variable. > - Enabled building of docs for for `module_param` module. > > Link: https://lore.kernel.org/rust-for-linux/20240705111455.142790-1-nmi@metaspace.dk/ [2] > Link: https://lore.kernel.org/all/20240819133345.3438739-1-nmi@metaspace.dk/ [1] > > --- > > --- > Andreas Hindborg (4): > rust: str: implement `PartialEq` for `BStr` > rust: str: implement `strip_prefix` for `BStr` > rust: str: add radix prefixed integer parsing functions > rust: add parameter support to the `module!` macro > > rust/kernel/lib.rs | 2 + > rust/kernel/module_param.rs | 238 +++++++++++++++++++++++++++++++++++++++++++ > rust/kernel/str.rs | 138 +++++++++++++++++++++++++ > rust/macros/helpers.rs | 10 ++ > rust/macros/lib.rs | 31 ++++++ > rust/macros/module.rs | 188 ++++++++++++++++++++++++++++++---- > samples/rust/rust_minimal.rs | 10 ++ > scripts/Makefile.build | 2 +- > 8 files changed, 600 insertions(+), 19 deletions(-) > --- > base-commit: fac04efc5c793dccbd07e2d59af9f90b7fc0dca4 > change-id: 20241211-module-params-v3-ae7e5c8d8b5a > > Best regards,
On Fri, Dec 13, 2024 at 1:24 PM Andreas Hindborg <a.hindborg@kernel.org> wrote: > > "Greg KH" <gregkh@linuxfoundation.org> writes: > > > On Fri, Dec 13, 2024 at 12:30:45PM +0100, Andreas Hindborg wrote: > >> This series extends the `module!` macro with support module parameters. > > > > Eeek, why? > > > > Module parameters are from the 1990's, back when we had no idea what we > > were doing and thought that a simple "one variable for a driver that > > controls multiple devices" was somehow a valid solution :) > > > > Please only really add module parameters if you can prove that you > > actually need a module parameter. > > I really need module parameters to make rust null block feature > compatible with C null block. Instead of providing module parameters to Rust code, you could implement that part of Rust nullblk in C. That way, you discourage future Rust drivers from using module parameters without making it impossible to have them in Rust nullblk. Alice
Alice Ryhl <aliceryhl@google.com> writes: > On Fri, Dec 13, 2024 at 1:24 PM Andreas Hindborg <a.hindborg@kernel.org> wrote: >> >> "Greg KH" <gregkh@linuxfoundation.org> writes: >> >> > On Fri, Dec 13, 2024 at 12:30:45PM +0100, Andreas Hindborg wrote: >> >> This series extends the `module!` macro with support module parameters. >> > >> > Eeek, why? >> > >> > Module parameters are from the 1990's, back when we had no idea what we >> > were doing and thought that a simple "one variable for a driver that >> > controls multiple devices" was somehow a valid solution :) >> > >> > Please only really add module parameters if you can prove that you >> > actually need a module parameter. >> >> I really need module parameters to make rust null block feature >> compatible with C null block. > > Instead of providing module parameters to Rust code, you could > implement that part of Rust nullblk in C. That way, you discourage > future Rust drivers from using module parameters without making it > impossible to have them in Rust nullblk. If the opinion of the community is really to phase out module parameters for all new drivers (is it?), I can maybe move the code in question into the Rust null_blk driver. I was kind of looking forward to having zero unsafe code in the driver though. On the other hand, rust null block might not be the only "rewrite in rust and keep compatibility" project to ever see the light of day. Best regards, Andreas Hindborg
On Fri, Dec 13, 2024 at 01:24:42PM +0100, Andreas Hindborg wrote: > "Greg KH" <gregkh@linuxfoundation.org> writes: > > > On Fri, Dec 13, 2024 at 12:30:45PM +0100, Andreas Hindborg wrote: > >> This series extends the `module!` macro with support module parameters. > > > > Eeek, why? > > > > Module parameters are from the 1990's, back when we had no idea what we > > were doing and thought that a simple "one variable for a driver that > > controls multiple devices" was somehow a valid solution :) > > > > Please only really add module parameters if you can prove that you > > actually need a module parameter. > > I really need module parameters to make rust null block feature > compatible with C null block. Is that a requirement? That wasn't documented here :( You should have put the user of these apis in the series as you have that code already in the tree, right? > Let's not block interfacing parts of the kernel because we decided that > the way we (well not me, I was not around) did things in the 80's was > less than stellar. I mean, we would get nowhere. On the contrary, if we don't learn from our past mistakes, we will constantly keep making them and prevent others from "doing the right thing" by default. I would strongly prefer that any driver not have any module parameters at all, as drivers don't work properly that way (again, they need to handle multiple devices, which does not work for a module parameter.) That's why we created sysfs, configfs, and lots of other things, to learn from our past mistakes. thanks, greg k-h
"Greg KH" <gregkh@linuxfoundation.org> writes: > On Fri, Dec 13, 2024 at 01:24:42PM +0100, Andreas Hindborg wrote: >> "Greg KH" <gregkh@linuxfoundation.org> writes: >> >> > On Fri, Dec 13, 2024 at 12:30:45PM +0100, Andreas Hindborg wrote: >> >> This series extends the `module!` macro with support module parameters. >> > >> > Eeek, why? >> > >> > Module parameters are from the 1990's, back when we had no idea what we >> > were doing and thought that a simple "one variable for a driver that >> > controls multiple devices" was somehow a valid solution :) >> > >> > Please only really add module parameters if you can prove that you >> > actually need a module parameter. >> >> I really need module parameters to make rust null block feature >> compatible with C null block. > > Is that a requirement? That wasn't documented here :( > > You should have put the user of these apis in the series as you have > that code already in the tree, right? Sorry, my mistake. I'm trying to build a rust implementation of C null_blk, and one the bits I need for that is module parameters. > >> Let's not block interfacing parts of the kernel because we decided that >> the way we (well not me, I was not around) did things in the 80's was >> less than stellar. I mean, we would get nowhere. > > On the contrary, if we don't learn from our past mistakes, we will > constantly keep making them and prevent others from "doing the right > thing" by default. > > I would strongly prefer that any driver not have any module parameters > at all, as drivers don't work properly that way (again, they need to > handle multiple devices, which does not work for a module parameter.) > > That's why we created sysfs, configfs, and lots of other things, to > learn from our past mistakes. OK. I understand. It makes sense even :) I wish I knew that this was a thing before I spent spare cycles fixing this up for v3 though. I'm not getting a clear reading on the following, perhaps you can clarify: - Is the community aligned on dropping module parameters for all new drivers? - If so, was this decided upon at some point or is this a fluid decision that is just manifesting now? - Does this ban of module parameters also cover cases where backwards compatibility is desirable? - Can we merge this so I can move forward at my current projected course, or should I plan on dealing with not having this available? Best regards, Andreas Hindborg
On Fri, Dec 13, 2024 at 04:38:30PM +0100, Andreas Hindborg wrote: > "Greg KH" <gregkh@linuxfoundation.org> writes: > > On Fri, Dec 13, 2024 at 01:24:42PM +0100, Andreas Hindborg wrote: > >> "Greg KH" <gregkh@linuxfoundation.org> writes: > >> > On Fri, Dec 13, 2024 at 12:30:45PM +0100, Andreas Hindborg wrote: > I'm not getting a clear reading on the following, perhaps you can > clarify: > > - Is the community aligned on dropping module parameters for all new > drivers? > - If so, was this decided upon at some point or is this a fluid > decision that is just manifesting now? It's something that I've been saying in review comments of drivers for many many years now. Again, it was one of the main reasons we created configfs and sysfs all those decades ago, because module parameters just do not work properly for drivers in almost all cases. > - Does this ban of module parameters also cover cases where backwards > compatibility is desirable? No, we don't break existing kernel features, but if you are writing a new driver, don't add them and then there's no compatibility issue. We don't normally allow "rewrites" of drivers, but if we do, yes, you would have to implement the old features if needed. As you just seem to want to write an "example" block driver, no need to add the module option there, just do it right this time in how to properly configure things. > - Can we merge this so I can move forward at my current projected > course, or should I plan on dealing with not having this available? We generally do not want to merge apis without any real users, as it's hard to justify them, right? Also, we don't even know if they work properly or not. thanks, greg k-h
On Fri, Dec 13, 2024 at 01:28:27PM +0100, Andreas Hindborg wrote: > > Hi Luis, Petr, Sami, Dani, > > I just noticed that I failed in email and forgot to add you to the > recipient list of this series. Do you want a resend, or is this > sufficient? If the patches didn't go to linux-modules, then yes, a resend would be good so that others subscribed to that list can also review. You can also wait for a v2. Up to you. I won't read it until I get it on my inbox. Luis
"Greg KH" <gregkh@linuxfoundation.org> writes: > On Fri, Dec 13, 2024 at 04:38:30PM +0100, Andreas Hindborg wrote: >> "Greg KH" <gregkh@linuxfoundation.org> writes: >> > On Fri, Dec 13, 2024 at 01:24:42PM +0100, Andreas Hindborg wrote: >> >> "Greg KH" <gregkh@linuxfoundation.org> writes: >> >> > On Fri, Dec 13, 2024 at 12:30:45PM +0100, Andreas Hindborg wrote: >> I'm not getting a clear reading on the following, perhaps you can >> clarify: >> >> - Is the community aligned on dropping module parameters for all new >> drivers? >> - If so, was this decided upon at some point or is this a fluid >> decision that is just manifesting now? > > It's something that I've been saying in review comments of drivers for > many many years now. Again, it was one of the main reasons we created > configfs and sysfs all those decades ago, because module parameters just > do not work properly for drivers in almost all cases. Thinking a bit about this - are you sure there are no situations where module parameters is actually the right choice? I could imagine deeply embedded deployments, potentially with all required modules linked in to a static kernel image. It is probably desirable to be able to pass configuration to the modules via the kernel command line in this situation. If not for configuration in the field, then for a development situation. Surely there are also situations in regular PC setups where it is desirable to pass configuration data to a module, so that it is available at load time. With configfs, module initialization becomes a two stage process of first loading and then configuring. That is not always what you would want. Since module parameters actually do show up in sysfs as writable entries, when the right flags are passed, they do feel very similar to sysfs/configfs for simple use cases. What are the reasons they should not be usable for these simple use cases? > >> - Does this ban of module parameters also cover cases where backwards >> compatibility is desirable? > > No, we don't break existing kernel features, but if you are writing a > new driver, don't add them and then there's no compatibility issue. > > We don't normally allow "rewrites" of drivers, but if we do, yes, you > would have to implement the old features if needed. > > As you just seem to want to write an "example" block driver, no need to > add the module option there, just do it right this time in how to > properly configure things. > >> - Can we merge this so I can move forward at my current projected >> course, or should I plan on dealing with not having this available? > > We generally do not want to merge apis without any real users, as it's > hard to justify them, right? Also, we don't even know if they work > properly or not. While null_blk is just a piece of testing infrastructure that you would not deploy in a production environment, it is still a "real user". I am sure we can agree on the importance of testing. The exercise I am undertaking is to produce a drop in replacement of the existing C null_blk driver. If all goes well, we are considering to swap the C implementation for the Rust implementation in X number of years. Granted - a lot of things have to fall into place for that to happen, but that is the plan. This plan does not really work well if the two modules do not have the same interface. I understand that you would like to phase out module parameters, but I don't think blocking their use from Rust is the right way to go about that task. If you really feel that module parameters have no place in new drivers, I would suggest that to be part of review process for each individual new driver - not at the stage of enabling module parameters for Rust in general. Best regards, Andreas Hindborg
On Mon, Dec 16, 2024 at 10:51:53AM +0100, Andreas Hindborg wrote: > "Greg KH" <gregkh@linuxfoundation.org> writes: > > > On Fri, Dec 13, 2024 at 04:38:30PM +0100, Andreas Hindborg wrote: > >> "Greg KH" <gregkh@linuxfoundation.org> writes: > >> > On Fri, Dec 13, 2024 at 01:24:42PM +0100, Andreas Hindborg wrote: > >> >> "Greg KH" <gregkh@linuxfoundation.org> writes: > >> >> > On Fri, Dec 13, 2024 at 12:30:45PM +0100, Andreas Hindborg wrote: > >> I'm not getting a clear reading on the following, perhaps you can > >> clarify: > >> > >> - Is the community aligned on dropping module parameters for all new > >> drivers? > >> - If so, was this decided upon at some point or is this a fluid > >> decision that is just manifesting now? > > > > It's something that I've been saying in review comments of drivers for > > many many years now. Again, it was one of the main reasons we created > > configfs and sysfs all those decades ago, because module parameters just > > do not work properly for drivers in almost all cases. > > Thinking a bit about this - are you sure there are no situations where > module parameters is actually the right choice? I could imagine deeply > embedded deployments, potentially with all required modules linked in to > a static kernel image. It is probably desirable to be able to pass > configuration to the modules via the kernel command line in this > situation. If not for configuration in the field, then for a development > situation. I'm not saying "no situations at all", I'm saying "almost no situations need a module parameter, and almost NO driver needs one." That does not mean "none". > Surely there are also situations in regular PC setups where it is > desirable to pass configuration data to a module, so that it is > available at load time. With configfs, module initialization becomes a > two stage process of first loading and then configuring. That is not > always what you would want. "regular" PC setups do not want module parameters either, they should all be dynamic busses and everything should "just work". > Since module parameters actually do show up in sysfs as writable > entries, when the right flags are passed, they do feel very similar to > sysfs/configfs for simple use cases. What are the reasons they should > not be usable for these simple use cases? Because they are almost never a good idea for a driver because drivers can control multiple devices and how do you show that in a single value? For non-drivers, it's a different issue, but that's not what you are considering here :) > >> - Does this ban of module parameters also cover cases where backwards > >> compatibility is desirable? > > > > No, we don't break existing kernel features, but if you are writing a > > new driver, don't add them and then there's no compatibility issue. > > > > We don't normally allow "rewrites" of drivers, but if we do, yes, you > > would have to implement the old features if needed. > > > > As you just seem to want to write an "example" block driver, no need to > > add the module option there, just do it right this time in how to > > properly configure things. > > > >> - Can we merge this so I can move forward at my current projected > >> course, or should I plan on dealing with not having this available? > > > > We generally do not want to merge apis without any real users, as it's > > hard to justify them, right? Also, we don't even know if they work > > properly or not. > > While null_blk is just a piece of testing infrastructure that you would > not deploy in a production environment, it is still a "real user". I am > sure we can agree on the importance of testing. > > The exercise I am undertaking is to produce a drop in replacement of the > existing C null_blk driver. If all goes well, we are considering to swap > the C implementation for the Rust implementation in X number of years. > Granted - a lot of things have to fall into place for that to happen, > but that is the plan. This plan does not really work well if the two > modules do not have the same interface. Why do you have to have the same interface? Why not do it "properly" and make it use configfs that way you can have multiple devices and test them all at the same time? As this is "just" a testing driver, there should not be any need to keep the same user/kernel api for setting things up, right? Again, don't make the mistakes we have in the past, drivers should NOT be using module parameters. > I understand that you would like to phase out module parameters, but I > don't think blocking their use from Rust is the right way to go about > that task. If you really feel that module parameters have no place in > new drivers, I would suggest that to be part of review process for each > individual new driver - not at the stage of enabling module parameters > for Rust in general. I'm saying that module parameters do NOT belong in a driver, which is what you are wanting to do here. And as for adding new apis, please only do so when you have a real user, I don't see a real user for module parameters in rust just yet. If that changes, I'll reconsider my stance :) thanks, greg k-h
On Mon, Dec 16, 2024 at 01:14:16PM +0100, Greg KH wrote: > On Mon, Dec 16, 2024 at 10:51:53AM +0100, Andreas Hindborg wrote: > > The exercise I am undertaking is to produce a drop in replacement of the > > existing C null_blk driver. If all goes well, we are considering to swap > > the C implementation for the Rust implementation in X number of years. > > Granted - a lot of things have to fall into place for that to happen, > > but that is the plan. This plan does not really work well if the two > > modules do not have the same interface. > > Why do you have to have the same interface? Why not do it "properly" > and make it use configfs that way you can have multiple devices and test > them all at the same time? Wait, null_blk already uses configfs, so just use that! I'd like to see the rust bindings for that api as that will be needed by lots of code. thanks, greg k-h
"Greg KH" <gregkh@linuxfoundation.org> writes: > On Mon, Dec 16, 2024 at 10:51:53AM +0100, Andreas Hindborg wrote: >> "Greg KH" <gregkh@linuxfoundation.org> writes: >> >> > On Fri, Dec 13, 2024 at 04:38:30PM +0100, Andreas Hindborg wrote: [cut] >> > >> >> - Can we merge this so I can move forward at my current projected >> >> course, or should I plan on dealing with not having this available? >> > >> > We generally do not want to merge apis without any real users, as it's >> > hard to justify them, right? Also, we don't even know if they work >> > properly or not. >> >> While null_blk is just a piece of testing infrastructure that you would >> not deploy in a production environment, it is still a "real user". I am >> sure we can agree on the importance of testing. >> >> The exercise I am undertaking is to produce a drop in replacement of the >> existing C null_blk driver. If all goes well, we are considering to swap >> the C implementation for the Rust implementation in X number of years. >> Granted - a lot of things have to fall into place for that to happen, >> but that is the plan. This plan does not really work well if the two >> modules do not have the same interface. > > Why do you have to have the same interface? Why not do it "properly" > and make it use configfs that way you can have multiple devices and test > them all at the same time? > > As this is "just" a testing driver, there should not be any need to keep > the same user/kernel api for setting things up, right? Because that would break all the users that use the old interface. > > Again, don't make the mistakes we have in the past, drivers should NOT > be using module parameters. > >> I understand that you would like to phase out module parameters, but I >> don't think blocking their use from Rust is the right way to go about >> that task. If you really feel that module parameters have no place in >> new drivers, I would suggest that to be part of review process for each >> individual new driver - not at the stage of enabling module parameters >> for Rust in general. > > I'm saying that module parameters do NOT belong in a driver, which is > what you are wanting to do here. And as for adding new apis, please > only do so when you have a real user, I don't see a real user for module > parameters in rust just yet. If that changes, I'll reconsider my stance :) I guess we disagree about what is "real" and what is not. In my view, null_blk is real, it is used by real people to do real work. They get real annoyed when the interface for their real tools change - thus making it more difficult to do this experiment. Best regards, Andreas Hindborg
"Greg KH" <gregkh@linuxfoundation.org> writes: > On Mon, Dec 16, 2024 at 01:14:16PM +0100, Greg KH wrote: >> On Mon, Dec 16, 2024 at 10:51:53AM +0100, Andreas Hindborg wrote: >> > The exercise I am undertaking is to produce a drop in replacement of the >> > existing C null_blk driver. If all goes well, we are considering to swap >> > the C implementation for the Rust implementation in X number of years. >> > Granted - a lot of things have to fall into place for that to happen, >> > but that is the plan. This plan does not really work well if the two >> > modules do not have the same interface. >> >> Why do you have to have the same interface? Why not do it "properly" >> and make it use configfs that way you can have multiple devices and test >> them all at the same time? > > Wait, null_blk already uses configfs, so just use that! I'd like to see > the rust bindings for that api as that will be needed by lots of code. I intend to support both. Best regards, Andreas Hindborg
On 12/16/24 6:02 AM, Andreas Hindborg wrote: >>> I understand that you would like to phase out module parameters, but I >>> don't think blocking their use from Rust is the right way to go about >>> that task. If you really feel that module parameters have no place in >>> new drivers, I would suggest that to be part of review process for each >>> individual new driver - not at the stage of enabling module parameters >>> for Rust in general. >> >> I'm saying that module parameters do NOT belong in a driver, which is >> what you are wanting to do here. And as for adding new apis, please >> only do so when you have a real user, I don't see a real user for module >> parameters in rust just yet. If that changes, I'll reconsider my stance :) > > I guess we disagree about what is "real" and what is not. > > In my view, null_blk is real, it is used by real people to do real work. > They get real annoyed when the interface for their real tools change - > thus making it more difficult to do this experiment. I'd have to agree with that - yes, null_blk doesn't host any real applications, but it is the backbone of a lot of testing that blktests and others do. Hence it's very real in that sense, and the rust version of null_blk should provide and mimic how the C version works for ease of testing. If this was a new driver where no prior art exists in terms of users and API, then I'd certainly agree with Greg. But that's not the case here.
On Mon, Dec 16, 2024 at 07:43:12AM -0700, Jens Axboe wrote: > On 12/16/24 6:02 AM, Andreas Hindborg wrote: > >>> I understand that you would like to phase out module parameters, but I > >>> don't think blocking their use from Rust is the right way to go about > >>> that task. If you really feel that module parameters have no place in > >>> new drivers, I would suggest that to be part of review process for each > >>> individual new driver - not at the stage of enabling module parameters > >>> for Rust in general. > >> > >> I'm saying that module parameters do NOT belong in a driver, which is > >> what you are wanting to do here. And as for adding new apis, please > >> only do so when you have a real user, I don't see a real user for module > >> parameters in rust just yet. If that changes, I'll reconsider my stance :) > > > > I guess we disagree about what is "real" and what is not. > > > > In my view, null_blk is real, it is used by real people to do real work. > > They get real annoyed when the interface for their real tools change - > > thus making it more difficult to do this experiment. > > I'd have to agree with that - yes, null_blk doesn't host any real > applications, but it is the backbone of a lot of testing that blktests > and others do. Hence it's very real in that sense, and the rust version > of null_blk should provide and mimic how the C version works for ease of > testing. > > If this was a new driver where no prior art exists in terms of users and > API, then I'd certainly agree with Greg. But that's not the case here. Ok, so are you going to drop the C version and go with the rust version if it shows up? Surely you don't want duplicate drivers for the same thing in the tree, right? thanks, greg k-h
On 12/16/24 8:03 AM, Greg KH wrote: > On Mon, Dec 16, 2024 at 07:43:12AM -0700, Jens Axboe wrote: >> On 12/16/24 6:02 AM, Andreas Hindborg wrote: >>>>> I understand that you would like to phase out module parameters, but I >>>>> don't think blocking their use from Rust is the right way to go about >>>>> that task. If you really feel that module parameters have no place in >>>>> new drivers, I would suggest that to be part of review process for each >>>>> individual new driver - not at the stage of enabling module parameters >>>>> for Rust in general. >>>> >>>> I'm saying that module parameters do NOT belong in a driver, which is >>>> what you are wanting to do here. And as for adding new apis, please >>>> only do so when you have a real user, I don't see a real user for module >>>> parameters in rust just yet. If that changes, I'll reconsider my stance :) >>> >>> I guess we disagree about what is "real" and what is not. >>> >>> In my view, null_blk is real, it is used by real people to do real work. >>> They get real annoyed when the interface for their real tools change - >>> thus making it more difficult to do this experiment. >> >> I'd have to agree with that - yes, null_blk doesn't host any real >> applications, but it is the backbone of a lot of testing that blktests >> and others do. Hence it's very real in that sense, and the rust version >> of null_blk should provide and mimic how the C version works for ease of >> testing. >> >> If this was a new driver where no prior art exists in terms of users and >> API, then I'd certainly agree with Greg. But that's not the case here. > > Ok, so are you going to drop the C version and go with the rust version > if it shows up? Surely you don't want duplicate drivers for the same > thing in the tree, right? Maybe at some point? The rust version is already there, it's just very limited compared to the C version so far. The point of the rust null_blk is to build out the API so that a real driver can get implemented as well. For now, I think the plan is to just have it be the rust playground on the block side. Given that null_blk is the center piece of a lot of testing, it's not necessarily an issue to have duplicate implementations of the same driver. In fact it may be pretty useful for people coming from either side and wanting to compare implementations and how to do various things in either language. It's like an actually useful skeleton driver in that sense too. Whether one or the other will be the only one in the tree in the future depends on a lot of other things that aren't necessarily driven or decided by the rust null_blk driver.
On Mon, Dec 16, 2024 at 4:08 PM Jens Axboe <axboe@kernel.dk> wrote: > > Maybe at some point? The rust version is already there, it's just very > limited compared to the C version so far. The point of the rust null_blk > is to build out the API so that a real driver can get implemented as > well. For now, I think the plan is to just have it be the rust > playground on the block side. > > Given that null_blk is the center piece of a lot of testing, it's not > necessarily an issue to have duplicate implementations of the same > driver. In fact it may be pretty useful for people coming from either > side and wanting to compare implementations and how to do various things > in either language. It's like an actually useful skeleton driver in that > sense too. Agreed. I would suggest to consider marking it as a Rust reference driver, since it is a prime candidate for it: https://rust-for-linux.com/rust-reference-drivers That way, it is clearer that the duplication is meant to build the abstractions and temporary in the long-term. Then we can also easily track which ones are meant to be those, and Greg can get justifiably angry at you/us if the duplication isn't resolved when the right time comes... :) Cheers, Miguel
On Mon, Dec 16, 2024 at 4:39 PM Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote: > > Agreed. I would suggest to consider marking it as a Rust reference > driver, since it is a prime candidate for it: > > https://rust-for-linux.com/rust-reference-drivers > > That way, it is clearer that the duplication is meant to build the > abstractions and temporary in the long-term. > > Then we can also easily track which ones are meant to be those, and > Greg can get justifiably angry at you/us if the duplication isn't > resolved when the right time comes... :) By the way, I half-jokingly suggested this elsewhere, but we could trivially allow module parameters only for particular modules, i.e. only allow to use the `params` key here if the name matches `rnull` (or if they set a special flag or whatever). Yes, it is a hack, but it would give people pause when trying to use the feature, i.e. to think twice. And, to me, it makes sense to encode/acknowledge this kind of thing explicitly anyway. So if that would unblock this and reduce the chance of repeating mistakes of the past, then we can easily do that too. Cheers, Miguel
On Fri, Dec 13, 2024 at 01:57:59PM +0100, Andreas Hindborg wrote: > Alice Ryhl <aliceryhl@google.com> writes: > > > On Fri, Dec 13, 2024 at 1:24 PM Andreas Hindborg <a.hindborg@kernel.org> wrote: > >> > >> "Greg KH" <gregkh@linuxfoundation.org> writes: > >> > >> > On Fri, Dec 13, 2024 at 12:30:45PM +0100, Andreas Hindborg wrote: > >> >> This series extends the `module!` macro with support module parameters. > >> > > >> > Eeek, why? > >> > > >> > Module parameters are from the 1990's, back when we had no idea what we > >> > were doing and thought that a simple "one variable for a driver that > >> > controls multiple devices" was somehow a valid solution :) > >> > > >> > Please only really add module parameters if you can prove that you > >> > actually need a module parameter. > >> > >> I really need module parameters to make rust null block feature > >> compatible with C null block. > > > > Instead of providing module parameters to Rust code, you could > > implement that part of Rust nullblk in C. That way, you discourage > > future Rust drivers from using module parameters without making it > > impossible to have them in Rust nullblk. > > If the opinion of the community is really to phase out module parameters > for all new drivers (is it?), I can maybe move the code in question into > the Rust null_blk driver. > > I was kind of looking forward to having zero unsafe code in the driver > though. > > On the other hand, rust null block might not be the only "rewrite in rust and keep > compatibility" project to ever see the light of day. We still have tons of module parameters with the _unsafe annotations, because they're really convenient for debugging and testing. Yes they're hacks, yes you cannot use them for production, but they're useful. So for drivers I'd say we definitely want to keep modules parameters around, they're just too convenient compared to debugfs, especially when it's things you have to set before the driver binds to the device. -Sima
On Mon, Dec 16, 2024 at 04:48:21PM +0100, Miguel Ojeda wrote: > On Mon, Dec 16, 2024 at 4:39 PM Miguel Ojeda > <miguel.ojeda.sandonis@gmail.com> wrote: > > > > Agreed. I would suggest to consider marking it as a Rust reference > > driver, since it is a prime candidate for it: > > > > https://rust-for-linux.com/rust-reference-drivers > > > > That way, it is clearer that the duplication is meant to build the > > abstractions and temporary in the long-term. > > > > Then we can also easily track which ones are meant to be those, and > > Greg can get justifiably angry at you/us if the duplication isn't > > resolved when the right time comes... :) > > By the way, I half-jokingly suggested this elsewhere, but we could > trivially allow module parameters only for particular modules, i.e. > only allow to use the `params` key here if the name matches `rnull` > (or if they set a special flag or whatever). > > Yes, it is a hack, but it would give people pause when trying to use > the feature, i.e. to think twice. And, to me, it makes sense to > encode/acknowledge this kind of thing explicitly anyway. > > So if that would unblock this and reduce the chance of repeating > mistakes of the past, then we can easily do that too. This seems like a great idea. An allowlist of drivers that are allowed to use module parameters would encourage *new* drivers to not use them, and that allowlist can have a comment atop it saying "Only add your driver to this list if it needs to maintain an interface compatible with an existing driver in order to avoid breaking userspace. Otherwise, use configfs, sysfs, debugfs, or something else other than module parameters." I wonder if we can implement such an allowlist for C modules, too. :)
This series extends the `module!` macro with support module parameters. It also adds some string to integer parsing functions and updates `BStr` with a method to strip a string prefix. This series stated out as code by Adam Bratschi-Kaye lifted from the original `rust` branch [1]. Link: https://github.com/Rust-for-Linux/linux/tree/bc22545f38d74473cfef3e9fd65432733435b79f [1] Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org> --- Changes since v2 [1]: - use `SyncUnsafeCell` rather than `static mut` and simplify parameter access - remove `Display` bound from `ModuleParam` - automatically generate documentation for `PARAM_OPS_.*` - remove `as *const _ as *mut_` phrasing - inline parameter name in struct instantiation in `emit_params` - move `RacyKernelParam` out of macro template - use C string literals rather than byte string literals with explicit null - template out `__{name}_{param_name}` in `emit_param` - indent template in `emit_params` - use let-else expression in `emit_params` to get rid of an indentation level - document `expect_string_field` - move invication of `impl_int_module_param` to be closer to macro def - move attributes after docs in `make_param_ops` - rename `impl_module_param` to impl_int_module_param` - use `ty` instead of `ident` in `impl_parse_int` - use `BStr` instead of `&str` for string manipulation - move string parsing functions to seperate patch and add examples, fix bugs - degrade comment about future support from doc comment to regular comment - remove std lib path from `Sized` marker - update documentation for `trait ModuleParam` Changes since v1 [2]: - Remove support for params without values (`NOARG_ALLOWED`). - Improve documentation for `try_from_param_arg`. - Use prelude import. - Refactor `try_from_param_arg` to return `Result`. - Refactor `ParseInt::from_str` to return `Result`. - Move C callable functions out of `ModuleParam` trait. - Rename literal string field parser to `expect_string_field`. - Move parameter parsing from generation to parsing stage. - Use absolute type paths in macro code. - Inline `kparam`and `read_func` values. - Resolve TODO regarding alignment attributes. - Remove unnecessary unsafe blocks in macro code. - Improve error message for unrecognized parameter types. - Do not use `self` receiver when reading parameter value. - Add parameter documentation to `module!` macro. - Use empty `enum` for parameter type. - Use `addr_of_mut` to get address of parameter value variable. - Enabled building of docs for for `module_param` module. Link: https://lore.kernel.org/rust-for-linux/20240705111455.142790-1-nmi@metaspace.dk/ [2] Link: https://lore.kernel.org/all/20240819133345.3438739-1-nmi@metaspace.dk/ [1] --- --- Andreas Hindborg (4): rust: str: implement `PartialEq` for `BStr` rust: str: implement `strip_prefix` for `BStr` rust: str: add radix prefixed integer parsing functions rust: add parameter support to the `module!` macro rust/kernel/lib.rs | 2 + rust/kernel/module_param.rs | 238 +++++++++++++++++++++++++++++++++++++++++++ rust/kernel/str.rs | 138 +++++++++++++++++++++++++ rust/macros/helpers.rs | 10 ++ rust/macros/lib.rs | 31 ++++++ rust/macros/module.rs | 188 ++++++++++++++++++++++++++++++---- samples/rust/rust_minimal.rs | 10 ++ scripts/Makefile.build | 2 +- 8 files changed, 600 insertions(+), 19 deletions(-) --- base-commit: fac04efc5c793dccbd07e2d59af9f90b7fc0dca4 change-id: 20241211-module-params-v3-ae7e5c8d8b5a Best regards,