Message-Id: <DAY4A657OJJF.3GE3LBCFXU6SI@nvidia.com>
Date: Sat, 28 Jun 2025 20:07:34 +0900
From: "Alexandre Courbot" <acourbot@...dia.com>
To: "Abdiel Janulgue" <abdiel.janulgue@...il.com>, "Lyude Paul"
 <lyude@...hat.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

On Fri Jun 27, 2025 at 5:31 AM JST, Abdiel Janulgue wrote:
>
>
> On 18/06/2025 04:03, Alexandre Courbot wrote:
>> 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`.
>> 
>
> Just commenting on this bit. From what I've seen, we don't actually leak 
> anything. The cast only creates a reference to the original C `struct 
> sg_table` object which was allocated and owned by whichever kernel 
> subsystem called sg_alloc_table(). Rust doesn't even allow us to take 
> ownership or to dereference the value, so this one is safe. Destructors 
> are not called on those "casted" objects.

Looks like I was confused about how `sg_table()` worked. This
method is introduced in [1] and calls `drm_gem_shmem_get_pages_sgt`. I
assumed that it did a `sg_alloc_table*` somewhere and returned the
result, leaving the caller responsible for releasing it - which would
indeed introduce a leak if we just converted that pointer into a
reference.

But instead it appears that the SG table is allocated once, mapped and
stored into `shmem->sgt`, and that's the pointer that is returned. Thus
`shmem` is the owner of the table and the semantics are indeed safe.
Thanks for the clarification.

[1] https://lore.kernel.org/rust-for-linux/20250521204654.1610607-8-lyude@redhat.com/

Powered by blists - more mailing lists