[Intel-gfx] [PATCH 1/6] dma-buf: add dynamic DMA-buf handling v10

[Daniel Vetter]

On Mon, Jun 24, 2019 at 11:23:34AM +0000, Koenig, Christian wrote:
> Am 21.06.19 um 18:27 schrieb Daniel Vetter:
> >>> Your scenario here is new, and iirc my suggestion back then was to
> >>> count the number of pending mappings so you don't go around calling
> >>> ->invalidate on mappings that don't exist.
> >> Well the key point is we don't call invalidate on mappings, but we call
> >> invalidate on attachments.
> >>
> >> When the invalidate on an attachment is received all the importer should at
> >> least start to tear down all mappings.
> > Hm, so either we invalidate mappings instead (pretty big change for
> > dma-buf, but maybe worth it). Or importers need to deal with invalidate on
> > stuff they're don't even have mapped anywhere anyway.
> 
> I actually I don't see a problem with this, but see below.
> 
> >> [SNIP]
> >>> - your scenario, where you call ->invalidate on an attachment which
> >>> doesn't have a mapping. I'll call that very lazy accounting, feels
> >>> like a bug :-) It's also very easy to fix by keeping track who
> >>> actually has a mapping, and then you fix it everywhere, not just for
> >>> the specific case of a recursion into the same caller.
> >> Yeah, exactly. Unfortunately it's not so easy to handle as just a counter.
> >>
> >> When somebody unmaps a mapping you need to know if that is already
> >> invalidated or not. And this requires tracking of each mapping.
> > Yeah we'd need to track mappings. Well, someone has to track mappings, and
> > atm it seems to be a mix of both importer and exporter (and dma-buf.c).
> 
> Maybe I'm missing something, but I don't see the mix?
> 
> Only the importer is responsible to tracking mappings, e.g. the importer 
> calls dma_buf_map_attachment() when it needs a mapping and calls 
> dma_buf_unmap_attachment() when it is done with the mapping.
> 
> In between those two neither the exporter nor the DMA-buf cares about 
> what mappings currently exist. And apart from debugging I actually don't 
> see a reason why they should.

Atm importer has to track which mappings it actually has. Plus dma-buf.c
also tracks that, somehow, with your recursion-preventation code. So
that's the mix.

