[2/3] rust: macros: allow generic parameter default values in `#[pin_data]`
Commit Message
This patch adds compatibilty for generic parameters defaults by using
the newly introduced `decl_generics` instead of the `impl_generics`.
Signed-off-by: Benno Lossin <benno.lossin@proton.me>
---
rust/kernel/init/macros.rs | 19 ++++++++++++++++++-
rust/macros/pin_data.rs | 3 ++-
2 files changed, 20 insertions(+), 2 deletions(-)
Comments
On Sat, Nov 25, 2023 at 12:51:09PM +0000, Benno Lossin wrote:
> This patch adds compatibilty for generic parameters defaults by using
> the newly introduced `decl_generics` instead of the `impl_generics`.
This says _what_ is happening here, but not _why_ this is needed at all.
Try taking a look a the kernel documentation for how to write a good
changelog text to make this much better. It's often times the most
difficult portion of making a kernel patch, the code is easy, writing a
summary of why everyone else should agree that this code is acceptable
is hard.
good luck!
greg k-h
On 25.11.23 15:26, Greg KH wrote:
> On Sat, Nov 25, 2023 at 12:51:09PM +0000, Benno Lossin wrote:
>> This patch adds compatibilty for generic parameters defaults by using
>> the newly introduced `decl_generics` instead of the `impl_generics`.
>
> This says _what_ is happening here, but not _why_ this is needed at all.
>
> Try taking a look a the kernel documentation for how to write a good
> changelog text to make this much better. It's often times the most
> difficult portion of making a kernel patch, the code is easy, writing a
> summary of why everyone else should agree that this code is acceptable
> is hard.
The reason is hidden in the third patch. Without this, the `#[pin_data]
macro would not allow specifying const generic parameter default values
and instead emit a compile error. I will add this to v2.
On Sat, Nov 25, 2023 at 03:02:00PM +0000, Benno Lossin wrote:
> On 25.11.23 15:26, Greg KH wrote:
> > On Sat, Nov 25, 2023 at 12:51:09PM +0000, Benno Lossin wrote:
> >> This patch adds compatibilty for generic parameters defaults by using
> >> the newly introduced `decl_generics` instead of the `impl_generics`.
> >
> > This says _what_ is happening here, but not _why_ this is needed at all.
> >
> > Try taking a look a the kernel documentation for how to write a good
> > changelog text to make this much better. It's often times the most
> > difficult portion of making a kernel patch, the code is easy, writing a
> > summary of why everyone else should agree that this code is acceptable
> > is hard.
>
> The reason is hidden in the third patch.
Please do not hide things, patches need to be stand-alone and
understandable that way, otherwise they will be rejected as no one can
understand why they would be needed.
> Without this, the `#[pin_data]
> macro would not allow specifying const generic parameter default values
> and instead emit a compile error.
That's nice, but it still doesn't tell me _why_ this is needed. Why
would I want any generic paramter default values at all? Who needs any
of this? What will it be used for? What does it actually do?
thanks,
greg k-h
On 25.11.23 16:10, Greg KH wrote:
> On Sat, Nov 25, 2023 at 03:02:00PM +0000, Benno Lossin wrote:
>> On 25.11.23 15:26, Greg KH wrote:
>>> On Sat, Nov 25, 2023 at 12:51:09PM +0000, Benno Lossin wrote:
>>>> This patch adds compatibilty for generic parameters defaults by using
>>>> the newly introduced `decl_generics` instead of the `impl_generics`.
>>>
>>> This says _what_ is happening here, but not _why_ this is needed at all.
>>>
>>> Try taking a look a the kernel documentation for how to write a good
>>> changelog text to make this much better. It's often times the most
>>> difficult portion of making a kernel patch, the code is easy, writing a
>>> summary of why everyone else should agree that this code is acceptable
>>> is hard.
>>
>> The reason is hidden in the third patch.
>
> Please do not hide things, patches need to be stand-alone and
> understandable that way, otherwise they will be rejected as no one can
> understand why they would be needed.
This was not my intention, I just realized this due to your question. I
wanted to point you to the third patch (which for some reason sadly does
not have the correct In-Reply-To header). Since that might help you
understand some of the context.
>> Without this, the `#[pin_data]
>> macro would not allow specifying const generic parameter default values
>> and instead emit a compile error.
>
> That's nice, but it still doesn't tell me _why_ this is needed. Why
> would I want any generic paramter default values at all? Who needs any
> of this? What will it be used for? What does it actually do?
`#[pin_data]` is a proc-macro that one can put on any struct to make the
pin-init API available for use with that struct. Since e.g. mutexes are
initialized using the pin-init API, you have to do this for anything
that contains a mutex.
This macro should be compatible with any struct definition even with
ones that have const generic parameter defaults. This was an oversight
in the original design, as it does not support that, since the proc
macro parsing cannot handle the `=` character.
The short answer for why one would want to have const generic parameter
defaults is that the language supports it. And since there is nothing
that prevents `#[pin_data]` to be implemented for such structs, we
should it do it.
Rust generally aims to make all features compatible
with each other and we would like to do the same for our
libraries/customized features.
The longer answer is a concrete example of a usecase for const generic
parameter defaults: the `Work<T, ID>` struct of the workqueue bindings.
The `ID` parameter is used to identify multiple instances of `Work`
within the same struct. But if you only intend to have a single `Work`
struct embedded in your struct, then there is no need to distinguish it
from something else (after all there is only one) and therefore we want
people to just write `Work<T>`. This is where the author of
`Work<T, ID>` can write:
struct Work<T, const ID: usize = 0> {
// ...
}
But the `= 0` syntax is currently not supported by `#[pin_data]`.
On Sat, Nov 25, 2023 at 03:26:02PM +0000, Benno Lossin wrote:
> On 25.11.23 16:10, Greg KH wrote:
> > On Sat, Nov 25, 2023 at 03:02:00PM +0000, Benno Lossin wrote:
> >> On 25.11.23 15:26, Greg KH wrote:
> >>> On Sat, Nov 25, 2023 at 12:51:09PM +0000, Benno Lossin wrote:
> >>>> This patch adds compatibilty for generic parameters defaults by using
> >>>> the newly introduced `decl_generics` instead of the `impl_generics`.
> >>>
> >>> This says _what_ is happening here, but not _why_ this is needed at all.
> >>>
> >>> Try taking a look a the kernel documentation for how to write a good
> >>> changelog text to make this much better. It's often times the most
> >>> difficult portion of making a kernel patch, the code is easy, writing a
> >>> summary of why everyone else should agree that this code is acceptable
> >>> is hard.
> >>
> >> The reason is hidden in the third patch.
> >
> > Please do not hide things, patches need to be stand-alone and
> > understandable that way, otherwise they will be rejected as no one can
> > understand why they would be needed.
>
> This was not my intention, I just realized this due to your question. I
> wanted to point you to the third patch (which for some reason sadly does
> not have the correct In-Reply-To header). Since that might help you
> understand some of the context.
Again, changes need to be understood on their own, so provide some hints
as to _why_ this is needed.
> >> Without this, the `#[pin_data]
> >> macro would not allow specifying const generic parameter default values
> >> and instead emit a compile error.
> >
> > That's nice, but it still doesn't tell me _why_ this is needed. Why
> > would I want any generic paramter default values at all? Who needs any
> > of this? What will it be used for? What does it actually do?
>
> `#[pin_data]` is a proc-macro that one can put on any struct to make the
> pin-init API available for use with that struct. Since e.g. mutexes are
> initialized using the pin-init API, you have to do this for anything
> that contains a mutex.
> This macro should be compatible with any struct definition even with
> ones that have const generic parameter defaults. This was an oversight
> in the original design, as it does not support that, since the proc
> macro parsing cannot handle the `=` character.
>
> The short answer for why one would want to have const generic parameter
> defaults is that the language supports it.
Wait, no, that's not what we do in the kernel. We only add support for
things that we actually need and use.
If you have no use for this, but it's here just "because we might want
it someday", then we can't take it for obvious reasons.
So provide a user of the feature, and then we can actually understand if
it is worth adding, or perhaps, it's not needed at all as other things
can be done.
> And since there is nothing
> that prevents `#[pin_data]` to be implemented for such structs, we
> should it do it.
> Rust generally aims to make all features compatible
> with each other and we would like to do the same for our
> libraries/customized features.
The kernel doesn't have a "library", that's not how we work, it's
self-contained and does not export anything nor work with external
libraries outside of its source tree.
> The longer answer is a concrete example of a usecase for const generic
> parameter defaults: the `Work<T, ID>` struct of the workqueue bindings.
> The `ID` parameter is used to identify multiple instances of `Work`
> within the same struct.
Why not just declare them as different names?
And multiple workqueues in a single structure are ripe for problems, are
you sure you need that?
> But if you only intend to have a single `Work`
> struct embedded in your struct, then there is no need to distinguish it
> from something else (after all there is only one) and therefore we want
> people to just write `Work<T>`. This is where the author of
> `Work<T, ID>` can write:
>
> struct Work<T, const ID: usize = 0> {
> // ...
> }
>
> But the `= 0` syntax is currently not supported by `#[pin_data]`.
Why not just force a name for either way it is declared? Wait, "id"?
What is that for and what will require and define that?
thanks,
greg k-h
On 11/25/23 17:03, Greg KH wrote:
>>>> Without this, the `#[pin_data]
>>>> macro would not allow specifying const generic parameter default values
>>>> and instead emit a compile error.
>>>
>>> That's nice, but it still doesn't tell me _why_ this is needed. Why
>>> would I want any generic paramter default values at all? Who needs any
>>> of this? What will it be used for? What does it actually do?
>>
>> `#[pin_data]` is a proc-macro that one can put on any struct to make the
>> pin-init API available for use with that struct. Since e.g. mutexes are
>> initialized using the pin-init API, you have to do this for anything
>> that contains a mutex.
>> This macro should be compatible with any struct definition even with
>> ones that have const generic parameter defaults. This was an oversight
>> in the original design, as it does not support that, since the proc
>> macro parsing cannot handle the `=` character.
>>
>> The short answer for why one would want to have const generic parameter
>> defaults is that the language supports it.
>
> Wait, no, that's not what we do in the kernel. We only add support for
> things that we actually need and use.
>
> If you have no use for this, but it's here just "because we might want
> it someday", then we can't take it for obvious reasons.
>
> So provide a user of the feature, and then we can actually understand if
> it is worth adding, or perhaps, it's not needed at all as other things
> can be done.
Here's how I see the proposed change: "The workqueue abstractions has to
use a backdoor to implement something because the safe and more
convenient API doesn't support it. Improve the safe API so that the
workqueue does not need the backdoor, then update the workqueue to not
use the backdoor."
>> And since there is nothing
>> that prevents `#[pin_data]` to be implemented for such structs, we
>> should it do it.
>> Rust generally aims to make all features compatible
>> with each other and we would like to do the same for our
>> libraries/customized features.
>
> The kernel doesn't have a "library", that's not how we work, it's
> self-contained and does not export anything nor work with external
> libraries outside of its source tree.
I guess this is a question of terminology. What do you call the kernel's
xarray if not a "library" for use by the rest of the kernel?
>> The longer answer is a concrete example of a usecase for const generic
>> parameter defaults: the `Work<T, ID>` struct of the workqueue bindings.
>> The `ID` parameter is used to identify multiple instances of `Work`
>> within the same struct.
>
> Why not just declare them as different names?
I would have preferred to use a textual name rather than an id, but
const generics currently only supports integers.
> And multiple workqueues in a single structure are ripe for problems, are
> you sure you need that?
Originally I had this in Binder for deferring both "flush" and "close".
However, I changed that and now I use a bitfield to keep track of
whether we need a flush or close. (So that if both operations are
scheduled, I can guarantee that I run the flush operation first.)
We could remove the ID from the workqueue abstractions now that I no
longer need it, but it would not really simplify that much in the
workqueue abstraction. Its complexity comes from having to embed the
work_struct inside a user-controlled struct, and once you have to
support that, supporting exactly one or any number of work_struct fields
is about the same difficulty.
The linked list abstraction (which I have not yet sent to the mailing
list) has the same feature, and there, Rust Binder actually *does* need
a single struct to have multiple list_head fields in some places, so at
least the current state means that these APIs are more consistent with
each other.
>> But if you only intend to have a single `Work`
>> struct embedded in your struct, then there is no need to distinguish it
>> from something else (after all there is only one) and therefore we want
>> people to just write `Work<T>`. This is where the author of
>> `Work<T, ID>` can write:
>>
>> struct Work<T, const ID: usize = 0> {
>> // ...
>> }
>>
>> But the `= 0` syntax is currently not supported by `#[pin_data]`.
>
> Why not just force a name for either way it is declared? Wait, "id"?
> What is that for and what will require and define that?
Each work_struct field specifies an id as part of its type, and when you
call `enqueue`, you use the same id to specify which work_struct to
enqueue to the workqueue. The ids are purely a compile-time thing, and
do not exist at runtime. If you give it an id for which there is no
corresponding field, it will fail to compile. If you use the same id for
two fields in the same struct, it will fail to compile. The id has to be
a compile-time constant.
Furthermore, since the workqueue uses a default parameter, you only have
to specify the id if you have multiple work_struct fields.
Alice
@@ -538,6 +538,7 @@ macro_rules! __pin_data {
),
@impl_generics($($impl_generics:tt)*),
@ty_generics($($ty_generics:tt)*),
+ @decl_generics($($decl_generics:tt)*),
@body({ $($fields:tt)* }),
) => {
// We now use token munching to iterate through all of the fields. While doing this we
@@ -560,6 +561,9 @@ macro_rules! __pin_data {
@impl_generics($($impl_generics)*),
// The 'ty generics', the generics that will need to be specified on the impl blocks.
@ty_generics($($ty_generics)*),
+ // The 'decl generics', the generics that need to be specified on the struct
+ // definition.
+ @decl_generics($($decl_generics)*),
// The where clause of any impl block and the declaration.
@where($($($whr)*)?),
// The remaining fields tokens that need to be processed.
@@ -585,6 +589,7 @@ macro_rules! __pin_data {
@name($name:ident),
@impl_generics($($impl_generics:tt)*),
@ty_generics($($ty_generics:tt)*),
+ @decl_generics($($decl_generics:tt)*),
@where($($whr:tt)*),
// We found a PhantomPinned field, this should generally be pinned!
@fields_munch($field:ident : $($($(::)?core::)?marker::)?PhantomPinned, $($rest:tt)*),
@@ -607,6 +612,7 @@ macro_rules! __pin_data {
@name($name),
@impl_generics($($impl_generics)*),
@ty_generics($($ty_generics)*),
+ @decl_generics($($decl_generics)*),
@where($($whr)*),
@fields_munch($($rest)*),
@pinned($($pinned)* $($accum)* $field: ::core::marker::PhantomPinned,),
@@ -623,6 +629,7 @@ macro_rules! __pin_data {
@name($name:ident),
@impl_generics($($impl_generics:tt)*),
@ty_generics($($ty_generics:tt)*),
+ @decl_generics($($decl_generics:tt)*),
@where($($whr:tt)*),
// We reached the field declaration.
@fields_munch($field:ident : $type:ty, $($rest:tt)*),
@@ -640,6 +647,7 @@ macro_rules! __pin_data {
@name($name),
@impl_generics($($impl_generics)*),
@ty_generics($($ty_generics)*),
+ @decl_generics($($decl_generics)*),
@where($($whr)*),
@fields_munch($($rest)*),
@pinned($($pinned)* $($accum)* $field: $type,),
@@ -656,6 +664,7 @@ macro_rules! __pin_data {
@name($name:ident),
@impl_generics($($impl_generics:tt)*),
@ty_generics($($ty_generics:tt)*),
+ @decl_generics($($decl_generics:tt)*),
@where($($whr:tt)*),
// We reached the field declaration.
@fields_munch($field:ident : $type:ty, $($rest:tt)*),
@@ -673,6 +682,7 @@ macro_rules! __pin_data {
@name($name),
@impl_generics($($impl_generics)*),
@ty_generics($($ty_generics)*),
+ @decl_generics($($decl_generics)*),
@where($($whr)*),
@fields_munch($($rest)*),
@pinned($($pinned)*),
@@ -689,6 +699,7 @@ macro_rules! __pin_data {
@name($name:ident),
@impl_generics($($impl_generics:tt)*),
@ty_generics($($ty_generics:tt)*),
+ @decl_generics($($decl_generics:tt)*),
@where($($whr:tt)*),
// We found the `#[pin]` attr.
@fields_munch(#[pin] $($rest:tt)*),
@@ -705,6 +716,7 @@ macro_rules! __pin_data {
@name($name),
@impl_generics($($impl_generics)*),
@ty_generics($($ty_generics)*),
+ @decl_generics($($decl_generics)*),
@where($($whr)*),
@fields_munch($($rest)*),
// We do not include `#[pin]` in the list of attributes, since it is not actually an
@@ -724,6 +736,7 @@ macro_rules! __pin_data {
@name($name:ident),
@impl_generics($($impl_generics:tt)*),
@ty_generics($($ty_generics:tt)*),
+ @decl_generics($($decl_generics:tt)*),
@where($($whr:tt)*),
// We reached the field declaration with visibility, for simplicity we only munch the
// visibility and put it into `$accum`.
@@ -741,6 +754,7 @@ macro_rules! __pin_data {
@name($name),
@impl_generics($($impl_generics)*),
@ty_generics($($ty_generics)*),
+ @decl_generics($($decl_generics)*),
@where($($whr)*),
@fields_munch($field $($rest)*),
@pinned($($pinned)*),
@@ -757,6 +771,7 @@ macro_rules! __pin_data {
@name($name:ident),
@impl_generics($($impl_generics:tt)*),
@ty_generics($($ty_generics:tt)*),
+ @decl_generics($($decl_generics:tt)*),
@where($($whr:tt)*),
// Some other attribute, just put it into `$accum`.
@fields_munch(#[$($attr:tt)*] $($rest:tt)*),
@@ -773,6 +788,7 @@ macro_rules! __pin_data {
@name($name),
@impl_generics($($impl_generics)*),
@ty_generics($($ty_generics)*),
+ @decl_generics($($decl_generics)*),
@where($($whr)*),
@fields_munch($($rest)*),
@pinned($($pinned)*),
@@ -789,6 +805,7 @@ macro_rules! __pin_data {
@name($name:ident),
@impl_generics($($impl_generics:tt)*),
@ty_generics($($ty_generics:tt)*),
+ @decl_generics($($decl_generics:tt)*),
@where($($whr:tt)*),
// We reached the end of the fields, plus an optional additional comma, since we added one
// before and the user is also allowed to put a trailing comma.
@@ -802,7 +819,7 @@ macro_rules! __pin_data {
) => {
// Declare the struct with all fields in the correct order.
$($struct_attrs)*
- $vis struct $name <$($impl_generics)*>
+ $vis struct $name <$($decl_generics)*>
where $($whr)*
{
$($fields)*
@@ -10,7 +10,7 @@ pub(crate) fn pin_data(args: TokenStream, input: TokenStream) -> TokenStream {
let (
Generics {
impl_generics,
- decl_generics: _,
+ decl_generics,
ty_generics,
},
rest,
@@ -77,6 +77,7 @@ pub(crate) fn pin_data(args: TokenStream, input: TokenStream) -> TokenStream {
@sig(#(#rest)*),
@impl_generics(#(#impl_generics)*),
@ty_generics(#(#ty_generics)*),
+ @decl_generics(#(#decl_generics)*),
@body(#last),
});
quoted.extend(errs);