| Message ID | 1474457767-26195-1-git-send-email-gustavo@padovan.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Am 21.09.2016 um 13:36 schrieb Gustavo Padovan: > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk> > > If the fences in the fence_array signal on the fence_array does not have > signalling enabled num_pending will not be updated accordingly. > > So when signaling is disabled check the signal of every fence with > fence_is_signaled() and then compare with num_pending to learn if the > fence_array was signalled or not. > > If we want to keep the poll_does_not_wait optimization I think we need > something like this. It keeps the same behaviour if signalling is enabled > but tries to calculated the state otherwise. > > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk> > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> First of all the patch is horrible wrong because fence_array_signaled() is called without any locks held. So you can run into a race condition between checking the fences here and enable signaling. Additional to that I'm not sure if that is such a good idea or not, cause fence_array_signaled() should be light weight and without calling enable_signaling there is not guarantee that fences will ever signal. Regards, Christian. > --- > drivers/dma-buf/fence-array.c | 19 ++++++++++++++++++- > 1 file changed, 18 insertions(+), 1 deletion(-) > > diff --git a/drivers/dma-buf/fence-array.c b/drivers/dma-buf/fence-array.c > index f1989fc..1eec271 100644 > --- a/drivers/dma-buf/fence-array.c > +++ b/drivers/dma-buf/fence-array.c > @@ -75,8 +75,25 @@ static bool fence_array_enable_signaling(struct fence *fence) > static bool fence_array_signaled(struct fence *fence) > { > struct fence_array *array = to_fence_array(fence); > + int i, num_pending; > + > + num_pending = atomic_read(&array->num_pending); > + > + /* > + * Before signaling is enabled, num_pending is static (set during array > + * construction as a count of all fences or set to 1 if signal_on_any > + * flag is passed. To ensure forward progress, i.e. a while > + * (!fence_is_signaled()) ; busy-loop eventually proceeds, we need to > + * check the current status of our fences. > + */ > + if (!test_bit(FENCE_FLAG_ENABLE_SIGNAL_BIT, &fence->flags)) { > + for (i = 0 ; i < array->num_fences; ++i) { > + if (fence_is_signaled(array->fences[i])) > + num_pending--; > + } > + } > > - return atomic_read(&array->num_pending) <= 0; > + return num_pending <= 0; > } > > static void fence_array_release(struct fence *fence)
On 2016年09月21日 19:36, Gustavo Padovan wrote: > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk> > > If the fences in the fence_array signal on the fence_array does not have > signalling enabled num_pending will not be updated accordingly. > > So when signaling is disabled check the signal of every fence with > fence_is_signaled() and then compare with num_pending to learn if the > fence_array was signalled or not. > > If we want to keep the poll_does_not_wait optimization I think we need > something like this. It keeps the same behaviour if signalling is enabled > but tries to calculated the state otherwise. > > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk> > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> Reviewed-by: Chunming Zhou <david1.zhou@amd.com> Regards, David Zhou > --- > drivers/dma-buf/fence-array.c | 19 ++++++++++++++++++- > 1 file changed, 18 insertions(+), 1 deletion(-) > > diff --git a/drivers/dma-buf/fence-array.c b/drivers/dma-buf/fence-array.c > index f1989fc..1eec271 100644 > --- a/drivers/dma-buf/fence-array.c > +++ b/drivers/dma-buf/fence-array.c > @@ -75,8 +75,25 @@ static bool fence_array_enable_signaling(struct fence *fence) > static bool fence_array_signaled(struct fence *fence) > { > struct fence_array *array = to_fence_array(fence); > + int i, num_pending; > + > + num_pending = atomic_read(&array->num_pending); > + > + /* > + * Before signaling is enabled, num_pending is static (set during array > + * construction as a count of all fences or set to 1 if signal_on_any > + * flag is passed. To ensure forward progress, i.e. a while > + * (!fence_is_signaled()) ; busy-loop eventually proceeds, we need to > + * check the current status of our fences. > + */ > + if (!test_bit(FENCE_FLAG_ENABLE_SIGNAL_BIT, &fence->flags)) { > + for (i = 0 ; i < array->num_fences; ++i) { > + if (fence_is_signaled(array->fences[i])) > + num_pending--; > + } > + } > > - return atomic_read(&array->num_pending) <= 0; > + return num_pending <= 0; > } > > static void fence_array_release(struct fence *fence)
Hi Christian, 2016-09-21 Christian König <christian.koenig@amd.com>: > Am 21.09.2016 um 13:36 schrieb Gustavo Padovan: > > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk> > > > > If the fences in the fence_array signal on the fence_array does not have > > signalling enabled num_pending will not be updated accordingly. > > > > So when signaling is disabled check the signal of every fence with > > fence_is_signaled() and then compare with num_pending to learn if the > > fence_array was signalled or not. > > > > If we want to keep the poll_does_not_wait optimization I think we need > > something like this. It keeps the same behaviour if signalling is enabled > > but tries to calculated the state otherwise. > > > > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk> > > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> > > First of all the patch is horrible wrong because fence_array_signaled() is > called without any locks held. So you can run into a race condition between > checking the fences here and enable signaling. Yes. it can, but I don't think the race condition is actually a problem. Maybe you have some use case that we are not seeing? > Additional to that I'm not sure if that is such a good idea or not, cause > fence_array_signaled() should be light weight and without calling > enable_signaling there is not guarantee that fences will ever signal. It is still lightweight for the case when signaling is enabled and fences can signal with signaling enabled or disabled so I don't see that as problem too. Gustavo
Am 22.09.2016 um 09:44 schrieb Gustavo Padovan: > Hi Christian, > > 2016-09-21 Christian König <christian.koenig@amd.com>: > >> Am 21.09.2016 um 13:36 schrieb Gustavo Padovan: >>> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk> >>> >>> If the fences in the fence_array signal on the fence_array does not have >>> signalling enabled num_pending will not be updated accordingly. >>> >>> So when signaling is disabled check the signal of every fence with >>> fence_is_signaled() and then compare with num_pending to learn if the >>> fence_array was signalled or not. >>> >>> If we want to keep the poll_does_not_wait optimization I think we need >>> something like this. It keeps the same behaviour if signalling is enabled >>> but tries to calculated the state otherwise. >>> >>> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk> >>> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> >> First of all the patch is horrible wrong because fence_array_signaled() is >> called without any locks held. So you can run into a race condition between >> checking the fences here and enable signaling. > Yes. it can, but I don't think the race condition is actually a problem. > Maybe you have some use case that we are not seeing? I'm not sure if that can really race, but if it does the check would return true while not all necessary fences are signaled yet. That would be really really bad for things like TTM where we just do a quick check in the page fault handler for example. I need to double check if that really could be a problem. >> Additional to that I'm not sure if that is such a good idea or not, cause >> fence_array_signaled() should be light weight and without calling >> enable_signaling there is not guarantee that fences will ever signal. > It is still lightweight for the case when signaling is enabled and > fences can signal with signaling enabled or disabled Nope that's not correct. The signaled callback is only optional. See the comment on the fence_is_signaled function: > * Returns true if the fence was already signaled, false if not. Since > this > * function doesn't enable signaling, it is not guaranteed to ever return > * true if fence_add_callback, fence_wait or fence_enable_sw_signaling > * haven't been called before. E.g. for example it is illegal to do something like "while(!fence_is_signaled(f)) sleep();" without enabling signaling before doing this. Could just be a misunderstanding, but the comments on your patch actually sounds a bit like somebody is trying to do exactly that. Regards, Christian. > so I don't see that > as problem too. > > Gustavo >
2016-09-22 Christian König <christian.koenig@amd.com>: > Am 22.09.2016 um 09:44 schrieb Gustavo Padovan: > > Hi Christian, > > > > 2016-09-21 Christian König <christian.koenig@amd.com>: > > > > > Am 21.09.2016 um 13:36 schrieb Gustavo Padovan: > > > > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk> > > > > > > > > If the fences in the fence_array signal on the fence_array does not have > > > > signalling enabled num_pending will not be updated accordingly. > > > > > > > > So when signaling is disabled check the signal of every fence with > > > > fence_is_signaled() and then compare with num_pending to learn if the > > > > fence_array was signalled or not. > > > > > > > > If we want to keep the poll_does_not_wait optimization I think we need > > > > something like this. It keeps the same behaviour if signalling is enabled > > > > but tries to calculated the state otherwise. > > > > > > > > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk> > > > > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> > > > First of all the patch is horrible wrong because fence_array_signaled() is > > > called without any locks held. So you can run into a race condition between > > > checking the fences here and enable signaling. > > Yes. it can, but I don't think the race condition is actually a problem. > > Maybe you have some use case that we are not seeing? > > I'm not sure if that can really race, but if it does the check would return > true while not all necessary fences are signaled yet. How? If signaling is disabled num_pending is equal to the number of fences (or 1 if signal_on_any) then we just check all fences. If all of them are signaled then num_pending will be zero. > > That would be really really bad for things like TTM where we just do a quick > check in the page fault handler for example. > > I need to double check if that really could be a problem. > > > > Additional to that I'm not sure if that is such a good idea or not, cause > > > fence_array_signaled() should be light weight and without calling > > > enable_signaling there is not guarantee that fences will ever signal. > > It is still lightweight for the case when signaling is enabled and > > fences can signal with signaling enabled or disabled > Nope that's not correct. The signaled callback is only optional. > > See the comment on the fence_is_signaled function: > > * Returns true if the fence was already signaled, false if not. Since > > this > > * function doesn't enable signaling, it is not guaranteed to ever return > > * true if fence_add_callback, fence_wait or fence_enable_sw_signaling > > * haven't been called before. Right, I was with explicit fencing in mind, we only enable signaling there if userspace polls. > E.g. for example it is illegal to do something like > "while(!fence_is_signaled(f)) sleep();" without enabling signaling before > doing this. > > Could just be a misunderstanding, but the comments on your patch actually > sounds a bit like somebody is trying to do exactly that. I think the usecase in mind here is poll(fd, timeout=0) Gustavo
Dropping the rest of the patch, cause that really doesn't make sense any more. Am 22.09.2016 um 12:40 schrieb Gustavo Padovan: >> E.g. for example it is illegal to do something like >> >"while(!fence_is_signaled(f)) sleep();" without enabling signaling before >> >doing this. >> > >> >Could just be a misunderstanding, but the comments on your patch actually >> >sounds a bit like somebody is trying to do exactly that. > I think the usecase in mind here is poll(fd, timeout=0) Exactly as I feared. Even if userspace polls with timeout=0 you still need to call enable_signaling(). Otherwise you can run into a situation where userspace only uses timeout=0 and so never activates the signaling check in the driver. This would in turn result in an endless loop on implementations where the driver never signals fences on their own. Regards, Christian.
2016-09-22 Christian König <christian.koenig@amd.com>: > Dropping the rest of the patch, cause that really doesn't make sense any > more. > > Am 22.09.2016 um 12:40 schrieb Gustavo Padovan: > > > E.g. for example it is illegal to do something like > > > >"while(!fence_is_signaled(f)) sleep();" without enabling signaling before > > > >doing this. > > > > > > > >Could just be a misunderstanding, but the comments on your patch actually > > > >sounds a bit like somebody is trying to do exactly that. > > I think the usecase in mind here is poll(fd, timeout=0) > > Exactly as I feared. Even if userspace polls with timeout=0 you still need > to call enable_signaling(). > > Otherwise you can run into a situation where userspace only uses timeout=0 > and so never activates the signaling check in the driver. > > This would in turn result in an endless loop on implementations where the > driver never signals fences on their own. Polling is optional, userspace may never call it. And DRM/KMS or GPU drivers will be doing fence_wait() themselves so signaling is enabled at some point. Gustavo
Am 22.09.2016 um 13:16 schrieb Gustavo Padovan: > 2016-09-22 Christian König <christian.koenig@amd.com>: > >> Dropping the rest of the patch, cause that really doesn't make sense any >> more. >> >> Am 22.09.2016 um 12:40 schrieb Gustavo Padovan: >>>> E.g. for example it is illegal to do something like >>>>> "while(!fence_is_signaled(f)) sleep();" without enabling signaling before >>>>> doing this. >>>>> >>>>> Could just be a misunderstanding, but the comments on your patch actually >>>>> sounds a bit like somebody is trying to do exactly that. >>> I think the usecase in mind here is poll(fd, timeout=0) >> Exactly as I feared. Even if userspace polls with timeout=0 you still need >> to call enable_signaling(). >> >> Otherwise you can run into a situation where userspace only uses timeout=0 >> and so never activates the signaling check in the driver. >> >> This would in turn result in an endless loop on implementations where the >> driver never signals fences on their own. > Polling is optional, userspace may never call it. And DRM/KMS or GPU > drivers will be doing fence_wait() themselves so signaling is enabled at > some point. No they won't. We have an use case where we clearly want to avoid that as much as possible because and so the driver never calls enable_signaling() on it's own. Exposing this poll function to userspace without enabling signaling is a clear NAK from my side. Christian. > > Gustavo >
Am 22.09.2016 um 16:11 schrieb Christian König: > Am 22.09.2016 um 13:16 schrieb Gustavo Padovan: >> 2016-09-22 Christian König <christian.koenig@amd.com>: >> >>> Dropping the rest of the patch, cause that really doesn't make sense >>> any >>> more. >>> >>> Am 22.09.2016 um 12:40 schrieb Gustavo Padovan: >>>>> E.g. for example it is illegal to do something like >>>>>> "while(!fence_is_signaled(f)) sleep();" without enabling >>>>>> signaling before >>>>>> doing this. >>>>>> >>>>>> Could just be a misunderstanding, but the comments on your patch >>>>>> actually >>>>>> sounds a bit like somebody is trying to do exactly that. >>>> I think the usecase in mind here is poll(fd, timeout=0) >>> Exactly as I feared. Even if userspace polls with timeout=0 you >>> still need >>> to call enable_signaling(). >>> >>> Otherwise you can run into a situation where userspace only uses >>> timeout=0 >>> and so never activates the signaling check in the driver. >>> >>> This would in turn result in an endless loop on implementations >>> where the >>> driver never signals fences on their own. >> Polling is optional, userspace may never call it. And DRM/KMS or GPU >> drivers will be doing fence_wait() themselves so signaling is enabled at >> some point. > > No they won't. We have an use case where we clearly want to avoid that > as much as possible because and so the driver never calls > enable_signaling() on it's own. Sorry hitting send to soon. That should read "because of the extreme overhead". Christian. > > Exposing this poll function to userspace without enabling signaling is > a clear NAK from my side. > > Christian. > >> >> Gustavo >> >
2016-09-22 Christian König <christian.koenig@amd.com>: > Am 22.09.2016 um 13:16 schrieb Gustavo Padovan: > > 2016-09-22 Christian König <christian.koenig@amd.com>: > > > > > Dropping the rest of the patch, cause that really doesn't make sense any > > > more. > > > > > > Am 22.09.2016 um 12:40 schrieb Gustavo Padovan: > > > > > E.g. for example it is illegal to do something like > > > > > > "while(!fence_is_signaled(f)) sleep();" without enabling signaling before > > > > > > doing this. > > > > > > > > > > > > Could just be a misunderstanding, but the comments on your patch actually > > > > > > sounds a bit like somebody is trying to do exactly that. > > > > I think the usecase in mind here is poll(fd, timeout=0) > > > Exactly as I feared. Even if userspace polls with timeout=0 you still need > > > to call enable_signaling(). > > > > > > Otherwise you can run into a situation where userspace only uses timeout=0 > > > and so never activates the signaling check in the driver. > > > > > > This would in turn result in an endless loop on implementations where the > > > driver never signals fences on their own. > > Polling is optional, userspace may never call it. And DRM/KMS or GPU > > drivers will be doing fence_wait() themselves so signaling is enabled at > > some point. > > No they won't. We have an use case where we clearly want to avoid that as > much as possible because and so the driver never calls enable_signaling() on > it's own. > > Exposing this poll function to userspace without enabling signaling is a > clear NAK from my side. Okay. So you are NAK'ing the does_not_pool_wait change. Should we revert that one then? It is already broken. Gustavo
Am 23.09.2016 um 13:30 schrieb Gustavo Padovan: > 2016-09-22 Christian König <christian.koenig@amd.com>: > >> Am 22.09.2016 um 13:16 schrieb Gustavo Padovan: >>> 2016-09-22 Christian König <christian.koenig@amd.com>: >>> >>>> Dropping the rest of the patch, cause that really doesn't make sense any >>>> more. >>>> >>>> Am 22.09.2016 um 12:40 schrieb Gustavo Padovan: >>>>>> E.g. for example it is illegal to do something like >>>>>>> "while(!fence_is_signaled(f)) sleep();" without enabling signaling before >>>>>>> doing this. >>>>>>> >>>>>>> Could just be a misunderstanding, but the comments on your patch actually >>>>>>> sounds a bit like somebody is trying to do exactly that. >>>>> I think the usecase in mind here is poll(fd, timeout=0) >>>> Exactly as I feared. Even if userspace polls with timeout=0 you still need >>>> to call enable_signaling(). >>>> >>>> Otherwise you can run into a situation where userspace only uses timeout=0 >>>> and so never activates the signaling check in the driver. >>>> >>>> This would in turn result in an endless loop on implementations where the >>>> driver never signals fences on their own. >>> Polling is optional, userspace may never call it. And DRM/KMS or GPU >>> drivers will be doing fence_wait() themselves so signaling is enabled at >>> some point. >> No they won't. We have an use case where we clearly want to avoid that as >> much as possible because and so the driver never calls enable_signaling() on >> it's own. >> >> Exposing this poll function to userspace without enabling signaling is a >> clear NAK from my side. > Okay. So you are NAK'ing the does_not_pool_wait change. Should we revert > that one then? It is already broken. Yeah, that would probably a good idea. The AMD driver changes which really rely on this aren't upstream yet, so if you point me to the commit hash I could revert that as well when we send that out. On the other hand the idea behind fence_is_signaled() is really that you check the status multiple times after enabling signaling. So I would prefer if you would revert this change preliminary. Double checking this patch (and thinking about it a bit more) reveals that it is most likely correct. So feel free to commit this one if it is still needed for something. Regards, Christian. > > Gustavo >
2016-09-23 Christian König <christian.koenig@amd.com>: > Am 23.09.2016 um 13:30 schrieb Gustavo Padovan: > > 2016-09-22 Christian König <christian.koenig@amd.com>: > > > > > Am 22.09.2016 um 13:16 schrieb Gustavo Padovan: > > > > 2016-09-22 Christian König <christian.koenig@amd.com>: > > > > > > > > > Dropping the rest of the patch, cause that really doesn't make sense any > > > > > more. > > > > > > > > > > Am 22.09.2016 um 12:40 schrieb Gustavo Padovan: > > > > > > > E.g. for example it is illegal to do something like > > > > > > > > "while(!fence_is_signaled(f)) sleep();" without enabling signaling before > > > > > > > > doing this. > > > > > > > > > > > > > > > > Could just be a misunderstanding, but the comments on your patch actually > > > > > > > > sounds a bit like somebody is trying to do exactly that. > > > > > > I think the usecase in mind here is poll(fd, timeout=0) > > > > > Exactly as I feared. Even if userspace polls with timeout=0 you still need > > > > > to call enable_signaling(). > > > > > > > > > > Otherwise you can run into a situation where userspace only uses timeout=0 > > > > > and so never activates the signaling check in the driver. > > > > > > > > > > This would in turn result in an endless loop on implementations where the > > > > > driver never signals fences on their own. > > > > Polling is optional, userspace may never call it. And DRM/KMS or GPU > > > > drivers will be doing fence_wait() themselves so signaling is enabled at > > > > some point. > > > No they won't. We have an use case where we clearly want to avoid that as > > > much as possible because and so the driver never calls enable_signaling() on > > > it's own. > > > > > > Exposing this poll function to userspace without enabling signaling is a > > > clear NAK from my side. > > Okay. So you are NAK'ing the does_not_pool_wait change. Should we revert > > that one then? It is already broken. > > Yeah, that would probably a good idea. The AMD driver changes which really > rely on this aren't upstream yet, so if you point me to the commit hash I > could revert that as well when we send that out. > > On the other hand the idea behind fence_is_signaled() is really that you > check the status multiple times after enabling signaling. So I would prefer > if you would revert this change preliminary. > > Double checking this patch (and thinking about it a bit more) reveals that > it is most likely correct. So feel free to commit this one if it is still > needed for something. It is this patch: ecebca7 dma-buf/sync-file: Avoid enable fence signaling if poll(.timeout=0) But if we revert it as you are a proposing we don't need my patch here anymore. However we would need to revert it now because it is broken. Shall I send a revert part? Gustavo
diff --git a/drivers/dma-buf/fence-array.c b/drivers/dma-buf/fence-array.c index f1989fc..1eec271 100644 --- a/drivers/dma-buf/fence-array.c +++ b/drivers/dma-buf/fence-array.c @@ -75,8 +75,25 @@ static bool fence_array_enable_signaling(struct fence *fence) static bool fence_array_signaled(struct fence *fence) { struct fence_array *array = to_fence_array(fence); + int i, num_pending; + + num_pending = atomic_read(&array->num_pending); + + /* + * Before signaling is enabled, num_pending is static (set during array + * construction as a count of all fences or set to 1 if signal_on_any + * flag is passed. To ensure forward progress, i.e. a while + * (!fence_is_signaled()) ; busy-loop eventually proceeds, we need to + * check the current status of our fences. + */ + if (!test_bit(FENCE_FLAG_ENABLE_SIGNAL_BIT, &fence->flags)) { + for (i = 0 ; i < array->num_fences; ++i) { + if (fence_is_signaled(array->fences[i])) + num_pending--; + } + } - return atomic_read(&array->num_pending) <= 0; + return num_pending <= 0; } static void fence_array_release(struct fence *fence)