Date:   Sat, 25 Nov 2023 19:25:40 +0100
From:   Alice Ryhl <alice@...l.io>
To:     Greg KH <gregkh@...uxfoundation.org>,
        Benno Lossin <benno.lossin@...ton.me>
Cc:     Miguel Ojeda <ojeda@...nel.org>,
        Alex Gaynor <alex.gaynor@...il.com>,
        Wedson Almeida Filho <wedsonaf@...il.com>,
        Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>,
        Björn Roy Baron <bjorn3_gh@...tonmail.com>,
        Andreas Hindborg <a.hindborg@...sung.com>,
        Alice Ryhl <aliceryhl@...gle.com>,
        Martin Rodriguez Reboredo <yakoyoku@...il.com>,
        rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/3] rust: macros: allow generic parameter default values
 in `#[pin_data]`

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

Powered by blists - more mailing lists