Message-Id: <DAP96FEJ0XWT.PWYMIE8IJAVQ@nvidia.com>
Date: Wed, 18 Jun 2025 10:03:46 +0900
From: "Alexandre Courbot" <acourbot@...dia.com>
To: "Lyude Paul" <lyude@...hat.com>, "Abdiel Janulgue"
 <abdiel.janulgue@...il.com>, "Jason Gunthorpe" <jgg@...pe.ca>
Cc: <dakr@...nel.org>, "Miguel Ojeda" <ojeda@...nel.org>, "Alex Gaynor"
 <alex.gaynor@...il.com>, "Boqun Feng" <boqun.feng@...il.com>, "Gary Guo"
 <gary@...yguo.net>, Björn Roy Baron
 <bjorn3_gh@...tonmail.com>, "Benno Lossin" <benno.lossin@...ton.me>,
 "Andreas Hindborg" <a.hindborg@...nel.org>, "Alice Ryhl"
 <aliceryhl@...gle.com>, "Trevor Gross" <tmgross@...ch.edu>, "Valentin Obst"
 <kernel@...entinobst.de>, "open list" <linux-kernel@...r.kernel.org>,
 "Marek Szyprowski" <m.szyprowski@...sung.com>, "Robin Murphy"
 <robin.murphy@....com>, <airlied@...hat.com>,
 <rust-for-linux@...r.kernel.org>, "open list:DMA MAPPING HELPERS"
 <iommu@...ts.linux.dev>, "Petr Tesarik" <petr@...arici.cz>, "Andrew Morton"
 <akpm@...ux-foundation.org>, "Herbert Xu" <herbert@...dor.apana.org.au>,
 "Sui Jingfeng" <sui.jingfeng@...ux.dev>, "Randy Dunlap"
 <rdunlap@...radead.org>, "Michael Kelley" <mhklinux@...look.com>
Subject: Re: [PATCH 1/2] rust: add initial scatterlist bindings

Hi Lyude, sorry for taking so long to come back to this!

