Message-Id: <DEJ61A0GHW1Z.3TXMGSHOQ6Y0X@nvidia.com>
Date: Thu, 27 Nov 2025 12:43:54 +0900
From: "Alexandre Courbot" <acourbot@...dia.com>
To: "Joel Fernandes" <joelagnelf@...dia.com>, "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>
Subject: Re: [PATCH v2 3/3] rust: clist: Add typed iteration with
 FromListHead trait

On Thu Nov 27, 2025 at 5:14 AM JST, Joel Fernandes wrote:
>
>
>> 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. 

I think the current 2 iterators design is elegant: the lower-level one
taking care of going through the list, and the higher-level one building
on top of that and adding upcasting. Maybe the lower-level one can be
made private, but I'd keep it in any case.

Powered by blists - more mailing lists