Re: [RFC] drm/scheduler: Unwrap job dependencies

[Rob Clark]

On Tue, Dec 5, 2023 at 8:56 AM Rob Clark <robdclark@xxxxxxxxx> wrote:
>
> On Tue, Dec 5, 2023 at 7:58 AM Christian König <christian.koenig@xxxxxxx> wrote:
> >
> > Am 05.12.23 um 16:41 schrieb Rob Clark:
> > > On Mon, Dec 4, 2023 at 10:46 PM Christian König
> > > <christian.koenig@xxxxxxx> wrote:
> > >> Am 04.12.23 um 22:54 schrieb Rob Clark:
> > >>> On Thu, Mar 23, 2023 at 2:30 PM Rob Clark <robdclark@xxxxxxxxx> wrote:
> > >>>> [SNIP]
> > >>> So, this patch turns out to blow up spectacularly with dma_fence
> > >>> refcnt underflows when I enable DRIVER_SYNCOBJ_TIMELINE .. I think,
> > >>> because it starts unwrapping fence chains, possibly in parallel with
> > >>> fence signaling on the retire path.  Is it supposed to be permissible
> > >>> to unwrap a fence chain concurrently?
> > >> The DMA-fence chain object and helper functions were designed so that
> > >> concurrent accesses to all elements are always possible.
> > >>
> > >> See dma_fence_chain_walk() and dma_fence_chain_get_prev() for example.
> > >> dma_fence_chain_walk() starts with a reference to the current fence (the
> > >> anchor of the walk) and tries to grab an up to date reference on the
> > >> previous fence in the chain. Only after that reference is successfully
> > >> acquired we drop the reference to the anchor where we started.
> > >>
> > >> Same for dma_fence_array_first(), dma_fence_array_next(). Here we hold a
> > >> reference to the array which in turn holds references to each fence
> > >> inside the array until it is destroyed itself.
> > >>
> > >> When this blows up we have somehow mixed up the references somewhere.
> > > That's what it looked like to me, but wanted to make sure I wasn't
> > > overlooking something subtle.  And in this case, the fence actually
> > > should be the syncobj timeline point fence, not the fence chain.
> > > Virtgpu has essentially the same logic (there we really do want to
> > > unwrap fences so we can pass host fences back to host rather than
> > > waiting in guest), I'm not sure if it would blow up in the same way.
> >
> > Well do you have a backtrace of what exactly happens?
> >
> > Maybe we have some _put() before _get() or something like this.
>
> I hacked up something to store the backtrace in dma_fence_release()
> (and leak the block so the backtrace would still be around later when
> dma_fence_get/put was later called) and ended up with:
>
> [  152.811360] freed at:
> [  152.813718]  dma_fence_release+0x30/0x134
> [  152.817865]  dma_fence_put+0x38/0x98 [gpu_sched]
> [  152.822657]  drm_sched_job_add_dependency+0x160/0x18c [gpu_sched]
> [  152.828948]  drm_sched_job_add_syncobj_dependency+0x58/0x88 [gpu_sched]
> [  152.835770]  msm_ioctl_gem_submit+0x580/0x1160 [msm]
> [  152.841070]  drm_ioctl_kernel+0xec/0x16c
> [  152.845132]  drm_ioctl+0x2e8/0x3f4
> [  152.848646]  vfs_ioctl+0x30/0x50
> [  152.851982]  __arm64_sys_ioctl+0x80/0xb4
> [  152.856039]  invoke_syscall+0x8c/0x120
> [  152.859919]  el0_svc_common.constprop.0+0xc0/0xdc
> [  152.864777]  do_el0_svc+0x24/0x30
> [  152.868207]  el0_svc+0x8c/0xd8
> [  152.871365]  el0t_64_sync_handler+0x84/0x12c
> [  152.875771]  el0t_64_sync+0x190/0x194
>
> I suppose that doesn't guarantee that this was the problematic put.
> But dropping this patch to unwrap the fence makes the problem go
> away..

Oh, hmm, _add_dependency() is consuming the fence reference

BR,
-R

> BR,
> -R
>
> > Thanks,
> > Christian.
> >
> > >
> > > BR,
> > > -R
> > >
> > >> Regards,
> > >> Christian.
> > >>
> > >>> BR,
> > >>> -R
> >