On Wed, Mar 4, 2020 at 10:27 AM Jason Ekstrand jason@jlekstrand.net wrote:
On Wed, Mar 4, 2020 at 2:34 AM Christian König christian.koenig@amd.com wrote:
Am 03.03.20 um 20:10 schrieb Jason Ekstrand:
On Thu, Feb 27, 2020 at 2:28 AM Christian König christian.koenig@amd.com wrote:
[SNIP]
However, I'm not sure what the best way is to do garbage collection on that so that we don't get an impossibly list of fence arrays.
Exactly yes. That's also the reason why the dma_fence_chain container I came up with for the sync timeline stuff has such a rather sophisticated garbage collection.
When some of the included fences signal you need to free up the array/chain and make sure that the memory for the container can be reused.
Currently (as of v2), I'm using dma_fence_array and being careful to not bother constructing one if there's only one fence in play. Is this insufficient? If so, maybe we should consider improving dma_fence_array.
That still won't work correctly in all cases. See the problem is not only optimization, but also avoiding situations where userspace can abuse the interface to do nasty things.
For example if userspace just calls that function in a loop you can create a long chain of dma_fence_array objects.
If that chain is then suddenly released the recursive dropping of references can overwrite the kernel stack.
For reference see what dance is necessary in the dma_fence_chain_release function to avoid that:
/* Manually unlink the chain as much as possible to avoidrecursion * and potential stack overflow. */ while ((prev = rcu_dereference_protected(chain->prev, true))) {
....
It took me quite a while to figure out how to do this without causing issues. But I don't see how this would be possible for dma_fence_array.
Ah, I see the issue now! It hadn't even occurred to me that userspace could use this to build up an infinite recursion chain. That's nasty! I'll give this some more thought and see if can come up with something clever.
Here's one thought: We could make dma_fence_array automatically collapse any arrays it references and instead directly reference their fences. This way, no matter how much the client chains things, they will never get more than one dma_fence_array. Of course, the difficulty here (answering my own question) comes if they ping-pong back-and-forth between something which constructs a dma_fence_array and something which constructs a dma_fence_chain to get array-of-chain-of-array-of-chain-of-... More thought needed.
Answering my own questions again... I think the array-of-chain-of-array case is also solvable.
For array-of-chain, we can simply add all unsignaled dma_fences in the chain to the array. The array won't signal until all of them have which is exactly the same behavior as if we'd added the chain itself.
For chain-of-array, we can add all unsignaled dma_fences in the array to the same point in the chain. There may be some fiddling with the chain numbering required here but I think we can get it so the chain won't signal until everything in the array has signaled and we get the same behavior as if we'd added the dma_fence_array to the chain.
In both cases, we end up with either a single array or a single and destruction doesn't require recursion. Thoughts?
--Jason