Message-ID: <352A2D64-C98F-4457-908F-4BB7AF12D858@nvidia.com>
Date: Wed, 26 Nov 2025 20:14:16 +0000
From: Joel Fernandes <joelagnelf@...dia.com>
To: Alexandre Courbot <acourbot@...dia.com>
CC: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"rust-for-linux@...r.kernel.org" <rust-for-linux@...r.kernel.org>,
	"dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
	"dakr@...nel.org" <dakr@...nel.org>, "airlied@...il.com" <airlied@...il.com>,
	Alistair Popple <apopple@...dia.com>, "ojeda@...nel.org" <ojeda@...nel.org>,
	"alex.gaynor@...il.com" <alex.gaynor@...il.com>, "boqun.feng@...il.com"
	<boqun.feng@...il.com>, "gary@...yguo.net" <gary@...yguo.net>,
	"bjorn3_gh@...tonmail.com" <bjorn3_gh@...tonmail.com>, "lossin@...nel.org"
	<lossin@...nel.org>, "a.hindborg@...nel.org" <a.hindborg@...nel.org>,
	"aliceryhl@...gle.com" <aliceryhl@...gle.com>, "tmgross@...ch.edu"
	<tmgross@...ch.edu>, "simona@...ll.ch" <simona@...ll.ch>,
	"maarten.lankhorst@...ux.intel.com" <maarten.lankhorst@...ux.intel.com>,
	"mripard@...nel.org" <mripard@...nel.org>, "tzimmermann@...e.de"
	<tzimmermann@...e.de>, John Hubbard <jhubbard@...dia.com>, Timur Tabi
	<ttabi@...dia.com>, "joel@...lfernandes.org" <joel@...lfernandes.org>,
	"elle@...thered-steel.dev" <elle@...thered-steel.dev>,
	"daniel.almeida@...labora.com" <daniel.almeida@...labora.com>, Andrea Righi
	<arighi@...dia.com>, "phasta@...nel.org" <phasta@...nel.org>,
	"nouveau@...ts.freedesktop.org" <nouveau@...ts.freedesktop.org>, Nouveau
	<nouveau-bounces@...ts.freedesktop.org>, Alexandre Courbot
	<acourbot@...dia.com>
Subject: Re: [PATCH v2 3/3] rust: clist: Add typed iteration with FromListHead
 trait



> On Nov 25, 2025, at 8:03 PM, Alexandre Courbot <acourbot@...dia.com> wrote:
> 
> 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. :)

Great! This makes it easy to use. I will do it this way then - I am assuming everyone is ok baking in this kind of usecase assumed (subtraction of offset). If anyone is not, please raise your concern. 

> 
>> 
>>> 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).

For low level iteration it is already via that type. There are 2 iterators. The higher level uses the lower level one. I could make it even simpler and collapse bother iterators into one - that yields the final type T. 

> 
> 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.

I prefer to generate the iterator separately as a second step  in case we have to extend it to do something other than iteration later, makes it more future ready and better separation of concerns IMO, a list can be more than just iterating, and as you said, we can collapse it later if needed.

Thanks.

Powered by blists - more mailing lists