Re: ✗ Fi.CI.BAT: failure for series starting with [1/6] dma-buf: add dynamic DMA-buf handling v13

[Daniel Vetter]

On Thu, Aug 8, 2019 at 1:05 PM Koenig, Christian
<Christian.Koenig at amd.com> wrote:
> Am 08.08.19 um 09:29 schrieb Daniel Vetter:
> > On Thu, Aug 8, 2019 at 9:09 AM Koenig, Christian
> > <Christian.Koenig at amd.com> wrote:
> >> Am 07.08.19 um 23:19 schrieb Daniel Vetter:
> >>> On Wed, Jul 31, 2019 at 10:55:02AM +0200, Daniel Vetter wrote:
> >>>> On Thu, Jun 27, 2019 at 09:28:11AM +0200, Christian König wrote:
> >>>>> Hi Daniel,
> >>>>>
> >>>>> those fails look like something random to me and not related to my patch
> >>>>> set. Correct?
> >>>> First one I looked at has the reservation_obj all over:
> >>>>
> >>>> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13438/fi-cml-u/igt@gem_exec_fence@basic-busy-default.html
> >>>>
> >>>> So 5 second guees is ... probably real?
> >>>>
> >>>> Note that with the entire lmem stuff going on right now there's massive
> >>>> discussions about how we're doing resv_obj vs obj->mm.lock the wrong way
> >>>> round in i915, so I'm not surprised at all that you managed to trip this.
> >>>>
> >>>> The way I see it right now is that obj->mm.lock needs to be limited to
> >>>> dealing with the i915 shrinker interactions only, and only for i915 native
> >>>> objects. And for dma-bufs we need to make sure it's not anywhere in the
> >>>> callchain. Unfortunately that's a massive refactor I guess ...
> >>> Thought about this some more, aside from just breaking i915 or waiting
> >>> until it's refactored (Both not awesome) I think the only option is get
> >>> back to the original caching. And figure out whether we really need to
> >>> take the direction into account for that, or whether upgrading to
> >>> bidirectional unconditionally won't be ok. I think there's only really two
> >>> cases where this matters:
> >>>
> >>> - display drivers using the cma/dma_alloc helpers. Everything is allocated
> >>>     fully coherent, cpu side wc, no flushing.
> >>>
> >>> - Everyone else (on platforms where there's actually some flushing going
> >>>     on) is for rendering gpus, and those always map bidirectional and want
> >>>     the mapping cached for as long as possible.
> >>>
> >>> With that we could go back to creating the cached mapping at attach time
> >>> and avoid inflicting the reservation object lock to places that would keel
> >>> over.
> >>>
> >>> Thoughts?
> >> Actually we had a not so nice internal mail thread with our hardware
> >> guys and it looks like we have tons of hardware bugs/exceptions that
> >> sometimes PCIe BARs are only readable or only writable. So it turned out
> >> that always caching with bidirectional won't work for us either.
> >>
> >> Additional to that I'm not sure how i915 actually triggered the issue,
> >> cause with the current code that shouldn't be possible.
> >>
> >> But independent of that I came to the conclusion that we first need to
> >> get to a common view of what the fences in the reservation mean or
> >> otherwise the whole stuff here isn't going to work smooth either.
> >>
> >> So working on that for now and when that's finished I will come back to
> >> this problem here again.
> > Yeah makes sense. I think we also need to clarify a bit the existing
> > rules around reservatrion_object, dma_fence signaling, and how that
> > nests with everything else (like memory allocation/fs_reclaim critical
> > sections, or mmap_sem).
> >
> > Ignore the drivers which just pin everything system memory (mostly
> > just socs) I think we have a bunch of groups, and they're all somewhat
> > incompatible with each another. Examples:
> >
> > - old ttm drivers (anything except amdgpu) nest the mmap_sem within
> > the reservation_object. That allows you to do copy_*_user while
> > holding reservations, simplifying command submission since you don't
> > need fallback paths when you take a fault. But means you have this
> > awkward trylock in the mmap path with no forward progress guarantee at
> > all.
> >
> > amdgpu fixed that (but left ttm alone), i915 also works like that with
> > mmap_sem being the outer lock.
>
> By the way that is incorrect. Both amdgpu as well as readeon don't use
> copy_to/from_user while holding the reservation lock.

Cool, this is great. When I recently re-read a pile of code in there I
only looked at the amdgpu cs again in detail. And yeah on rechecking
at least nouveau does it backwards still, first it calls
nouveau_gem_pushbuf_validate (which does all the ww_mutex stuff), then
nouveau_gem_pushbuf_reloc_apply (which calls copy_from_user and
doesn't seem to have any fallback/retry or slowpath that does the copy
without holding).

I think vmwgfx is also (no longer?) inverted, it seems to drop the
resv_obj locks in  vmw_validation_bo_fence(), and only after that call
vmw_execbuf_copy_fence_user(). So not a resv_obj vs. mmap_sem
inversion, but the copy_*_user might trigger the shrinker. At least
amdgpu and i915 rely on being able to wait on published fences in
there, so we might look at some other inversion here. Unfortunately
this isn't one lockdep can catch :-/

> The last time I checked the only driver still doing that was nouveau.
>
> Maybe time to add a might_lock() so that we will be informed about
> misuse by lockdep?

Not even a might_lock, just priming the lockdep dependency tracking by
essentially doing a quick sequence of:

mutex_lock(mmap_sem);
reservation_object_lock();
fs_reclaim_acquire(GFP_KERNEL);

And then unwinding. Once you've done that then lockdep will know that
a) resv_obj nests within mmap_sem
b) you can allocate memory when holding a resv_obj

And like with any other lockdep splats it will loudly complain if
anyone breaks the rules. Since we know that nouveau (and probably also
vmwgfx I guess) breaks the rules we'll probably need to hide this
behind CONFIG_DEBUG_WW_MUTEX_SLOWPATH or something similar.

I'm also working on some lockdep annotation improvements for
mmu_notifier. We might also want to make sure we handle that part
consistent across drivers, but I'm much less clear on what the real
rules are there (aside from i915 userptr looks really strange compared
to the amdgpu one).
-Daniel

>
> Christian.
>
> >
> > - other is reservation_object vs memory allocations. Currently all
> > drivers assume you can allocate memory while holding a reservation,
> > but i915 gem folks seem to have some plans to change that for i915.
> > Which isn't going to work I think, so we need to clarify that before
> > things get more inconsistent.
> >
> > Above two can at least be ensured by adding somme lockdep annotations
> > and dependency priming, see i915_gem_shrinker_taints_mutex for what I
> > have in mind for reservation_obj.
> >
> > The real pain/scary thing is dma_fence. All the
> > shrinkers/mmu_notifiers/hmm_mirrors we have assume that you can wait
> > for a fence from allocation contexts/direct reclaim. Which means
> > nothing you do between publishing a fence somewhere (dma-buf, syncobj,
> > syncpt fd) and signalling that fence is allowed to allocate memory or
> > pull in any dependencies which might need memory allocations. I think
> > we're mostly ok with this, but there's some i915 patches that break
> > this.
> >
> > Much worse is that lockdep can't help us check this: dma_fence is
> > essentially struct completion on steroids, and the cross-release
> > lockdep support for struct completion looks like it's never going to
> > get merged. So no debugging aids to make sure we get this right, all
> > we have is review and testing and machines deadlocking in really
> > complicated ways if we get it wrong.
> >
> > Cheers, Daniel
>


-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch