Message-Id: <DEI7ZVKG4JLA.FH1MEMUQLNUK@nvidia.com>
Date: Wed, 26 Nov 2025 10:03:26 +0900
From: "Alexandre Courbot" <acourbot@...dia.com>
To: "Joel Fernandes" <joelagnelf@...dia.com>, "Alexandre Courbot"
 <acourbot@...dia.com>, <linux-kernel@...r.kernel.org>,
 <rust-for-linux@...r.kernel.org>, <dri-devel@...ts.freedesktop.org>,
 <dakr@...nel.org>, <airlied@...il.com>
Cc: <apopple@...dia.com>, <ojeda@...nel.org>, <alex.gaynor@...il.com>,
 <boqun.feng@...il.com>, <gary@...yguo.net>, <bjorn3_gh@...tonmail.com>,
 <lossin@...nel.org>, <a.hindborg@...nel.org>, <aliceryhl@...gle.com>,
 <tmgross@...ch.edu>, <simona@...ll.ch>,
 <maarten.lankhorst@...ux.intel.com>, <mripard@...nel.org>,
 <tzimmermann@...e.de>, <jhubbard@...dia.com>, <ttabi@...dia.com>,
 <joel@...lfernandes.org>, <elle@...thered-steel.dev>,
 <daniel.almeida@...labora.com>, <arighi@...dia.com>, <phasta@...nel.org>,
 <nouveau@...ts.freedesktop.org>, "Nouveau"
 <nouveau-bounces@...ts.freedesktop.org>
Subject: Re: [PATCH v2 3/3] rust: clist: Add typed iteration with
 FromListHead trait

On Wed Nov 26, 2025 at 8:29 AM JST, Joel Fernandes wrote:
> Hi Alex,
>
> On 11/24/2025 2:01 AM, Alexandre Courbot wrote:
>>>  ///
>>>  /// # Invariants
>>>  ///
>>> @@ -69,6 +156,15 @@ pub fn iter_heads(&self) -> ClistHeadIter<'_> {
>>>              head: &self.0,
>>>          }
>>>      }
>>> +
>>> +    /// Create a high-level iterator over typed items.
>>> +    #[inline]
>>> +    pub fn iter<L: ClistLink>(&self) -> ClistIter<'_, L> {
>>> +        ClistIter {
>>> +            head_iter: self.iter_heads(),
>>> +            _phantom: PhantomData,
>>> +        }
>>> +    }
>> This looks very dangerous, as it gives any caller the freedom to specify
>> the type they want to upcast the `Clist` to, without using unsafe code.
>> One could easily invoke this with the wrong type and get no build error
>> or warning whatsoever.
>> 
>> A safer version would have the `Clist` generic over the kind of
>> conversion that needs to be performed, using e.g. a closure:
>> 
>>   pub struct Clist<'a, T, C: Fn(*mut bindings::list_head) -> *mut T> {
>>       head: &'a ClistHead,
>>       conv: C,
>>   }
>> 
>> `from_raw` would also take the closure as argument, which forces the
>> creator of the list to both specify what that list is for, and use an
>> `unsafe` statement for unsafe code. Here is a dummy example:
>> 
>>     let head: bindings::list_head = ...;
>> 
>>     // SAFETY: list_head always corresponds to the `list` member of
>>     // `type_embedding_list_head`.
>>     let conv = |head: *mut bindings::list_head| unsafe {
>>         crate::container_of!(head, type_embedding_list_head, list)
>>     };
>> 
>>     // SAFETY: ...
>>     unsafe { Clist::from_raw(head, conv) }
>> 
>> Then `conv` would be passed down to the `ClistIter` so it can return
>> references to the correct type.
>> 
>> By doing so you can remove the `ClinkList` and `FromListHead` traits,
>> the `impl_from_list_head` and `clist_iterate` macros, as well as the
>> hidden ad-hoc types these create. And importantly, all unsafe code must
>> be explicitly specified in an `unsafe` block, nothing is hidden by
>> macros.
>> 
>> This approach works better imho because each `list_head` is unique in
>> how it has to be iterated: there is no benefit in implementing things
>> using types and traits that will only ever be used in a single place
>> anyway. And if there was, we could always create a newtype for that.
>
> I agree with your safety concerns, indeed it is possible without any safety
> comments to build iterators yielding objects of random type. I think the conv
> function is a good idea and with the addition of unsafe blocks within the conv.
>
> One thing I am concerned is with the user interface. I would like to keep the
> user interface as simple as possible. I am hoping that with implementing your
> idea here on this with the closure, we can still keep it simple, perhaps getting
> the assistance of macros. I will give it a try.

We should be able to build more convenient interfaces on top of this
closure-based design (hopefully without the help of macros).

But first, one needs to recognize that this Clist interface is not your
regular, easy-to-use Rust interface - it is designed for specific cases
where we need to interact with C code and do unsafe things anyway.

Still, the most common (maybe even the only?) conversion pattern will be
"substract an offset from the address of this list_head and cast to this
type". Instead of expressing this through a closure using
`container_of`, maybe we can have a dedicated constructor for these
cases:

  pub unsafe from_raw_and_offset<const LIST_OFFSET: usize>(ptr: *mut bindings::list_head) ->  Clist<'a, T, ...>

`LIST_OFFSET` could be specified by callers using the `offset_of` macro.
This method would then call the more generic `from_raw` constructor,
passing the right closure. And with that you have a more convenient
interface. :)

>
>> Also as I suspected in v1 `Clist` appears to do very little apart from
>> providing an iterator, so I'm more convinced that the front type for
>> this should be `ClistHead`.
>
> This part I don't agree with. I prefer to keep it as `Clist` which wraps a
> sentinel list head. A random `ClistHead` is not necessarily a sentinel.

I expressed myself poorly - what I meant of that `ClistHead` should be
the only type you need for the low-level iteration (which should not be
public).

And if Clist ends up just being a provider for a ClistIterator, you
might just as well return a ClistIterator from the beginning. Anyway,
collapsing the two types into one can be done after the design matures
if it turns out to be the right thing to do, so feel free to keep both
for now.

Powered by blists - more mailing lists