Message-Id: <DDZYCRCPYMOL.RMTIF0R404Q4@nvidia.com>
Date: Tue, 04 Nov 2025 22:42:05 +0900
From: "Alexandre Courbot" <acourbot@...dia.com>
To: "Joel Fernandes" <joelagnelf@...dia.com>, "Alexandre Courbot"
 <acourbot@...dia.com>
Cc: <linux-kernel@...r.kernel.org>, <rust-for-linux@...r.kernel.org>,
 <dri-devel@...ts.freedesktop.org>, <dakr@...nel.org>, "David Airlie"
 <airlied@...il.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@...lfernandes.org>, "Elle Rhumsaa" <elle@...thered-steel.dev>,
 "Daniel Almeida" <daniel.almeida@...labora.com>, "Andrea Righi"
 <arighi@...dia.com>, "Philipp Stanner" <phasta@...nel.org>,
 <nouveau@...ts.freedesktop.org>, "Nouveau"
 <nouveau-bounces@...ts.freedesktop.org>
Subject: Re: [PATCH RFC 1/4] rust: clist: Add abstraction for iterating over
 C linked lists

On Tue Nov 4, 2025 at 9:58 AM JST, Joel Fernandes wrote:
> On Sat, Nov 01, 2025 at 12:51:32PM +0900, Alexandre Courbot wrote:
>> Hi Joel,
>> 
>> On Fri Oct 31, 2025 at 4:06 AM JST, Joel Fernandes wrote:
>> <snip>
>> > diff --git a/rust/kernel/clist.rs b/rust/kernel/clist.rs
>> > new file mode 100644
>> > index 000000000000..e6a46974b1ba
>> > --- /dev/null
>> > +++ b/rust/kernel/clist.rs
>> > @@ -0,0 +1,296 @@
>> > +// SPDX-License-Identifier: GPL-2.0
>> > +
>> > +//! List processing module for C list_head linked lists.
>> > +//!
>> > +//! C header: [`include/linux/list.h`](srctree/include/linux/list.h)
>> > +//!
>> > +//! Provides a safe interface for iterating over C's intrusive `list_head` structures.
>> > +//! At the moment, supports only read-only iteration.
>> > +//!
>> > +//! # Examples
>> > +//!
>> > +//! ```ignore
>> 
>> Examples pull double-duty as unit tests, and making them build and run
>> ensures that they never fall out-of-date as the code evolves. Please
>> make sure that they can be built and run. Supporting code that you don't
>> want to show in the doc can be put behind `#`.
>
> Sure, the reason I didn't do it was there are a couple of challenges:
>
> 1. For clist, there is a "C language" component" as well in the
> sample, as these are lists coming from C. I am not sure how to do that as a
> doc example, I might have to emulate a C struct containing a list_head in
> Rust itself. Is that something we're Ok with? After all the purpose of a
> sample, is to show how this could be used to interface with lists coming from
> actual C code.

Ah, that's a very valid point.

>From the point of view of the documentation reader and the test itself,
I guess it's ok if the C struct is constructed manually from the
bindings as that part won't appear in the generated doc if you put it
behind `#`. So it will render just fine.

What I'm more worried about is that it might be a PITA to write. :/ But
maybe the core folks have an example for how this could be done cleanly
and in a reusable way (i.e. we don't want to duplicate the dummy list
creation code for every example).

>
> 2. For DRM buddy, #1 is not an issue, however CONFIG_DRM_BUDDY has to be
> enabled for the test to work. I have to figure out how to make a doc test be
> kernel CONFIG dependent. What if the CONFIG required is disabled, is there a
> standard way to make doc tests not fail for features that are disabled? Are the
> doc tests skipped in such a situation? Just something I have to figure out.
> Perhaps wrapping it is #cfg is sufficient.

Tests are not expected to run if the config option of the feature they
test is not enabled - how could they anyway. :) So this part is working
as intended I think.

<snip>
>> > +/// [`Clist`] can have its ownership transferred between threads ([`Send`]) if `T: Send`.
>> > +/// But its never [`Sync`] - concurrent Rust threads with `&Clist` could call C FFI unsafely.
>> > +/// For concurrent access, wrap in a `Mutex` or other synchronization primitive.
>> > +///
>> > +/// # Examples
>> > +///
>> > +/// ```ignore
>> > +/// use kernel::clist::Clist;
>> > +///
>> > +/// // C code provides the populated list_head.
>> > +/// let list = unsafe { Clist::<Item>::new(c_list_head) };
>> > +///
>> > +/// // Iterate over items.
>> > +/// for item in list.iter() {
>> > +///     // Process item.
>> > +/// }
>> > +/// ```
>> > +pub struct Clist<T: FromListHead> {
>> > +    head: *mut bindings::list_head,
>> > +    _phantom: PhantomData<T>,
>> > +}
>> > +
>> > +// SAFETY: Safe to send Clist if T is Send (pointer moves, C data stays in place).
>> > +unsafe impl<T: FromListHead + Send> Send for Clist<T> {}
>> > +
>> > +impl<T: FromListHead> Clist<T> {
>> > +    /// Wrap an existing C list_head pointer without taking ownership.
>> > +    ///
>> > +    /// # Safety
>> > +    ///
>> > +    /// Caller must ensure:
>> > +    /// - `head` points to a valid, initialized list_head.
>> > +    /// - `head` remains valid for the lifetime of the returned [`Clist`].
>> > +    /// - The list is not modified by C code while [`Clist`] exists. Caller must
>> > +    ///   implement mutual exclusion algorithms if required, to coordinate with C.
>> > +    /// - Caller is responsible for requesting or working with C to free `head` if needed.
>> > +    pub unsafe fn new(head: *mut bindings::list_head) -> Self {
>> > +        // SAFETY: Caller ensures head is valid and initialized
>> > +        Self {
>> > +            head,
>> > +            _phantom: PhantomData,
>> > +        }
>> > +    }
>> 
>> I am wondering whether `CList` serves an actual purpose beyond providing
>> ` CListIter` to iterate on... Would it make sense to merge both types
>> into a single one that implements `Iterator`?
>> 
>
> It would force us to store iteration state into `Clist`, I think for that
> reason it would be great to keep them separate IMO.

My comment was more an intuition than a strongly held opinion, so please
use your judgment as you perform the redesign. :) I.e. if it ends up
that one type collapses completely and ends up being a almost empty,
maybe that means we don't need it at all in the end.

Powered by blists - more mailing lists