Message-ID: <87bjv0u5j0.fsf@kernel.org>
Date: Mon, 17 Feb 2025 13:20:19 +0100
From: Andreas Hindborg <a.hindborg@...nel.org>
To: "Benno Lossin" <benno.lossin@...ton.me>
Cc: "Danilo Krummrich" <dakr@...nel.org>,  "Miguel Ojeda"
 <ojeda@...nel.org>,  "Alex Gaynor" <alex.gaynor@...il.com>,  "Boqun Feng"
 <boqun.feng@...il.com>,  "Gary Guo" <gary@...yguo.net>,  Björn Roy Baron
 <bjorn3_gh@...tonmail.com>,  "Alice Ryhl" <aliceryhl@...gle.com>,  "Trevor
 Gross" <tmgross@...ch.edu>,  "Joel Becker" <jlbec@...lplan.org>,
  "Christoph Hellwig" <hch@....de>,  "Peter Zijlstra"
 <peterz@...radead.org>,  "Ingo Molnar" <mingo@...hat.com>,  "Will Deacon"
 <will@...nel.org>,  "Waiman Long" <longman@...hat.com>,  "Fiona Behrens"
 <me@...enk.dev>,  "Charalampos Mitrodimas" <charmitro@...teo.net>,
  <rust-for-linux@...r.kernel.org>,  <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 2/3] rust: configfs: introduce rust support for configfs

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

> On 17.02.25 12:08, Andreas Hindborg wrote:

[...]

>>>
>>>> +//!
>>>> +//! See the [rust_configfs.rs] sample for a full example use of this module.
>>>
>>> It could also be useful to just put the example directly here into the
>>> docs instead of/additionally to having it as a sample.
>>
>> I don't think we should duplicate the code. As long as the link works, I
>> think having it separately is fine.
>
> When I'm coding in my editor and read some docs directly in it, it often
> is annoying to find a link, because then I have to open the docs in my
> web-browser.
> I understand that you don't want to duplicate the code (and it also is a
> bit too much for a short example), so how about having a simpler
> example? Maybe with only a single operation that has no associated data
> (use `()`)?

Sure, we can do that.

>
>>>> +//!
>>>> +//! C header: [`include/linux/configfs.h`](srctree/include/linux/configfs.h)
>>>> +//!
>>>> +//! [C documentation]: srctree/Documentation/filesystems/configfs.rst
>>>> +//! [rust_configfs.rs]: srctree/samples/rust/rust_configfs.rs
>>>> +
>>>> +use crate::alloc::flags;
>>>> +use crate::container_of;
>>>> +use crate::page::PAGE_SIZE;
>>>> +use crate::prelude::*;
>>>> +use crate::str::CString;
>>>> +use crate::sync::Arc;
>>>> +use crate::types::ForeignOwnable;
>>>> +use crate::types::Opaque;
>>>> +use core::cell::UnsafeCell;
>>>> +use core::marker::PhantomData;
>>>> +use core::ptr::addr_of;
>>>> +use core::ptr::addr_of_mut;
>>>
>>> I usually would import this like so:
>>>
>>>     use crate::{
>>>         alloc::flags,
>>>         container_of,
>>>         page::PAGE_SIZE,
>>>         prelude::*,
>>>         str::CString,
>>>         sync::Arc,
>>>         types::{ForeignOwnable, Opaque},
>>>     };
>>>     use core::{
>>>         cell::UnsafeCell,
>>>         marker::PhantomData,
>>>         ptr::{addr_of, addr_of_mut},
>>>     };
>>>
>>> To me this is more readable.
>>
>> I disagree with that. I don't think what you suggest is easier to read,
>> and it is much more difficult to work with when rebasing and merging
>> things. This was discussed elsewhere in the past without reaching a
>> conclusion. I think we should come to a consensus on what style we
>> should adopt for the imports.
>
> Yeah for rebasing it is annoying... I think we discussed at some point
> of maybe having a script that automatically merges imports, but that
> runs into the issue of keeping all of them, which might not be
> necessary, because the code below doesn't use everything...
> We should discuss this on a more general basis.
>
> To me the merged form is more readable, because I can better see at a
> glance what things are used from where. But maybe that is just due to
> familiarity with it.
>
> Created an issue to track this: https://github.com/Rust-for-Linux/linux/issues/1143

Cool 👍

>
>>>> +
>>>> +/// A `configfs` subsystem.
>>>> +///
>>>> +/// This is the top level entrypoint for a `configfs` hierarchy. To register
>>>> +/// with configfs, embed a field of this type into your kernel module struct.
>>>> +#[pin_data(PinnedDrop)]
>>>> +pub struct Subsystem<Data> {
>>>
>>> Usually, we don't have multi-character generics, any specific reason
>>> that you chose `Data` here over `T` or `D`?
>>
>> Yes, I find it more descriptive. The patch set went through quite a bit
>> of evolution, and the generics got a bit complicated in earlier
>> iterations, which necessitated more descriptive generic type parameter
>> names. It's not so bad in this version after I restricted the pointer
>> type to just `Arc`, but I still think that using a word rather a single
>> letter makes the code easier to comprehend at first pass.
>
> Makes sense. I'm not opposed to it, but I am a bit cautious, because one
> disadvantage with using multi-character names for generics is that one
> cannot easily see if a type is a generic or not. Maybe that is not as
> important as I think it could be, but to me it seems useful.

If you use an editor with semantic highlighting, you can style the
generic identifiers. I am currently trying out Helix, and that is
unfortunately on of the features it is missing. Can't have it all I
guess.

>
> What do the others think?
>
>> Also, using a word is allowed as per the API guideline document [2]:
>>
>>       > concise UpperCamelCase, usually single uppercase letter: T
>>
>> https://rust-lang.github.io/api-guidelines/naming.html
>>
>>>
>>>> +    #[pin]
>>>> +    subsystem: Opaque<bindings::configfs_subsystem>,
>>>> +    #[pin]
>>>> +    data: Data,
>>>> +}
>>>> +
>>>> +// SAFETY: We do not provide any operations on `Subsystem`.
>>>> +unsafe impl<Data> Sync for Subsystem<Data> {}
>>>> +
>>>> +// SAFETY: Ownership of `Subsystem` can safely be transferred to other threads.
>>>> +unsafe impl<Data> Send for Subsystem<Data> {}
>>>> +
>>>> +impl<Data> Subsystem<Data> {
>>>> +    /// Create an initializer for a [`Subsystem`].
>>>> +    ///
>>>> +    /// The subsystem will appear in configfs as a directory name given by
>>>> +    /// `name`. The attributes available in directory are specified by
>>>> +    /// `item_type`.
>>>> +    pub fn new(
>>>> +        name: &'static CStr,
>>>> +        item_type: &'static ItemType<Subsystem<Data>, Data>,
>>>> +        data: impl PinInit<Data, Error>,
>>>> +    ) -> impl PinInit<Self, Error> {
>>>> +        try_pin_init!(Self {
>>>> +            subsystem <- kernel::init::zeroed().chain(
>>>> +                |place: &mut Opaque<bindings::configfs_subsystem>| {
>>>> +                    // SAFETY: All of `place` is valid for write.
>>>> +                    unsafe {
>>>> +                        addr_of_mut!((*place.get()).su_group.cg_item.ci_name )
>>>> +                            .write(name.as_ptr().cast_mut().cast())
>>>> +                    };
>>>> +                    // SAFETY: All of `place` is valid for write.
>>>> +                    unsafe {
>>>> +                        addr_of_mut!((*place.get()).su_group.cg_item.ci_type)
>>>> +                            .write(item_type.as_ptr())
>>>> +                    };
>>>> +                    // SAFETY: We initialized the required fields of `place.group` above.
>>>> +                    unsafe { bindings::config_group_init(&mut (*placeget()).su_group) };
>>>> +                    // SAFETY: `place.su_mutex` is valid for use as a mutex.
>>>> +                    unsafe { bindings::__mutex_init(
>>>> +                        &mut (*place.get()).su_mutex,
>>>> +                        kernel::optional_name!().as_char_ptr(),
>>>> +                        kernel::static_lock_class!().as_ptr())
>>>
>>> Formatting for this code is weird.
>>>
>>> (since this is inside of the `try_pin_init!` macro, rustfmt doesn't
>>> format it, since `<-` isn't part of rust syntax, so it doesn't know what
>>> to do. I usually fix this by replacing all `<-` with `:`, format and
>>> then change things back)
>>
>> Such is the perils of macros. I'll try to go over it again. Perhaps we
>> could make `rustfmt` understand `<-`?
>
> There have been several discussions about teaching rustfmt custom macro
> rules, but I don't think that such a feature exists.
>
>>> Also, is there no function in C that does all of this initialization for
>>> you?
>>
>> I might be able to do a little better. There is a C function that takes
>> care of initialization of `ci_name` and `ci_type` as well. I can't
>> recall if there was a particular reason for not using it, but I'll
>> check.
>
> Just checking that we don't miss an initialization function, since that
> makes it easier to maintain the code.

Yea, I think I can use the other version that takes name and type.

[...]

>>>> +                unsafe {
>>>> +                    $crate::macros::paste!( [< $data:upper _ATTRS >])
>>>> +                        .add::<N, $attr, _>(
>>>> +                            & $crate::macros::paste!( [< $data:upper _ $name:upper _ATTR >])
>>>> +                        )
>>>> +                };
>>>> +            }),
>>>
>>> You can merge the two `paste!` invocations into one:
>>
>> Is that better?
>
> I feel it is. You trade one indentation level for having less characters
> in the body. To me that is worth it, because then I don't have to ignore
> the `$crate::macros::paste!` characters.

I'll give it a shot.

[...]

>>>> +                    // SAFETY: We are expanding `configfs_attrs`.
>>>> +                    static [< $data:upper _ $name:upper _ATTR >]:
>>>> +                      $crate::configfs::Attribute<$attr, $data, $data> =
>>>> +                        unsafe {
>>>> +                            $crate::configfs::Attribute::new(c_str!(::core::stringify!($name)))
>>>> +                        };
>>>> +                }
>>>> +            )*
>>>> +
>>>> +
>>>> +            const N: usize = $cnt + 1usize;
>>>
>>> Why do we need an additional copy? To have a zero entry at the end for C
>>> to know it's the end of the list? If so, a comment here would be very
>>> helpful.
>>
>> Yes, we need space for a null terminator. I'll add a comment.
>>
>> We actually have a static check to make sure that we not missing this.
>
> Where is this static check?

In `Attribute::add`:

        if I >= N - 1 {
            kernel::build_error!("Invalid attribute index");
        }



Best regards,
Andreas Hindborg

Powered by blists - more mailing lists