> >> [SNIP]
> >>> But I guess there's other fixes too possible.
> >>>
> >>> Either way none of this is about recursion, I think the recursive case
> >>> is simply the one where you've hit this already. Drivers will have to
> >>> handle all these additional ->invalidates no matter what with your
> >>> current proposal. After all the point here is that the exporter can
> >>> move the buffers around whenever it feels like, for whatever reasons.
> >> The recursion case is still perfectly valid. In the importer I need to
> >> ignore invalidations which are caused by creating a mapping.
> >>
> >> Otherwise it is perfectly possible that we invalidate a mapping because of
> >> its creation which will result in creating a new one....
> >>
> >> So even if you fix up your mapping case, you absolutely still need this to
> >> prevent recursion :)
> > Hm, but if we stop tracking attachments and instead start tracking
> > mappings, then how is that possible:
> 
> Yeah, but why should we do this? I don't see a benefit here. Importers 
> just create/destroy mappings as they need them.
> 
> > 1. importer has no mappings
> > 2. importer creates attachment. still no mapping
> > 3. importer calls dma_buf_attach_map_sg, still no mapping at this point
> > 4. we call into the exporter implementation. still no mapping
> > 5. exporter does whatever it does. still no mapping
> > 6. exporter finishes. conceptually from the dma-buf pov, _this_ is where
> > the mapping starts to exist.
> > 7. invalidates (hey the exporter maybe changed its mind!) are totally
> > fine, and will be serialized with ww_mutex.
> >
> > So I kinda don understand why the exporter here is allowed to call
> > invalidate too early (the mapping doesn't exist yet from dma-buf pov), and
> > dma-buf needs to filter it out.
> >
> > But anywhere else where we might call ->invalidate and there's not yet a
> > mapping (again purely from dma-buf pov), there the importer is supposed to
> > do the filter.
> 
> Maybe this becomes clearer if we call the callback "moved" instead of 
> "invalidated"?
> 
> I mean this is actually all about the exporter informing the importer 
> that the DMA-buf in question is moving to a new location.
> 
> That we need to create a new mapping and destroy the old one at some 
> point is an implementation detail on the importer.
> 
> I mean the long term idea is to use this for notification that a buffer 
> is moving inside the same driver as well. And in this particular case I 
> actually don't think that we would create mappings at all. Thinking more 
> about it this is actually a really good argument to favor the 
> implementation as it is currently.
> 
> > Someone needs to keep track of all this, and I want clear
> > responsibilities. What they are exactly is not that important.
> 
> Clear responsibilities is indeed a good idea.
> 
> >>> We could also combine the last two with some helpers, e.g. if your
> >>> exporter really expects importers to delay the unmap until it's no
> >>> longer in use, then we could do a small helper which puts all these
> >>> unmaps onto a list with a worker. But I think you want to integrate
> >>> that into your exporters lru management directly.
> >>>
> >>>> So this is just the most defensive thing I was able to come up with,
> >>>> which leaves the least possibility for drivers to do something stupid.
> >>> Maybe we're still talking past each another, but I feel like the big
> >>> issues are all still there. Problem identified, yes, solved, no.
> >> Yeah, agreed your last mail sounded like we are absolutely on the same page
> >> on the problem now.
> > So I pondered a few ideas while working out:
> >
> > 1) We drop this filtering. Importer needs to keep track of all its
> > mappings and filter out invalidates that aren't for that specific importer
> > (either because already invalidated, or not yet mapped, or whatever).
> > Feels fragile.
> >
> > 2) dma-buf.c keeps track of all the mappings. Will be quite invasive I
> > think, and will duplicate what either the importer or exporter are already
> > doing anyway. We might need a map_dynamic_sg so that the invalidate
> > happens on that, and the invalidate is one-shot (i.e. once unmapped you
> > can never use the same mapping again, you need to create a new one). Will
> > probably be quite a bit of code churn.
> >
> > 3) Like two, but instead of creating something new we change the semantics
> > of attachments. For dynamic dma-buf importers, create a new
> > dma_buf_attach_and_map_sg, which does both, together, atomically. Same for
> > unmap. For unmap I see a clever option here:  These attachements are
> > one-shot only, i.e. you attach_and_map_sg, then you get an invalidate,
> > after that the attachment is dead. Can't ever remap it. I think this would
> > neatly solve the "you've invalidated this already" issue. But it is a bit
> > more code churn I guess. Also, it breaks the idea of using the attachment
> > to indicate for which devices a buffer should be accessible, so it's not
> > accidentally pinned into a bad spot. We might still need that even for
> > dynamic importers.
> >
> > 4) The exporter keeps track of which attachments have a mapping, and
> > invalidates them individually. Not sure this would actually solve
> > anything because of the double-invalidate thing. If we go with this we'd
> > probably need to put the responsibility of delaying the unmap after the
> > corresponding fence has signalled also onto the exporter. So importer
> > would unmap from it's ->invalidate callback, but exporter needs to obey
> > the current fences attached to the resv_obj. Also feels a bit fragile
> > since it depends upon the exporter instead of shared code to do the right
> > thing.
> 
> 5) Just keep it as it is currently implemented, but we work on the 
> documentation to make it clear how it is supposed to work.
> 
> I mean as far as I can see this is actually doing exactly what is expected.

It's surprises me, so at least subverts my expectations.

> > I think there's probably a few more variants that might work, but those is
> > what I came up with.
> 
> I will take a moment and look into #1 as well, but I still don't see the 
> need to change anything.

One thing that's tricky is races here, i.e. what if an ->invalidate from
the exporter races with a map from the importer. The usual way to solve
these is by being really careful when you publish a new $thing, in this
case when we add the new mapping to whatever tracking list.

If we quench all these races with the ww_mutex in the reservation, then
the importer can trivially prevent the recursion you're preventing
explicitly here in dma-buf.c already. If we go with something more
fancy/lockless (atm definitely not the plan), then only the exporter can
stop the races through very careful ordering.

Anyway, I feel like we at least understand each another now about what the
question is ("who's responsible for tracking mappings"). Answer still a
bit in the open.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch