| Message ID | 1467992691-12442-1-git-send-email-gustavo@padovan.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Op 08-07-16 om 17:44 schreef Gustavo Padovan: > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk> > > Signalling doesn't need to be enabled at sync_file creation, it is only > required if userspace waiting the fence to signal through poll(). > > Thus we delay fence_add_callback() until poll is called. It only adds the > callback the first time poll() is called. This avoid re-adding the same > callback multiple times. > > v2: rebase and update to work with new fence support for sync_file > > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk> > --- > This patch applies on top of my latest sync_file changes to support > fence_array: https://lkml.org/lkml/2016/7/4/534 > > drivers/dma-buf/sync_file.c | 23 ++++++++++++++--------- > include/linux/sync_file.h | 2 ++ > 2 files changed, 16 insertions(+), 9 deletions(-) > > diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c > index 61a687c..1db4a64 100644 > --- a/drivers/dma-buf/sync_file.c > +++ b/drivers/dma-buf/sync_file.c > @@ -86,8 +86,6 @@ struct sync_file *sync_file_create(struct fence *fence) > fence->ops->get_timeline_name(fence), fence->context, > fence->seqno); > > - fence_add_callback(fence, &sync_file->cb, fence_check_cb_func); > - > return sync_file; > } > EXPORT_SYMBOL(sync_file_create); > @@ -269,9 +267,6 @@ static struct sync_file *sync_file_merge(const char *name, struct sync_file *a, > goto err; > } > > - fence_add_callback(sync_file->fence, &sync_file->cb, > - fence_check_cb_func); > - > strlcpy(sync_file->name, name, sizeof(sync_file->name)); > return sync_file; > > @@ -286,7 +281,6 @@ static void sync_file_free(struct kref *kref) > struct sync_file *sync_file = container_of(kref, struct sync_file, > kref); > > - fence_remove_callback(sync_file->fence, &sync_file->cb); > fence_put(sync_file->fence); > kfree(sync_file); > } > @@ -306,13 +300,24 @@ static unsigned int sync_file_poll(struct file *file, poll_table *wait) > > poll_wait(file, &sync_file->wq, wait); > > + if (!sync_file->enabled) { > + fence_add_callback(sync_file->fence, &sync_file->cb, > + fence_check_cb_func); > + sync_file->enabled = true; > + } Won't this blow up completely with 2 threads polling at the same time? ~Maarten
2016-07-10 Maarten Lankhorst <maarten.lankhorst@linux.intel.com>: > Op 08-07-16 om 17:44 schreef Gustavo Padovan: > > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk> > > > > Signalling doesn't need to be enabled at sync_file creation, it is only > > required if userspace waiting the fence to signal through poll(). > > > > Thus we delay fence_add_callback() until poll is called. It only adds the > > callback the first time poll() is called. This avoid re-adding the same > > callback multiple times. > > > > v2: rebase and update to work with new fence support for sync_file > > > > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk> > > --- > > This patch applies on top of my latest sync_file changes to support > > fence_array: https://lkml.org/lkml/2016/7/4/534 > > > > drivers/dma-buf/sync_file.c | 23 ++++++++++++++--------- > > include/linux/sync_file.h | 2 ++ > > 2 files changed, 16 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c > > index 61a687c..1db4a64 100644 > > --- a/drivers/dma-buf/sync_file.c > > +++ b/drivers/dma-buf/sync_file.c > > @@ -86,8 +86,6 @@ struct sync_file *sync_file_create(struct fence *fence) > > fence->ops->get_timeline_name(fence), fence->context, > > fence->seqno); > > > > - fence_add_callback(fence, &sync_file->cb, fence_check_cb_func); > > - > > return sync_file; > > } > > EXPORT_SYMBOL(sync_file_create); > > @@ -269,9 +267,6 @@ static struct sync_file *sync_file_merge(const char *name, struct sync_file *a, > > goto err; > > } > > > > - fence_add_callback(sync_file->fence, &sync_file->cb, > > - fence_check_cb_func); > > - > > strlcpy(sync_file->name, name, sizeof(sync_file->name)); > > return sync_file; > > > > @@ -286,7 +281,6 @@ static void sync_file_free(struct kref *kref) > > struct sync_file *sync_file = container_of(kref, struct sync_file, > > kref); > > > > - fence_remove_callback(sync_file->fence, &sync_file->cb); > > fence_put(sync_file->fence); > > kfree(sync_file); > > } > > @@ -306,13 +300,24 @@ static unsigned int sync_file_poll(struct file *file, poll_table *wait) > > > > poll_wait(file, &sync_file->wq, wait); > > > > + if (!sync_file->enabled) { > > + fence_add_callback(sync_file->fence, &sync_file->cb, > > + fence_check_cb_func); > > + sync_file->enabled = true; > > + } > Won't this blow up completely with 2 threads polling at the same time? Indeed, using atomic operations on enabled should fix this. Gustavo
Op 11-07-16 om 22:27 schreef Gustavo Padovan: > 2016-07-10 Maarten Lankhorst <maarten.lankhorst@linux.intel.com>: > >> Op 08-07-16 om 17:44 schreef Gustavo Padovan: >>> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk> >>> >>> Signalling doesn't need to be enabled at sync_file creation, it is only >>> required if userspace waiting the fence to signal through poll(). >>> >>> Thus we delay fence_add_callback() until poll is called. It only adds the >>> callback the first time poll() is called. This avoid re-adding the same >>> callback multiple times. >>> >>> v2: rebase and update to work with new fence support for sync_file >>> >>> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk> >>> --- >>> This patch applies on top of my latest sync_file changes to support >>> fence_array: https://lkml.org/lkml/2016/7/4/534 >>> >>> drivers/dma-buf/sync_file.c | 23 ++++++++++++++--------- >>> include/linux/sync_file.h | 2 ++ >>> 2 files changed, 16 insertions(+), 9 deletions(-) >>> >>> diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c >>> index 61a687c..1db4a64 100644 >>> --- a/drivers/dma-buf/sync_file.c >>> +++ b/drivers/dma-buf/sync_file.c >>> @@ -86,8 +86,6 @@ struct sync_file *sync_file_create(struct fence *fence) >>> fence->ops->get_timeline_name(fence), fence->context, >>> fence->seqno); >>> >>> - fence_add_callback(fence, &sync_file->cb, fence_check_cb_func); >>> - >>> return sync_file; >>> } >>> EXPORT_SYMBOL(sync_file_create); >>> @@ -269,9 +267,6 @@ static struct sync_file *sync_file_merge(const char *name, struct sync_file *a, >>> goto err; >>> } >>> >>> - fence_add_callback(sync_file->fence, &sync_file->cb, >>> - fence_check_cb_func); >>> - >>> strlcpy(sync_file->name, name, sizeof(sync_file->name)); >>> return sync_file; >>> >>> @@ -286,7 +281,6 @@ static void sync_file_free(struct kref *kref) >>> struct sync_file *sync_file = container_of(kref, struct sync_file, >>> kref); >>> >>> - fence_remove_callback(sync_file->fence, &sync_file->cb); >>> fence_put(sync_file->fence); >>> kfree(sync_file); >>> } >>> @@ -306,13 +300,24 @@ static unsigned int sync_file_poll(struct file *file, poll_table *wait) >>> >>> poll_wait(file, &sync_file->wq, wait); >>> >>> + if (!sync_file->enabled) { >>> + fence_add_callback(sync_file->fence, &sync_file->cb, >>> + fence_check_cb_func); >>> + sync_file->enabled = true; >>> + } >> Won't this blow up completely with 2 threads polling at the same time? > Indeed, using atomic operations on enabled should fix this. No, it still would blow up without locking around fence_remove/add_callback too.. Personally I would just add the callback once, then remove it in destructor. Something like: poll: if (!atomic_xchg(&sync_file->enabled, 1)) { if (fence_add_callback(...) < 0) wake up sync_file->wq, fence is signaled } sync_file_free: if (atomic_read(&sync_file->enabled)) fence_remove_callback(...); fence_put() It's not like fence can disable hw signaling when all callbacks are removed anyway, it's harmless to keep it on the list. ~Maarten
2016-07-12 Maarten Lankhorst <maarten.lankhorst@linux.intel.com>: > Op 11-07-16 om 22:27 schreef Gustavo Padovan: > > 2016-07-10 Maarten Lankhorst <maarten.lankhorst@linux.intel.com>: > > > >> Op 08-07-16 om 17:44 schreef Gustavo Padovan: > >>> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk> > >>> > >>> Signalling doesn't need to be enabled at sync_file creation, it is only > >>> required if userspace waiting the fence to signal through poll(). > >>> > >>> Thus we delay fence_add_callback() until poll is called. It only adds the > >>> callback the first time poll() is called. This avoid re-adding the same > >>> callback multiple times. > >>> > >>> v2: rebase and update to work with new fence support for sync_file > >>> > >>> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk> > >>> --- > >>> This patch applies on top of my latest sync_file changes to support > >>> fence_array: https://lkml.org/lkml/2016/7/4/534 > >>> > >>> drivers/dma-buf/sync_file.c | 23 ++++++++++++++--------- > >>> include/linux/sync_file.h | 2 ++ > >>> 2 files changed, 16 insertions(+), 9 deletions(-) > >>> > >>> diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c > >>> index 61a687c..1db4a64 100644 > >>> --- a/drivers/dma-buf/sync_file.c > >>> +++ b/drivers/dma-buf/sync_file.c > >>> @@ -86,8 +86,6 @@ struct sync_file *sync_file_create(struct fence *fence) > >>> fence->ops->get_timeline_name(fence), fence->context, > >>> fence->seqno); > >>> > >>> - fence_add_callback(fence, &sync_file->cb, fence_check_cb_func); > >>> - > >>> return sync_file; > >>> } > >>> EXPORT_SYMBOL(sync_file_create); > >>> @@ -269,9 +267,6 @@ static struct sync_file *sync_file_merge(const char *name, struct sync_file *a, > >>> goto err; > >>> } > >>> > >>> - fence_add_callback(sync_file->fence, &sync_file->cb, > >>> - fence_check_cb_func); > >>> - > >>> strlcpy(sync_file->name, name, sizeof(sync_file->name)); > >>> return sync_file; > >>> > >>> @@ -286,7 +281,6 @@ static void sync_file_free(struct kref *kref) > >>> struct sync_file *sync_file = container_of(kref, struct sync_file, > >>> kref); > >>> > >>> - fence_remove_callback(sync_file->fence, &sync_file->cb); > >>> fence_put(sync_file->fence); > >>> kfree(sync_file); > >>> } > >>> @@ -306,13 +300,24 @@ static unsigned int sync_file_poll(struct file *file, poll_table *wait) > >>> > >>> poll_wait(file, &sync_file->wq, wait); > >>> > >>> + if (!sync_file->enabled) { > >>> + fence_add_callback(sync_file->fence, &sync_file->cb, > >>> + fence_check_cb_func); > >>> + sync_file->enabled = true; > >>> + } > >> Won't this blow up completely with 2 threads polling at the same time? > > Indeed, using atomic operations on enabled should fix this. > No, it still would blow up without locking around fence_remove/add_callback too.. > > Personally I would just add the callback once, then remove it in destructor. > > Something like: > > poll: > if (!atomic_xchg(&sync_file->enabled, 1)) { > if (fence_add_callback(...) < 0) > wake up sync_file->wq, fence is signaled > } > > sync_file_free: > if (atomic_read(&sync_file->enabled)) > fence_remove_callback(...); > > fence_put() > > It's not like fence can disable hw signaling when all callbacks are removed anyway, > it's harmless to keep it on the list. Move the callback removal to sync_file_free() seems a good idea to me. I'll rework this patch taking in account your suggestions and resend it. Gustavo
On Tue, Jul 12, 2016 at 10:46:45AM +0200, Maarten Lankhorst wrote: > Op 11-07-16 om 22:27 schreef Gustavo Padovan: > > 2016-07-10 Maarten Lankhorst <maarten.lankhorst@linux.intel.com>: > > > >> Op 08-07-16 om 17:44 schreef Gustavo Padovan: > >>> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk> > >>> > >>> Signalling doesn't need to be enabled at sync_file creation, it is only > >>> required if userspace waiting the fence to signal through poll(). > >>> > >>> Thus we delay fence_add_callback() until poll is called. It only adds the > >>> callback the first time poll() is called. This avoid re-adding the same > >>> callback multiple times. > >>> > >>> v2: rebase and update to work with new fence support for sync_file > >>> > >>> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk> > >>> --- > >>> This patch applies on top of my latest sync_file changes to support > >>> fence_array: https://lkml.org/lkml/2016/7/4/534 > >>> > >>> drivers/dma-buf/sync_file.c | 23 ++++++++++++++--------- > >>> include/linux/sync_file.h | 2 ++ > >>> 2 files changed, 16 insertions(+), 9 deletions(-) > >>> > >>> diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c > >>> index 61a687c..1db4a64 100644 > >>> --- a/drivers/dma-buf/sync_file.c > >>> +++ b/drivers/dma-buf/sync_file.c > >>> @@ -86,8 +86,6 @@ struct sync_file *sync_file_create(struct fence *fence) > >>> fence->ops->get_timeline_name(fence), fence->context, > >>> fence->seqno); > >>> > >>> - fence_add_callback(fence, &sync_file->cb, fence_check_cb_func); > >>> - > >>> return sync_file; > >>> } > >>> EXPORT_SYMBOL(sync_file_create); > >>> @@ -269,9 +267,6 @@ static struct sync_file *sync_file_merge(const char *name, struct sync_file *a, > >>> goto err; > >>> } > >>> > >>> - fence_add_callback(sync_file->fence, &sync_file->cb, > >>> - fence_check_cb_func); > >>> - > >>> strlcpy(sync_file->name, name, sizeof(sync_file->name)); > >>> return sync_file; > >>> > >>> @@ -286,7 +281,6 @@ static void sync_file_free(struct kref *kref) > >>> struct sync_file *sync_file = container_of(kref, struct sync_file, > >>> kref); > >>> > >>> - fence_remove_callback(sync_file->fence, &sync_file->cb); > >>> fence_put(sync_file->fence); > >>> kfree(sync_file); > >>> } > >>> @@ -306,13 +300,24 @@ static unsigned int sync_file_poll(struct file *file, poll_table *wait) > >>> > >>> poll_wait(file, &sync_file->wq, wait); > >>> > >>> + if (!sync_file->enabled) { > >>> + fence_add_callback(sync_file->fence, &sync_file->cb, > >>> + fence_check_cb_func); > >>> + sync_file->enabled = true; > >>> + } > >> Won't this blow up completely with 2 threads polling at the same time? > > Indeed, using atomic operations on enabled should fix this. > No, it still would blow up without locking around fence_remove/add_callback too.. > > Personally I would just add the callback once, then remove it in destructor. > > Something like: > > poll: > if (!atomic_xchg(&sync_file->enabled, 1)) { > if (fence_add_callback(...) < 0) > wake up sync_file->wq, fence is signaled > } > > sync_file_free: > if (atomic_read(&sync_file->enabled)) > fence_remove_callback(...); > > fence_put() > > It's not like fence can disable hw signaling when all callbacks are removed anyway, > it's harmless to keep it on the list. +1 from me on this plan, removing the callbacks doesn't provide any value (before the sync_file is destroyed). Also, when someone polls, you can expect to get repolled a lot until the fences finally all signal. -Daniel
diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c index 61a687c..1db4a64 100644 --- a/drivers/dma-buf/sync_file.c +++ b/drivers/dma-buf/sync_file.c @@ -86,8 +86,6 @@ struct sync_file *sync_file_create(struct fence *fence) fence->ops->get_timeline_name(fence), fence->context, fence->seqno); - fence_add_callback(fence, &sync_file->cb, fence_check_cb_func); - return sync_file; } EXPORT_SYMBOL(sync_file_create); @@ -269,9 +267,6 @@ static struct sync_file *sync_file_merge(const char *name, struct sync_file *a, goto err; } - fence_add_callback(sync_file->fence, &sync_file->cb, - fence_check_cb_func); - strlcpy(sync_file->name, name, sizeof(sync_file->name)); return sync_file; @@ -286,7 +281,6 @@ static void sync_file_free(struct kref *kref) struct sync_file *sync_file = container_of(kref, struct sync_file, kref); - fence_remove_callback(sync_file->fence, &sync_file->cb); fence_put(sync_file->fence); kfree(sync_file); } @@ -306,13 +300,24 @@ static unsigned int sync_file_poll(struct file *file, poll_table *wait) poll_wait(file, &sync_file->wq, wait); + if (!sync_file->enabled) { + fence_add_callback(sync_file->fence, &sync_file->cb, + fence_check_cb_func); + sync_file->enabled = true; + } + status = fence_is_signaled(sync_file->fence); - if (status) - return POLLIN; + if (!status) + return 0; + + fence_remove_callback(sync_file->fence, &sync_file->cb); + sync_file->enabled = false; + if (status < 0) return POLLERR; - return 0; + + return POLLIN; } static long sync_file_ioctl_merge(struct sync_file *sync_file, diff --git a/include/linux/sync_file.h b/include/linux/sync_file.h index f7de5a0..16cbafd 100644 --- a/include/linux/sync_file.h +++ b/include/linux/sync_file.h @@ -28,6 +28,7 @@ * @name: name of sync_file. Useful for debugging * @sync_file_list: membership in global file list * @wq: wait queue for fence signaling + * @enabled: wether fence signal is enabled or not * @fence: fence with the fences in the sync_file * @cb: fence callback information */ @@ -40,6 +41,7 @@ struct sync_file { #endif wait_queue_head_t wq; + bool enabled; struct fence *fence; struct fence_cb cb;