Benno Lossin
2025-Jul-03 20:36 UTC
[PATCH v13 2/5] rust: support formatting of foreign types
On Thu Jul 3, 2025 at 8:55 PM CEST, Tamir Duberstein wrote:> On Thu, Jul 3, 2025 at 11:08?AM Benno Lossin <lossin at kernel.org> wrote: >> On Thu Jul 3, 2025 at 3:55 PM CEST, Tamir Duberstein wrote: >> > On Thu, Jul 3, 2025 at 5:32?AM Benno Lossin <lossin at kernel.org> wrote: >> >> On Tue Jul 1, 2025 at 6:49 PM CEST, Tamir Duberstein wrote: >> >> > Introduce a `fmt!` macro which wraps all arguments in >> >> > `kernel::fmt::Adapter` and a `kernel::fmt::Display` trait. This enables >> >> > formatting of foreign types (like `core::ffi::CStr`) that do not >> >> > implement `core::fmt::Display` due to concerns around lossy conversions which >> >> > do not apply in the kernel. >> >> > >> >> > Replace all direct calls to `format_args!` with `fmt!`. >> >> > >> >> > Replace all implementations of `core::fmt::Display` with implementations >> >> > of `kernel::fmt::Display`. >> >> > >> >> > Suggested-by: Alice Ryhl <aliceryhl at google.com> >> >> > Link: https://rust-for-linux.zulipchat.com/#narrow/channel/288089-General/topic/Custom.20formatting/with/516476467 >> >> > Acked-by: Greg Kroah-Hartman <gregkh at linuxfoundation.org> >> >> > Reviewed-by: Alice Ryhl <aliceryhl at google.com> >> >> > Signed-off-by: Tamir Duberstein <tamird at gmail.com> >> >> > --- >> >> > drivers/block/rnull.rs | 2 +- >> >> > drivers/gpu/nova-core/gpu.rs | 4 +- >> >> > rust/kernel/block/mq.rs | 2 +- >> >> > rust/kernel/device.rs | 2 +- >> >> > rust/kernel/fmt.rs | 89 +++++++++++++++++++++++++++++++++++++++ >> >> > rust/kernel/kunit.rs | 6 +-- >> >> > rust/kernel/lib.rs | 1 + >> >> > rust/kernel/prelude.rs | 3 +- >> >> > rust/kernel/print.rs | 4 +- >> >> > rust/kernel/seq_file.rs | 2 +- >> >> > rust/kernel/str.rs | 22 ++++------ >> >> > rust/macros/fmt.rs | 99 ++++++++++++++++++++++++++++++++++++++++++++ >> >> > rust/macros/lib.rs | 19 +++++++++ >> >> > rust/macros/quote.rs | 7 ++++ >> >> > scripts/rustdoc_test_gen.rs | 2 +- >> >> > 15 files changed, 236 insertions(+), 28 deletions(-) >> >> >> >> This would be a lot easier to review if he proc-macro and the call >> >> replacement were different patches. >> >> >> >> Also the `kernel/fmt.rs` file should be a different commit. >> > >> > Can you help me understand why? The changes you ask to be separated >> > would all be in different files, so why would separate commits make it >> > easier to review? >> >> It takes less time to go through the entire patch and give a RB. I can >> take smaller time chunks and don't have to get back into the entire >> context of the patch when I don't have 30-60min available. > > Ah, I see what you mean. Yeah, the requirement to RB the entire patch > does mean there's a benefit to smaller patches. > >> In this patch the biggest problem is the rename & addition of new >> things, maybe just adding 200 lines in those files could be okay to go >> together, see below for more. > > After implementing your suggestion of re-exporting things from > `kernel::fmt` the diffstat is > > 26 files changed, 253 insertions(+), 51 deletions(-) > > so I guess I could do all the additions in one patch, but then > *everything* else has to go in a single patch together because the > formatting macros either want core::fmt::Display or > kernel::fmt::Display; they can't work in a halfway state.I don't understand, can't you just do: * add `rust/kernel/fmt.rs`, * add `rust/macros/fmt.rs`, * change all occurrences of `core::fmt` to `kernel::fmt` and `format_args!` to `fmt!`. The last one could be split by subsystem, no? Some subsystems might interact and thus need simultaneous splitting, but there should be some independent ones.>> > I prefer to keep things in one commit because the changes are highly >> > interdependent. The proc macro doesn't make sense without >> > kernel/fmt.rs and kernel/fmt.rs is useless without the proc macro. >> >> I think that `Adapter`, the custom `Display` and their impl blocks >> don't need to be in the same commit as the proc-macro. They are related, >> but maybe someone is not well-versed in proc-macros and thus doesn't >> want to review that part. > > Sure, I guess I will split them. But as noted above: changing the > formatting macros and all the types' trait implementations has to be a > "flag day" change.See above.>> >> > +impl_fmt_adapter_forward!(Debug, LowerHex, UpperHex, Octal, Binary, Pointer, LowerExp, UpperExp); >> >> > + >> >> > +/// A copy of [`fmt::Display`] that allows us to implement it for foreign types. >> >> > +/// >> >> > +/// Types should implement this trait rather than [`fmt::Display`]. Together with the [`Adapter`] >> >> > +/// type and [`fmt!`] macro, it allows for formatting foreign types (e.g. types from core) which do >> >> > +/// not implement [`fmt::Display`] directly. >> >> > +/// >> >> > +/// [`fmt!`]: crate::prelude::fmt! >> >> > +pub trait Display { >> >> > + /// Same as [`fmt::Display::fmt`]. >> >> > + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result; >> >> > +} >> >> > + >> >> > +impl<T: ?Sized + Display> Display for &T { >> >> > + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { >> >> > + Display::fmt(*self, f) >> >> > + } >> >> > +} >> >> > + >> >> > +impl<T: ?Sized + Display> fmt::Display for Adapter<&T> { >> >> > + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { >> >> > + let Self(t) = self; >> >> > + Display::fmt(t, f) >> >> >> >> Why not `Display::fmt(&self.0, f)`? >> > >> > I like destructuring because it shows me that there's only one field. >> > With `self.0` I don't see that. >> >> And what is the benefit here? > > In general the benefit is that the method does not ignore some portion > of `Self`. A method that uses `self.0` would not provoke a compiler > error in case another field is added, while this form would.Yeah, but why would that change happen here? And even if it got another field, why would that invalidate the impl of `fn fmt`? --- Cheers, Benno
Tamir Duberstein
2025-Jul-03 22:42 UTC
[PATCH v13 2/5] rust: support formatting of foreign types
On Thu, Jul 3, 2025 at 4:36?PM Benno Lossin <lossin at kernel.org> wrote:> > On Thu Jul 3, 2025 at 8:55 PM CEST, Tamir Duberstein wrote: > > On Thu, Jul 3, 2025 at 11:08?AM Benno Lossin <lossin at kernel.org> wrote: > >> On Thu Jul 3, 2025 at 3:55 PM CEST, Tamir Duberstein wrote: > >> > On Thu, Jul 3, 2025 at 5:32?AM Benno Lossin <lossin at kernel.org> wrote: > >> >> On Tue Jul 1, 2025 at 6:49 PM CEST, Tamir Duberstein wrote: > >> >> > Introduce a `fmt!` macro which wraps all arguments in > >> >> > `kernel::fmt::Adapter` and a `kernel::fmt::Display` trait. This enables > >> >> > formatting of foreign types (like `core::ffi::CStr`) that do not > >> >> > implement `core::fmt::Display` due to concerns around lossy conversions which > >> >> > do not apply in the kernel. > >> >> > > >> >> > Replace all direct calls to `format_args!` with `fmt!`. > >> >> > > >> >> > Replace all implementations of `core::fmt::Display` with implementations > >> >> > of `kernel::fmt::Display`. > >> >> > > >> >> > Suggested-by: Alice Ryhl <aliceryhl at google.com> > >> >> > Link: https://rust-for-linux.zulipchat.com/#narrow/channel/288089-General/topic/Custom.20formatting/with/516476467 > >> >> > Acked-by: Greg Kroah-Hartman <gregkh at linuxfoundation.org> > >> >> > Reviewed-by: Alice Ryhl <aliceryhl at google.com> > >> >> > Signed-off-by: Tamir Duberstein <tamird at gmail.com> > >> >> > --- > >> >> > drivers/block/rnull.rs | 2 +- > >> >> > drivers/gpu/nova-core/gpu.rs | 4 +- > >> >> > rust/kernel/block/mq.rs | 2 +- > >> >> > rust/kernel/device.rs | 2 +- > >> >> > rust/kernel/fmt.rs | 89 +++++++++++++++++++++++++++++++++++++++ > >> >> > rust/kernel/kunit.rs | 6 +-- > >> >> > rust/kernel/lib.rs | 1 + > >> >> > rust/kernel/prelude.rs | 3 +- > >> >> > rust/kernel/print.rs | 4 +- > >> >> > rust/kernel/seq_file.rs | 2 +- > >> >> > rust/kernel/str.rs | 22 ++++------ > >> >> > rust/macros/fmt.rs | 99 ++++++++++++++++++++++++++++++++++++++++++++ > >> >> > rust/macros/lib.rs | 19 +++++++++ > >> >> > rust/macros/quote.rs | 7 ++++ > >> >> > scripts/rustdoc_test_gen.rs | 2 +- > >> >> > 15 files changed, 236 insertions(+), 28 deletions(-) > >> >> > >> >> This would be a lot easier to review if he proc-macro and the call > >> >> replacement were different patches. > >> >> > >> >> Also the `kernel/fmt.rs` file should be a different commit. > >> > > >> > Can you help me understand why? The changes you ask to be separated > >> > would all be in different files, so why would separate commits make it > >> > easier to review? > >> > >> It takes less time to go through the entire patch and give a RB. I can > >> take smaller time chunks and don't have to get back into the entire > >> context of the patch when I don't have 30-60min available. > > > > Ah, I see what you mean. Yeah, the requirement to RB the entire patch > > does mean there's a benefit to smaller patches. > > > >> In this patch the biggest problem is the rename & addition of new > >> things, maybe just adding 200 lines in those files could be okay to go > >> together, see below for more. > > > > After implementing your suggestion of re-exporting things from > > `kernel::fmt` the diffstat is > > > > 26 files changed, 253 insertions(+), 51 deletions(-) > > > > so I guess I could do all the additions in one patch, but then > > *everything* else has to go in a single patch together because the > > formatting macros either want core::fmt::Display or > > kernel::fmt::Display; they can't work in a halfway state. > > I don't understand, can't you just do: > > * add `rust/kernel/fmt.rs`, > * add `rust/macros/fmt.rs`, > * change all occurrences of `core::fmt` to `kernel::fmt` and > `format_args!` to `fmt!`.Yes, such a split could be done - I will do so in the next spin> The last one could be split by subsystem, no? Some subsystems might > interact and thus need simultaneous splitting, but there should be some > independent ones.Yes, it probably can. As you say, some subsystems might interact - the claimed benefit of doing this subsystem-by-subsystem split is that it avoids conflicts with ongoing work that will conflict with a large patch, but this is also the downside; if ongoing work changes the set of interactions between subsystems then a maintainer may find themselves unable to emit the log message they want (because one subsystem is using kernel::fmt while another is still on core::fmt).> > >> > I prefer to keep things in one commit because the changes are highly > >> > interdependent. The proc macro doesn't make sense without > >> > kernel/fmt.rs and kernel/fmt.rs is useless without the proc macro. > >> > >> I think that `Adapter`, the custom `Display` and their impl blocks > >> don't need to be in the same commit as the proc-macro. They are related, > >> but maybe someone is not well-versed in proc-macros and thus doesn't > >> want to review that part. > > > > Sure, I guess I will split them. But as noted above: changing the > > formatting macros and all the types' trait implementations has to be a > > "flag day" change. > > See above. > > >> >> > +impl_fmt_adapter_forward!(Debug, LowerHex, UpperHex, Octal, Binary, Pointer, LowerExp, UpperExp); > >> >> > + > >> >> > +/// A copy of [`fmt::Display`] that allows us to implement it for foreign types. > >> >> > +/// > >> >> > +/// Types should implement this trait rather than [`fmt::Display`]. Together with the [`Adapter`] > >> >> > +/// type and [`fmt!`] macro, it allows for formatting foreign types (e.g. types from core) which do > >> >> > +/// not implement [`fmt::Display`] directly. > >> >> > +/// > >> >> > +/// [`fmt!`]: crate::prelude::fmt! > >> >> > +pub trait Display { > >> >> > + /// Same as [`fmt::Display::fmt`]. > >> >> > + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result; > >> >> > +} > >> >> > + > >> >> > +impl<T: ?Sized + Display> Display for &T { > >> >> > + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { > >> >> > + Display::fmt(*self, f) > >> >> > + } > >> >> > +} > >> >> > + > >> >> > +impl<T: ?Sized + Display> fmt::Display for Adapter<&T> { > >> >> > + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { > >> >> > + let Self(t) = self; > >> >> > + Display::fmt(t, f) > >> >> > >> >> Why not `Display::fmt(&self.0, f)`? > >> > > >> > I like destructuring because it shows me that there's only one field. > >> > With `self.0` I don't see that. > >> > >> And what is the benefit here? > > > > In general the benefit is that the method does not ignore some portion > > of `Self`. A method that uses `self.0` would not provoke a compiler > > error in case another field is added, while this form would. > > Yeah, but why would that change happen here? And even if it got another > field, why would that invalidate the impl of `fn fmt`?I don't know, but I would rather force a person to make that decision when they add another field rather than assume that such an addition wouldn't require changes here.
Tamir Duberstein
2025-Jul-03 23:24 UTC
[PATCH v13 2/5] rust: support formatting of foreign types
On Thu, Jul 3, 2025 at 6:41?PM Tamir Duberstein <tamird at gmail.com> wrote:> > On Thu, Jul 3, 2025 at 4:36?PM Benno Lossin <lossin at kernel.org> wrote: > > > > On Thu Jul 3, 2025 at 8:55 PM CEST, Tamir Duberstein wrote: > > > On Thu, Jul 3, 2025 at 11:08?AM Benno Lossin <lossin at kernel.org> wrote: > > >> On Thu Jul 3, 2025 at 3:55 PM CEST, Tamir Duberstein wrote: > > >> > On Thu, Jul 3, 2025 at 5:32?AM Benno Lossin <lossin at kernel.org> wrote: > > >> >> On Tue Jul 1, 2025 at 6:49 PM CEST, Tamir Duberstein wrote: > > >> >> > Introduce a `fmt!` macro which wraps all arguments in > > >> >> > `kernel::fmt::Adapter` and a `kernel::fmt::Display` trait. This enables > > >> >> > formatting of foreign types (like `core::ffi::CStr`) that do not > > >> >> > implement `core::fmt::Display` due to concerns around lossy conversions which > > >> >> > do not apply in the kernel. > > >> >> > > > >> >> > Replace all direct calls to `format_args!` with `fmt!`. > > >> >> > > > >> >> > Replace all implementations of `core::fmt::Display` with implementations > > >> >> > of `kernel::fmt::Display`. > > >> >> > > > >> >> > Suggested-by: Alice Ryhl <aliceryhl at google.com> > > >> >> > Link: https://rust-for-linux.zulipchat.com/#narrow/channel/288089-General/topic/Custom.20formatting/with/516476467 > > >> >> > Acked-by: Greg Kroah-Hartman <gregkh at linuxfoundation.org> > > >> >> > Reviewed-by: Alice Ryhl <aliceryhl at google.com> > > >> >> > Signed-off-by: Tamir Duberstein <tamird at gmail.com> > > >> >> > --- > > >> >> > drivers/block/rnull.rs | 2 +- > > >> >> > drivers/gpu/nova-core/gpu.rs | 4 +- > > >> >> > rust/kernel/block/mq.rs | 2 +- > > >> >> > rust/kernel/device.rs | 2 +- > > >> >> > rust/kernel/fmt.rs | 89 +++++++++++++++++++++++++++++++++++++++ > > >> >> > rust/kernel/kunit.rs | 6 +-- > > >> >> > rust/kernel/lib.rs | 1 + > > >> >> > rust/kernel/prelude.rs | 3 +- > > >> >> > rust/kernel/print.rs | 4 +- > > >> >> > rust/kernel/seq_file.rs | 2 +- > > >> >> > rust/kernel/str.rs | 22 ++++------ > > >> >> > rust/macros/fmt.rs | 99 ++++++++++++++++++++++++++++++++++++++++++++ > > >> >> > rust/macros/lib.rs | 19 +++++++++ > > >> >> > rust/macros/quote.rs | 7 ++++ > > >> >> > scripts/rustdoc_test_gen.rs | 2 +- > > >> >> > 15 files changed, 236 insertions(+), 28 deletions(-) > > >> >> > > >> >> This would be a lot easier to review if he proc-macro and the call > > >> >> replacement were different patches. > > >> >> > > >> >> Also the `kernel/fmt.rs` file should be a different commit. > > >> > > > >> > Can you help me understand why? The changes you ask to be separated > > >> > would all be in different files, so why would separate commits make it > > >> > easier to review? > > >> > > >> It takes less time to go through the entire patch and give a RB. I can > > >> take smaller time chunks and don't have to get back into the entire > > >> context of the patch when I don't have 30-60min available. > > > > > > Ah, I see what you mean. Yeah, the requirement to RB the entire patch > > > does mean there's a benefit to smaller patches. > > > > > >> In this patch the biggest problem is the rename & addition of new > > >> things, maybe just adding 200 lines in those files could be okay to go > > >> together, see below for more. > > > > > > After implementing your suggestion of re-exporting things from > > > `kernel::fmt` the diffstat is > > > > > > 26 files changed, 253 insertions(+), 51 deletions(-) > > > > > > so I guess I could do all the additions in one patch, but then > > > *everything* else has to go in a single patch together because the > > > formatting macros either want core::fmt::Display or > > > kernel::fmt::Display; they can't work in a halfway state. > > > > I don't understand, can't you just do: > > > > * add `rust/kernel/fmt.rs`, > > * add `rust/macros/fmt.rs`, > > * change all occurrences of `core::fmt` to `kernel::fmt` and > > `format_args!` to `fmt!`. > > Yes, such a split could be done - I will do so in the next spin > > > > The last one could be split by subsystem, no? Some subsystems might > > interact and thus need simultaneous splitting, but there should be some > > independent ones. > > Yes, it probably can. As you say, some subsystems might interact - the > claimed benefit of doing this subsystem-by-subsystem split is that it > avoids conflicts with ongoing work that will conflict with a large > patch, but this is also the downside; if ongoing work changes the set > of interactions between subsystems then a maintainer may find > themselves unable to emit the log message they want (because one > subsystem is using kernel::fmt while another is still on core::fmt).I gave this a try. I ran into the problem that `format_args!` (and, after this patch, `fmt!`) is at the center of `print_macro!`, which itself underpins various other formatting macros. This means we'd have to bifurcate the formatting infrastructure to support an incremental migration. That's quite a bit of code, and likely quite a mess in the resulting git history -- and that's setting aside the toil required to figure out the correct combinations of subsystems that must migrate together.
Andrew Lunn
2025-Jul-04 07:57 UTC
[PATCH v13 2/5] rust: support formatting of foreign types
> Yes, it probably can. As you say, some subsystems might interact - the > claimed benefit of doing this subsystem-by-subsystem split is that it > avoids conflicts with ongoing work that will conflict with a large > patch, but this is also the downside; if ongoing work changes the set > of interactions between subsystems then a maintainer may find > themselves unable to emit the log message they want (because one > subsystem is using kernel::fmt while another is still on core::fmt).This sounds like an abstraction problem. As a developer, i just want an API to print stuff. I don't care about what happens underneath. Could you add an implementation of the API which uses core:fmt underneath. Get that merged. You can then convert each subsystem one by one to use the new API. Since all you are changing is the API, not the implementation, there is no compatibility issues. Then, once all users are converted to the API, you can have one patch which flips the implementation from core:fmt to kernel:fmt. It might take you three kernel cycles to get this done, but that is relatively fast for a tree wide change, which sometimes takes years. Andrew
Benno Lossin
2025-Jul-04 10:05 UTC
[PATCH v13 2/5] rust: support formatting of foreign types
On Fri Jul 4, 2025 at 12:41 AM CEST, Tamir Duberstein wrote:> On Thu, Jul 3, 2025 at 4:36?PM Benno Lossin <lossin at kernel.org> wrote: >> On Thu Jul 3, 2025 at 8:55 PM CEST, Tamir Duberstein wrote: >> > On Thu, Jul 3, 2025 at 11:08?AM Benno Lossin <lossin at kernel.org> wrote: >> >> On Thu Jul 3, 2025 at 3:55 PM CEST, Tamir Duberstein wrote: >> >> > On Thu, Jul 3, 2025 at 5:32?AM Benno Lossin <lossin at kernel.org> wrote: >> >> >> On Tue Jul 1, 2025 at 6:49 PM CEST, Tamir Duberstein wrote: >> >> >> > +impl<T: ?Sized + Display> fmt::Display for Adapter<&T> { >> >> >> > + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { >> >> >> > + let Self(t) = self; >> >> >> > + Display::fmt(t, f) >> >> >> >> >> >> Why not `Display::fmt(&self.0, f)`? >> >> > >> >> > I like destructuring because it shows me that there's only one field. >> >> > With `self.0` I don't see that. >> >> >> >> And what is the benefit here? >> > >> > In general the benefit is that the method does not ignore some portion >> > of `Self`. A method that uses `self.0` would not provoke a compiler >> > error in case another field is added, while this form would. >> >> Yeah, but why would that change happen here? And even if it got another >> field, why would that invalidate the impl of `fn fmt`? > > I don't know, but I would rather force a person to make that decision > when they add another field rather than assume that such an addition > wouldn't require changes here.I don't think so. If this were in another file, then destructuring might make sense if the struct could conceivably get more fields in the future **and** it if the other file relied on there only being one field (or if it *had* to be changed when there was a field added). This isn't the case here so it's just unnecessary noise. --- Cheers, Benno