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

[Abdiel Janulgue]

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.

/Abdiel