Re: [PATCH 1/2] rust: add initial scatterlist bindings

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

On Fri May 30, 2025 at 11:14 PM JST, Jason Gunthorpe wrote:
> On Fri, May 30, 2025 at 11:02:02PM +0900, Alexandre Courbot wrote:
>> You would have a trait for providing the pages and their range:
>> 
>>     /// Provides a list of pages that can be used to build a `SGTable`.
>>     trait SGTablePages {
>>         /// Returns an iterator to the pages providing the backing memory of `self`.
>>         fn pages_iter<'a>(&'a self) -> impl Iterator<Item = &'a bindings::page>;
>>         /// Returns the effective range of the mapping.
>>         fn range(&self) -> Range<usize>;
>>     }
>>
>> The `SGTable` becomes something like:
>> 
>>     struct SGTable<P: SGTablePages, T: MapState>
>>     {
>>         table: Opaque<bindings::sg_table>,
>>         pages: P,
>>         _s: PhantomData<T>,
>>     }
>
> At this point it isn't exactly a sgtable anymore, it is some rust
> specific way to get a dma mapped scatterlist. Most of the actual ways
> to use a sgtable's cpu side would become unavailable for safety
> reasons.
>
> That seems fine to me, and is what I was suggesting when I said not to
> expose set_page at all.
>
> But I would maybe lean into it a bit more, why have the type state at
> all anymore if the flow is SGTablePages -> SgTable -> Dma Mapped?
> There isn't really a reason to expose the CPU populated but not yet
> mapped state to the user at all. They can't do anything with it. Just
> directly create the DMA mapped scatterlist and only expose the DMA
> list through the rust API in a single step.
>
> So much simpler to understand and doesn't leak the bad decisions of
> the scatterlist design.

I would be fully on board with a simpler design, definitely. The reason
why I've tried to keep some doors open is that as you mentioned
scatterlist is used in many different ways, and I am not familiar enough
with all these uses to draw a line and say "we will never ever need to
do that".

Like unmapping a buffer and remapping it later sounds like a plausible
use (say, if the device's own DMA space is limited), so preserving at
least 2 states sounds sensible.

> Certainly the initial uses of scatterlist don't need to ever know
> about or touch the CPU side of the scatterlist, and it would be great
> if Rust could stay that way..

Yeah I am also more and more convinced we don't need to expose that part
and should just write it at initialization time and never touch it
afterwards.