[PATCH 2/2] dma-buf: cleanup shared fence removal

[Koenig, Christian]

Am 28.06.19 um 11:41 schrieb Daniel Vetter:
> On Fri, Jun 28, 2019 at 10:40 AM Christian König
> <ckoenig.leichtzumerken at gmail.com> wrote:
>> Am 28.06.19 um 09:30 schrieb Daniel Vetter:
>>> On Fri, Jun 28, 2019 at 8:32 AM Koenig, Christian
>>> <Christian.Koenig at amd.com> wrote:
>>>> Am 27.06.19 um 21:57 schrieb Daniel Vetter:
>>>>> [SNIP]
>>> Well yeah you have to wait for outstanding rendering. Everyone does
>>> that. The problem is that ->eviction_fence does not represent
>>> rendering, it's a very contrived way to implement the ->notify_move
>>> callback.
>>>
>>> For real fences you have to wait for rendering to complete, putting
>>> aside corner cases like destroying an entire context with all its
>>> pending rendering. Not really something we should optimize for I
>>> think.
>> No, real fences I don't need to wait for the rendering to complete either.
>>
>> If userspace said that this per process resource is dead and can be
>> removed, we don't need to wait for it to become idle.
> But why did you attach a fence in the first place?

Because so that others can still wait for it.

See it is perfectly valid to export this buffer object to let's say the 
Intel driver and in this case I don't get a move notification.

And I really don't want to add another workaround where I add the fences 
only when the BO is exported or stuff like that.

>> [SNIP]
>> As far as I can see it is perfectly valid to remove all fences from this
>> process as soon as the page tables are up to date.
> I'm not objecting the design, but just the implementation.

Ok, then we can at least agree on that.

>>> So with the magic amdkfd_eviction fence I agree this makes sense. The
>>> problem I'm having here is that the magic eviction fence itself
>>> doesn't make sense. What I expect will happen (in terms of the new
>>> dynamic dma-buf stuff, I don't have the ttm-ism ready to explain it in
>>> those concepts).
>>>
>>> - when you submit command buffers, you _dont_ attach fences to all
>>> involved buffers
>> That's not going to work because then the memory management then thinks
>> that the buffer is immediately movable, which it isn't,
> I guess we need to fix that then. I pretty much assumed that
> ->notify_move could add whatever fences you might want to add. Which
> would very neatly allow us to solve this problem here, instead of
> coming up with fake fences and fun stuff like that.

Adding the fence later on is not a solution because we need something 
which beforehand can check if a buffer is movable or not.

In the case of a move_notify the decision to move it is already done and 
you can't say oh sorry I have to evict my process and reprogram the 
hardware or whatever.

Especially when you do this in an OOM situation.

> If ->notify_move can't add fences, then the you have to attach the
> right fences to all of the bo, all the time. And a CS model where
> userspace just updates the working set and keeps submitting stuff,
> while submitting new batches. And the kernel preempts the entire
> context if memory needs to be evicted, and re-runs it only once the
> working set is available again.
>
> No the eviction_fence is not a good design solution for this, and imo
> you should have rejected that.

Actually I was the one who suggested that because the alternatives 
doesn't sounded like they would work.

[SNIP]
>> See ttm_bo_individualize_resv() as well, here we do something similar
>> for GFX what the KFD does when it releases memory.
> Uh wut. I guess more funky tricks I need to first learn about, but
> yeah doesn't make much sense ot me right now.
>
>> E.g. for per process resources we copy over the current fences into an
>> individualized reservation object, to make sure that this can be freed
>> up at some time in the future.
> Why are you deleteing an object where others outside of your driver
> can still add fences? Just from this description this doesn't make
> sense to me ...

Multiple BOs share a single reservation object. This is used for example 
for page tables.

To map 16GB or memory I can easily have more than 8k BOs for the page 
tables.

So what we did was to use reservation object of the root page table for 
all other BOs of the VM as well.

Otherwise you would need to add a fence to 8k reservation objects and 
that is not really feasible.

That works fine, except for the case when you want to free up a page 
table. In this case we individualize the BO, copy the fences over and 
say ok we free that up when the current set of fences is signaled.

>> But I really want to go a step further and say ok, all fences from this
>> process can be removed after we updated the page tables.
>>
>>> No where do you need to remove a fence, because you never attached a
>>> bogus fence.
>>>
>>> Now with the magic eviction trick amdkfd uses, you can't do that,
>>> because you need to attach this magic fence to all buffers, all the
>>> time. And since it still needs to work like a fence it's one-shot
>>> only, i.e. instead of a reuseable ->notify_move callback you need to
>>> create a new fence every time ->enable_signalling is called. So in a
>>> way replacing fences is just an artifact of some very, very crazy
>>> calling convention.
>>>
>>> If you have a real callback, there's no need for cycling through
>>> fences, and therefore there's also no need to optimize their removal.
>>>
>>> Or did you work under the assumption that ->notify_move cannot attach
>>> new fences, and therefore you'd have to roll this magic fence trick to
>>> even more places?
>> Well that notify_move approach was what was initially suggested by the
>> KFD team as well. The problem is simply that there is no general
>> notify_move callback for buffers yet.
>>
>> Adding that one is exactly what my patches for dynamic DMA-buf are
>> currently doing :)
> Ok, so we agree at least for the amdkfd that the ->notify_move is the
> right solution here.

Actually I don't.

If we don't add some fence to the reservation object the memory 
management has no chance of knowing that this object is used by somebody.

Regards,
Christian.

>   Imo lets get that done and remove eviction_fence,
> instead of promoting that design pattern even more.
>
>>>>> Now I guess I understand the mechanics of this somewhat, and what
>>>>> you're doing, and lit ooks even somewhat safe. But I have no idea what
>>>>> this is supposed to achieve. It feels a bit like ->notify_move, but
>>>>> implemented in the most horrible way possible. Or maybe something
>>>>> else.
>>>>>
>>>>> Really no idea.
>>>>>
>>>>> And given that we've wasted I few pages full of paragraphs already on
>>>>> trying to explain what your new little helper is for, when it's safe
>>>>> to use, when it's maybe not a good idea, and we still haven't even
>>>>> bottomed out on what this is for ... well I really don't think it's a
>>>>> good idea to inflict this into core code. Because just blindly
>>>>> removing normal fences is not safe.
>>>>>
>>>>> Especially with like half a sentence of kerneldoc that explains
>>>>> nothing of all this complexity.
>>> Still makes no sense to me to have in core code.
>> Well it is still better to have this in the core code than messing with
>> the reservation object internals in the driver code.
> Uh no. Imo if you do really funky stuff and abuse fences to get a
> one-shot callback, then you get to keep the cost of maintaining that.
> You _are_ already fundamentally tearing down the abstraction
> dma_fence/resv_obj is supposed to provide. I mean you're essentially
> relying on the exporter calling ->enable_signalling (of the magic
> fence) at roughly the spot where it would call ->notify_move. All
> without that being documented or clarified as how it's supposed to be
> done. Accessing a few private fields is the least offense here :-)
> -Daniel