Message-Id: <DC9VAFQUGJ77.3E5GETM4HPPUK@nvidia.com>
Date: Sat, 23 Aug 2025 23:12:43 +0900
From: "Alexandre Courbot" <acourbot@...dia.com>
To: "Danilo Krummrich" <dakr@...nel.org>
Cc: <akpm@...ux-foundation.org>, <ojeda@...nel.org>,
 <alex.gaynor@...il.com>, <boqun.feng@...il.com>, <gary@...yguo.net>,
 <bjorn3_gh@...tonmail.com>, <lossin@...nel.org>, <a.hindborg@...nel.org>,
 <aliceryhl@...gle.com>, <tmgross@...ch.edu>, <abdiel.janulgue@...il.com>,
 <jgg@...pe.ca>, <lyude@...hat.com>, <robin.murphy@....com>,
 <daniel.almeida@...labora.com>, <rust-for-linux@...r.kernel.org>,
 <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 3/5] rust: scatterlist: Add type-state abstraction
 for sg_table

On Sat Aug 23, 2025 at 10:48 PM JST, Danilo Krummrich wrote:
> On Sat Aug 23, 2025 at 3:22 PM CEST, Alexandre Courbot wrote:
>> On Thu Aug 21, 2025 at 1:52 AM JST, Danilo Krummrich wrote:
>>> +impl SGEntry {
>>> +    /// Convert a raw `struct scatterlist *` to a `&'a SGEntry`.
>>> +    ///
>>> +    /// # Safety
>>> +    ///
>>> +    /// Callers must ensure that the `struct scatterlist` pointed to by `ptr` is valid for the
>>> +    /// lifetime `'a`.
>>> +    #[inline]
>>> +    unsafe fn from_raw<'a>(ptr: *mut bindings::scatterlist) -> &'a Self {
>>> +        // SAFETY: The safety requirements of this function guarantee that `ptr` is a valid pointer
>>
>> nit: "guarantees".
>
> "guarantee" seems correct to me; it's "requirements" not "requirement".
>
> (I think we commonly use the plural, i.e. "requirements" even if we end up
> listing a single requirement only.)

Ah, you are correct! I missed the plural on "requirements".

>
>> <snip>
>>> +impl SGTable {
>>> +    /// Creates a borrowed `&'a SGTable` from a raw `struct sg_table` pointer.
>>> +    ///
>>> +    /// This allows safe access to an `sg_table` that is managed elsewhere (for example, in C code).
>>
>> nit: "to a".
>
> I'm not a native speaker, but I think "an" is correct, since "sg_table" is
> pronounced with a vowel sound, /ɛs/, at the beginning.

TIL. :)

>
>>> +    ///
>>> +    /// # Safety
>>> +    ///
>>> +    /// Callers must ensure that:
>>> +    ///
>>> +    /// - the `struct sg_table` pointed to by `ptr` is valid for the entire lifetime of `'a`,
>>> +    /// - the data behind `ptr` is not modified concurrently for the duration of `'a`.
>>> +    #[inline]
>>> +    pub unsafe fn from_raw<'a>(ptr: *mut bindings::sg_table) -> &'a Self {
>>> +        // SAFETY: The safety requirements of this function guarantee that `ptr` is a valid pointer
>>> +        // to a `struct sg_table` for the duration of `'a`.
>>> +        unsafe { &*ptr.cast() }
>>> +    }
>>> +
>>> +    #[inline]
>>> +    fn as_raw(&self) -> *mut bindings::sg_table {
>>> +        self.inner.0.get()
>>> +    }
>>> +
>>> +    fn as_iter(&self) -> SGTableIter<'_> {
>>> +        // SAFETY: `self.as_raw()` is a valid pointer to a `struct sg_table`.
>>> +        let ptr = unsafe { (*self.as_raw()).sgl };
>>> +
>>> +        // SAFETY: `ptr` is guaranteed to be a valid pointer to a `struct scatterlist`.
>>> +        let pos = Some(unsafe { SGEntry::from_raw(ptr) });
>>> +
>>> +        SGTableIter { pos }SGEntry
>>> +    }
>>> +}
>>> +
>>> +/// # Invariants
>>> +///
>>> +/// - `sgt` is a valid pointer to a `struct sg_table` for the entire lifetime of an [`DmaMapSgt`].
>>
>> nit: "of the".
>
> This one I don't know for sure, maybe a native speaker can help.
>
> I chose "for", since I think it indicates duration and "of" rather belonging,
> but I honestly don't know. :)

I didn't give enough context. I meant "of the [`DmaMapSgt`]" (as in, it
cannot be any DmaMapSgt; it has to be this particular one).

>
>>> +/// - `sgt` is always DMA mapped.
>>> +struct DmaMapSgt {
>>
>> Minor point: I'd call this structure `DmaMappedSgt` to highlight the
>> fact that it is actively mapped. Or alternatively document it and its
>> members so that fact is clear.
>>
>> <snip>
>>> +impl<'a> IntoIterator for &'a SGTable {
>>> +    type Item = &'a SGEntry;
>>> +    type IntoIter = SGTableIter<'a>;
>>> +
>>> +    #[inline]
>>> +    fn into_iter(self) -> Self::IntoIter {
>>> +        self.as_iter()
>>> +    }
>>> +}
>>
>> While using this for Nova, I found it a bit unnatural having to call
>> `into_iter` on references intead of just having an `iter` method.
>> `into_iter` sounds like the passed object is consumed, while it is
>> actually its (copied) reference that is. Why not have a regular `iter`
>> method on `SGTable`? Actually we do have one, but it is called `as_iter`
>> and is private for some reason. :)
>
> I think it makes sense to rename to SGTable::iter() and make it public.
>
> I'm also fine removing the IntoIterator implementation, it seems pretty unlikely
> that we'll have another type that provides an Iterator with SGEntry items we
> need a generic interface for.

I assumed there was some Rust pattern to this `IntoIterator`
implementation on a reference, but I cannot see its usefulness when an
`iter` method also works on a reference anyway. So yeah if there is no
reason against it I think this would be more intuitive.

>
>>> +
>>> +/// An [`Iterator`] over the [`SGEntry`] items of an [`SGTable`].
>>> +pub struct SGTableIter<'a> {
>>> +    pos: Option<&'a SGEntry>,
>>> +}
>>> +
>>> +impl<'a> Iterator for SGTableIter<'a> {
>>> +    type Item = &'a SGEntry;
>>> +
>>> +    fn next(&mut self) -> Option<Self::Item> {
>>> +        let entry = self.pos?;
>>> +
>>> +        // SAFETY: `entry.as_raw()` is a valid pointer to a `struct scatterlist`.
>>> +        let next = unsafe { bindings::sg_next(entry.as_raw()) };
>>> +
>>> +        self.pos = (!next.is_null()).then(|| {
>>> +            // SAFETY: If `next` is not NULL, `sg_next()` guarantees to return a valid pointer to
>>> +            // the next `struct scatterlist`.
>>> +            unsafe { SGEntry::from_raw(next) }
>>> +        });
>>
>> This might be missing a stop condition.
>
> [...]
>
>> follow the advice given by the documentation of
>> `sg_dma_address` and also stop if the DMA length of the next one is
>> zero.
>
> Doh! I was even aware of this before sending the initial version and simply
> forgot to add this stop condition after having been interrupted.
>
> Thanks a lot for catching this!

A detail whose knowledge is typically acquired through considerable
suffering. :)

Powered by blists - more mailing lists