Message-Id: <DEOLRLCZQMBG.1WHBR4YL8SKYR@nvidia.com>
Date: Wed, 03 Dec 2025 22:06:52 +0900
From: "Alexandre Courbot" <acourbot@...dia.com>
To: "Joel Fernandes" <joelagnelf@...dia.com>,
 <linux-kernel@...r.kernel.org>, <rust-for-linux@...r.kernel.org>,
 <dri-devel@...ts.freedesktop.org>, <nouveau@...ts.freedesktop.org>, "Danilo
 Krummrich" <dakr@...nel.org>, "Dave Airlie" <airlied@...il.com>
Cc: "Alexandre Courbot" <acourbot@...dia.com>, "Alistair Popple"
 <apopple@...dia.com>, "Miguel Ojeda" <ojeda@...nel.org>, "Alex Gaynor"
 <alex.gaynor@...il.com>, "Boqun Feng" <boqun.feng@...il.com>, "Gary Guo"
 <gary@...yguo.net>, <bjorn3_gh@...tonmail.com>, "Benno Lossin"
 <lossin@...nel.org>, "Andreas Hindborg" <a.hindborg@...nel.org>, "Alice
 Ryhl" <aliceryhl@...gle.com>, "Trevor Gross" <tmgross@...ch.edu>, "Simona
 Vetter" <simona@...ll.ch>, "Maarten Lankhorst"
 <maarten.lankhorst@...ux.intel.com>, "Maxime Ripard" <mripard@...nel.org>,
 "Thomas Zimmermann" <tzimmermann@...e.de>, "John Hubbard"
 <jhubbard@...dia.com>, "Timur Tabi" <ttabi@...dia.com>, "Joel Fernandes"
 <joel@...lfernandes.org>, "Lyude Paul" <elle@...thered-steel.dev>, "Daniel
 Almeida" <daniel.almeida@...labora.com>, "Andrea Righi"
 <arighi@...dia.com>, "Philipp Stanner" <phasta@...nel.org>, "Nouveau"
 <nouveau-bounces@...ts.freedesktop.org>
Subject: Re: [PATCH v3] rust: clist: Add support to interface with C linked
 lists

On Sun Nov 30, 2025 at 6:30 AM JST, Joel Fernandes wrote:
<snip>
> +/// Low-level iterator over `list_head` nodes.
> +///
> +/// An iterator used to iterate over a C intrusive linked list (`list_head`). Caller has to
> +/// perform conversion of returned `ClistHead` to an item (using `container_of` macro or similar).
> +///
> +/// # Invariants
> +///
> +/// `ClistHeadIter` is iterating over an allocated, initialized and valid list.
> +struct ClistHeadIter<'a> {
> +    current_head: &'a ClistHead,
> +    list_head: &'a ClistHead,
> +    exhausted: bool,

Mmm, I suspect `exhausted` is not needed at all - please see below.

> +}
> +
> +impl<'a> Iterator for ClistHeadIter<'a> {
> +    type Item = &'a ClistHead;
> +
> +    #[inline]
> +    fn next(&mut self) -> Option<Self::Item> {
> +        if self.exhausted {
> +            return None;
> +        }
> +
> +        // Advance to next node.
> +        self.current_head = self.current_head.next();
> +
> +        // Check if we've circled back to the sentinel head.
> +        if self.current_head == self.list_head {
> +            self.exhausted = true;
> +            return None;
> +        }
> +
> +        Some(self.current_head)
> +    }

IIUC you could just rewrite this as

    let next = self.current_head.next();
    if next == self.list_head {
        None
    } else {
        self.current_head = next;
        Some(self.current_head)
    }

and drop `exhausted` altogether.

> +}
> +
> +impl<'a> FusedIterator for ClistHeadIter<'a> {}

Is there a reason for not implementing `DoubleEndedIterator` as well?

> +
> +/// A typed C linked list with a sentinel head.
> +///
> +/// A sentinel head represents the entire linked list and can be used for
> +/// iteration over items of type `T`, it is not associated with a specific item.
> +///
> +/// # Invariants
> +///
> +/// - `head` is an allocated and valid C `list_head` structure that is the list's sentinel.
> +/// - `offset` is the byte offset of the `list_head` field within the C struct that `T` wraps.
> +///
> +/// # Safety
> +///
> +/// - All the list's `list_head` nodes must be allocated and have valid next/prev pointers.
> +/// - The underlying `list_head` (and entire list) must not be modified by C for the
> +///   lifetime 'a of `Clist`.
> +pub struct Clist<'a, T> {
> +    head: &'a ClistHead,
> +    offset: usize,

Joining the chorus suggesting to move `offset` to a const generic. Not
only will it make the struct smaller, it is also better because now
every single C `list_head` becomes its own `Clist` type, allowing you to
discriminate between them in the type system! Not that we will ever make
use of that, but still! :)

In my review of v2 I also suggested to use a closure as the generic
argument, and that the offset case could be a specialization, but
looking at this version I realize this was overengineered - just using
the offset should be enough for all cases, and is much more elegant, so
that looks like the right call indeed.

> +    _phantom: PhantomData<&'a T>,
> +}
> +
> +impl<'a, T> Clist<'a, T> {
> +    /// Create a typed `Clist` from a raw sentinel `list_head` pointer.
> +    ///
> +    /// The const generic `OFFSET` specifies the byte offset of the `list_head` field within
> +    /// the C struct that `T` wraps.
> +    ///
> +    /// # Safety
> +    ///
> +    /// - `ptr` must be a valid pointer to an allocated and initialized `list_head` structure
> +    ///   representing a list sentinel.
> +    /// - `ptr` must remain valid and unmodified for the lifetime `'a`.
> +    /// - The list must contain items where the `list_head` field is at byte offset `OFFSET`.
> +    /// - `T` must be `#[repr(transparent)]` over the C struct.
> +    #[inline]
> +    pub unsafe fn from_raw_and_offset<const OFFSET: usize>(ptr: *mut bindings::list_head) -> Self {
> +        Self {
> +            // SAFETY: Caller guarantees `ptr` is a valid, sentinel `list_head` object.
> +            head: unsafe { ClistHead::from_raw(ptr) },
> +            offset: OFFSET,
> +            _phantom: PhantomData,
> +        }
> +    }
> +
> +    /// Get the raw sentinel `list_head` pointer.
> +    #[inline]
> +    pub fn as_raw(&self) -> *mut bindings::list_head {
> +        self.head.as_raw()
> +    }
> +
> +    /// Check if the list is empty.
> +    #[inline]
> +    pub fn is_empty(&self) -> bool {
> +        let raw = self.as_raw();
> +        // SAFETY: self.as_raw() is valid per type invariants.
> +        unsafe { (*raw).next == raw }
> +    }
> +
> +    /// Create an iterator over typed items.
> +    #[inline]
> +    pub fn iter(&self) -> ClistIter<'a, T> {
> +        ClistIter {
> +            head_iter: ClistHeadIter {
> +                current_head: self.head,
> +                list_head: self.head,
> +                exhausted: false,
> +            },
> +            offset: self.offset,
> +            _phantom: PhantomData,
> +        }
> +    }
> +}
> +
> +/// High-level iterator over typed list items.
> +pub struct ClistIter<'a, T> {
> +    head_iter: ClistHeadIter<'a>,
> +    offset: usize,

Now that Clist's offset has moved to a const generic, let's do the same
here as well.

Overall I think this version looks pretty clean! A nice,
easy to understand wrapper over the C API.

Powered by blists - more mailing lists