Alexandre Courbot
2025-Nov-04 13:42 UTC
[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.
Miguel Ojeda
2025-Nov-04 14:07 UTC
[PATCH RFC 1/4] rust: clist: Add abstraction for iterating over C linked lists
On Tue, Nov 4, 2025 at 2:42?PM Alexandre Courbot <acourbot at nvidia.com> wrote:> > 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).Using a shared module/file may be good enough, as long as the `#[path = ...] mod ...;` or `include!(...)` is hidden with `#`, i.e. as long as the user does not need to see that to understand the example. But, yeah, we have already a few places in the tree with fake `mod bindings` for doctests and things like that. Cc'ing Guillaume in case there is a better way to do this. The "have something applied to several parts of docs" has come up before for references too (the "external references map" I proposed). In any case, even if the example does not run, it is still way better to have it at least build instead of completely ignored, because that means it will not become stale. Cheers, Miguel