On Tue Jun 10, 2025 at 2:44 AM JST, Lyude Paul wrote:
> On Thu, 2025-06-05 at 22:56 +0900, Alexandre Courbot wrote:
>> On Thu Jun 5, 2025 at 10:30 PM JST, Abdiel Janulgue wrote:
>> > 
>> > 
>> > On 05/06/2025 08:51, Alexandre Courbot wrote:
>> > > Maybe I need more context to understand your problem, but the point of
>> > > this design is precisely that it doesn't make any assumption about the
>> > > memory layout - all `P` needs to do is provide the pages describing the
>> > > memory backing.
>> > > 
>> > > Assuming that `_owner` here is the owner of the memory, couldn't you
>> > > flip your data layout and pass `_owner` (or rather a newtype wrapping
>> > > it) to `SGTable`, thus removing the need for a custom type?
>> > 
>> > I think what Lyude has in mind here (Lyude, correct me if I'm wrong) is 
>> > for cases where we need to have a rust SGTable instances for those 
>> > struct sg_table that we didn't allocate ourselves for instance in the 
>> > gem shmem bindings. So memory layout needs to match for
>> > #[repr(transparent)] to work
>> 
>> Thanks, I think I am starting to understand and this is a problem
>> indeed. I should probably take a look at the DRM code to get my answers,
>> but IIUC in `OwnedSGTable`, `sg_table` is already provided by the C side
>> and is backed by `_owner`?
>
> Correct. You generally create a gem shmem object, and then you can call a
> function to ask gem to create an sg_table and hand it back to you. I should
> note my priorities have shifted a bit from trying to get shmem bindings
> upstream, but currently it's still the best example I have of this usecase.
>
> So, for gem shmem this means we can operate with an SGTable in two ways:
>
>  * gem_shmem_object.sg_table() -> Result<&kernel::scatterlist::SGTable>
>  * gem_shmem_object.owned_sg_table() ->
>    Result<kernel::drm::gem::shmem::OwnedSGTable<T: DriverObject>
>
> I'm going to call the first return type SGTable and the second one
> OwnedSGTable, just so I can type a bit less.
>
> The first, sg_table(), quite literally just calls drm_gem_shmem_get_pages_sgt
> which attempts to allocate the table and return a pointer to it on success. We
> then cast that to &SGTable and return it to the user. This can be good for

Mmm but if you cast the returned C pointer into a `&SGTable`, then who
owns (and is responsible for freeing) the `SGTable` it refers to? If the
pointer is just turned into a reference then this might leak the `struct
sg_table`.

> usecases where we might only want to look up the SGTable once or twice, and
> only need it for the duration of the & reference for the gem object. This also
> skips needing to take a reference on the gem object as a result, so it's a bit
> cheaper.
>
> The second, owned_sg_table(), just calls .sg_table(), and then takes a
> reference to the gem object on success and returns both as an OwnedSGTable.
> This is for situations where we might be using the SGTable rather frequently,
> e.g. beyond the lifetime of the & reference to the gem object, and we want to
> avoid having to handle a fallible operation for initializing the sg_table each
> time we use it. OwnedSGTable then just implements Deref<SGTable>, which lets
> us use it pretty much identically to a raw SGTable.
>
> With all of this being said though, I think this means we really have two
> classes of operations around sg_table:
>
>    1. Operations that are reasonable to make available on anything that
>       provide a pointer to an sg_table, embedded or not. (iterating through
>       pages immutably, checking the size of the table, etc.).
>    2. Operations that are context-specific. For example: manually assigning
>       pages would be context-specific since it relies on the context that
>       we're the ones creating and allocating the table from scratch.
>
> My reason for leaning towards having SGTable be a barebones wrapper that other
> types can embed or point to is because I think that for any interface that
> handles sg_table creation for the user, the most common class 2 behavior
> across these interfaces is what the SGTable's lifetime is actually tied to and
> where it comes from. For all of these interfaces, `P` (like in your example)
> would be nearly identical implementations that just read from the embedded
> sg_table anyhow. And we'd also have to have separate SGTable implementors for
> each interface even if the majority of them don't introduce much/any
> specialized behavior. It's probably worth noting as well: if getting the
> SGTable from gem shmem wasn't fallible then I likely wouldn't have even added
> the OwnedSGTable type and simply stuck with an & reference to SGTable instead.

I guess what this highlights is that we need one additional layer
between the `SGTable` and the (now optional) `SGTablePages` trait.

The only way I can think of to satisfy your use-case is to have an
unsafe constructor for `SGTable` that takes an already-existing `struct
sg_table` and fully manages it. IMHO it should still store a second
parameter, which ensures that the backing memory of the passed `struct
sg_table` is still there and doesn't move. In your first case, this
second parameter could be a reference to `gem_shmem_object`; in the
second case, an `ARef`. It is the responsibility of the caller to ensure
that the second parameter is indeed tied to the lifetime of the pages
described by the `struct sg_table`.

Then on top of that, we would have a safe constructor for anything that
implements `SGTablePages`, so we can cover common use-cases like `VVec`
at ease. This trait would become a helper instead of being the only way
to create a `SGTable`.

WDYT? I don't think we can provide something that is safe to use without
also storing a "guarantor" for the backing memory of the `struct
sg_table`. And IIUC the casting of the C pointer into a Rust reference
means that there is no owner to eventually free the `struct sg_table` so
it cannot be done anyway - but please correct me if I misunderstood.

>
> I think that this design also doesn't prevent us from abstracting more complex
> behavior through traits anyway. If we had a DeviceSGTable and CpuSGTable, both
> can easily embed a basic SGTable and then implement additional behavior like
> mutable iteration through pages either through a trait that each type
> implements or just through implementing such methods on the types themselves.
> And behavior in type 1 can just be exposed simply through implementing
> Deref<SGTable> again.
>
> Hopefully that makes sense? Let me know if I made any mistakes or need to
> clarify anything though :)

It makes a lot of sense and I clearly overlooked this case, so thank
you. I might still be missing a few details of your explanation, so
would appreciate if we can keep iterating until we reach something that
fully covers what you described. :)

Powered by blists - more mailing lists