Re: [PATCH RFC 1/4] rust: clist: Add abstraction for iterating over C linked lists

["Alexandre Courbot" <acourbot@nvidia.com>]

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.