Re: [PATCH v5 4/5] rust: Add dma_fence abstractions

[Daniel Almeida]

> 
>>> +}
>>> +// Necessary to guarantee that `inner` always comes first and can be freed by C.
>>> +// Also useful for using casts instead of container_of().
>>> +#[repr(C)]
>>> +#[pin_data]
>>> +struct DriverFenceData<'a, T: Send + Sync + FenceCtxOps> {
>>> +    #[pin]
>>> +    /// The inner fence.
>>> +    // Must always be the first member so that unsafe casting works; but also
>>> +    // necessary so that the C backend can free the allocation (coming from our
>>> +    // Rust code) with kfree_rcu().
>>> +    inner: Fence,
>>> +    /// Callback head for dropping this in a deferred manner through RCU.
>>> +    rcu_head: bindings::callback_head,
>>> +    /// Reference to access the FenceCtx. Useful for obtaining name parameters.
>>> +    fctx: &'a FenceCtx<T>,
>> 
>> This creates a self-referential borrow in the JobQueue, since it holds both (a) the
>> context to mint new seqnos from and (b) the xarray with fences which borrow from
>> that same context. This issue does not exist with the Arc and we should really
>> fix it before moving with this series.
> 
> Can you detail that a bit more. So your JobQueue version has a `data:
> T` where T holds both the xarray and a jobqueue. The xarray contains
> fences that hold references to the FenceCtx. But shouldn't that be fine
> since the life time ensures that the drop order is correct?
> 
> 
> btw we are recently coming up with a proposal on how JobQueue could
> store DriverFences for the driver. I want to present some RFC for that
> soonish.
> 

Let me address this first, I will reply to your other points later today :)

So about “your JobQueue version has a data: T where T holds both the xarray
and a jobqueue”: no. The JobQueue has an xarray backing all the stages and
a context to mint new seqnos from:


struct JobQueueInner {
  fifo: XArray<XaEntry>
  fctx: FenceContext
  // other fields..
}

This fifo backs all the stages in the queue, and we track stages by tracking
indices pointing to the XArray.

XaEntry is split between a fallible prepare() and a infallible commit():

// prepare() stores this:
XaEntry::Reserved {
    job: Arc<T::Job>,
    uninit_fence: DriverFenceAllocation<???, ...>,
    deps: KVec<ARef<Fence>>,
    // other fields
}

XaEntry::Live is similar, except that the allocation is initialized and there's
the whole DriverFence/Fence split and etc.

Under your proposed design, fences must borrow their context from somewhere,
and this somewhere is a sibling field in the JobQueue. This relationship is
unrepresentable in Rust. It only works if you don't store these two things
together.

There are probably a few different ways of fixing this, but I recommend going
back to refcounting.

— Daniel