Re: [RFC PATCH 2/2 with seqcount v3] reservation: add suppport for read-only access using rcu
op 20-05-14 17:13, Thomas Hellstrom schreef:
On 05/19/2014 03:13 PM, Maarten Lankhorst wrote:
op 19-05-14 15:42, Thomas Hellstrom schreef:
Hi, Maarten!
Some nitpicks, and that krealloc within rcu lock still worries me.
Otherwise looks good.
/Thomas
On 04/23/2014 12:15 PM, Maarten Lankhorst wrote:
@@ -55,8 +60,8 @@ int reservation_object_reserve_shared(struct
reservation_object *obj)
kfree(obj->staged);
obj->staged = NULL;
return 0;
-}
-max = old->shared_max * 2;
+} else
+max = old->shared_max * 2;
Perhaps as a separate reformatting patch?
I'll fold it in to the patch that added
reservation_object_reserve_shared.
+
+int reservation_object_get_fences_rcu(struct reservation_object *obj,
+ struct fence **pfence_excl,
+ unsigned *pshared_count,
+ struct fence ***pshared)
+{
+unsigned shared_count = 0;
+unsigned retry = 1;
+struct fence **shared = NULL, *fence_excl = NULL;
+int ret = 0;
+
+while (retry) {
+struct reservation_object_list *fobj;
+unsigned seq;
+
+seq = read_seqcount_begin(&obj->seq);
+
+rcu_read_lock();
+
+fobj = rcu_dereference(obj->fence);
+if (fobj) {
+struct fence **nshared;
+
+shared_count = ACCESS_ONCE(fobj->shared_count);
ACCESS_ONCE() shouldn't be needed inside the seqlock?
Yes it is, shared_count may be increased, leading to potential
different sizes for krealloc and memcpy
if the ACCESS_ONCE is removed. I could use shared_max here instead,
which stays the same,
but it would waste more memory.
Maarten, Another perhaps ignorant question WRT this,
Does ACCESS_ONCE() guarantee that the value accessed is read atomically?
Well I've reworked the code to use shared_max, so this point is moot. :-)
On any archs I'm aware of it would work, either the old or new value would be
visible, as long as natural alignment is used.
rcu uses the same trick in the rcu_dereference macro, so if this didn't work
rcu wouldn't work either.
~Maarten
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 2/2 with seqcount v3] reservation: add suppport for read-only access using rcu
On 05/19/2014 03:13 PM, Maarten Lankhorst wrote:
> op 19-05-14 15:42, Thomas Hellstrom schreef:
>> Hi, Maarten!
>>
>> Some nitpicks, and that krealloc within rcu lock still worries me.
>> Otherwise looks good.
>>
>> /Thomas
>>
>>
>>
>> On 04/23/2014 12:15 PM, Maarten Lankhorst wrote:
>>> @@ -55,8 +60,8 @@ int reservation_object_reserve_shared(struct
>>> reservation_object *obj)
>>> kfree(obj->staged);
>>> obj->staged = NULL;
>>> return 0;
>>> -}
>>> -max = old->shared_max * 2;
>>> +} else
>>> +max = old->shared_max * 2;
>> Perhaps as a separate reformatting patch?
> I'll fold it in to the patch that added
> reservation_object_reserve_shared.
>>> +
>>> +int reservation_object_get_fences_rcu(struct reservation_object *obj,
>>> + struct fence **pfence_excl,
>>> + unsigned *pshared_count,
>>> + struct fence ***pshared)
>>> +{
>>> +unsigned shared_count = 0;
>>> +unsigned retry = 1;
>>> +struct fence **shared = NULL, *fence_excl = NULL;
>>> +int ret = 0;
>>> +
>>> +while (retry) {
>>> +struct reservation_object_list *fobj;
>>> +unsigned seq;
>>> +
>>> +seq = read_seqcount_begin(&obj->seq);
>>> +
>>> +rcu_read_lock();
>>> +
>>> +fobj = rcu_dereference(obj->fence);
>>> +if (fobj) {
>>> +struct fence **nshared;
>>> +
>>> +shared_count = ACCESS_ONCE(fobj->shared_count);
>> ACCESS_ONCE() shouldn't be needed inside the seqlock?
> Yes it is, shared_count may be increased, leading to potential
> different sizes for krealloc and memcpy
> if the ACCESS_ONCE is removed. I could use shared_max here instead,
> which stays the same,
> but it would waste more memory.
Maarten, Another perhaps ignorant question WRT this,
Does ACCESS_ONCE() guarantee that the value accessed is read atomically?
/Thomas
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 2/2 with seqcount v3] reservation: add suppport for read-only access using rcu
On 05/19/2014 03:13 PM, Maarten Lankhorst wrote:
> op 19-05-14 15:42, Thomas Hellstrom schreef:
>> Hi, Maarten!
>>
>> Some nitpicks, and that krealloc within rcu lock still worries me.
>> Otherwise looks good.
>>
>> /Thomas
>>
>>
>>
>> On 04/23/2014 12:15 PM, Maarten Lankhorst wrote:
>>> @@ -55,8 +60,8 @@ int reservation_object_reserve_shared(struct
>>> reservation_object *obj)
>>> kfree(obj->staged);
>>> obj->staged = NULL;
>>> return 0;
>>> -}
>>> -max = old->shared_max * 2;
>>> +} else
>>> +max = old->shared_max * 2;
>> Perhaps as a separate reformatting patch?
> I'll fold it in to the patch that added
> reservation_object_reserve_shared.
>>> +
>>> +int reservation_object_get_fences_rcu(struct reservation_object *obj,
>>> + struct fence **pfence_excl,
>>> + unsigned *pshared_count,
>>> + struct fence ***pshared)
>>> +{
>>> +unsigned shared_count = 0;
>>> +unsigned retry = 1;
>>> +struct fence **shared = NULL, *fence_excl = NULL;
>>> +int ret = 0;
>>> +
>>> +while (retry) {
>>> +struct reservation_object_list *fobj;
>>> +unsigned seq;
>>> +
>>> +seq = read_seqcount_begin(&obj->seq);
>>> +
>>> +rcu_read_lock();
>>> +
>>> +fobj = rcu_dereference(obj->fence);
>>> +if (fobj) {
>>> +struct fence **nshared;
>>> +
>>> +shared_count = ACCESS_ONCE(fobj->shared_count);
>> ACCESS_ONCE() shouldn't be needed inside the seqlock?
> Yes it is, shared_count may be increased, leading to potential
> different sizes for krealloc and memcpy
> if the ACCESS_ONCE is removed. I could use shared_max here instead,
> which stays the same,
> but it would waste more memory.
OK.
>
>>> +nshared = krealloc(shared, sizeof(*shared) *
>>> shared_count, GFP_KERNEL);
>> Again, krealloc should be a sleeping function, and not suitable within a
>> RCU read lock? I still think this krealloc should be moved to the start
>> of the retry loop, and we should start with a suitable guess of
>> shared_count (perhaps 0?) It's not like we're going to waste a lot of
>> memory
> But shared_count is only known when holding the rcu lock.
>
> What about this change?
Sure. That should work.
/Thomas
>
> @@ -254,16 +254,27 @@ int reservation_object_get_fences_rcu(struct
> reservation_object *obj,
> fobj = rcu_dereference(obj->fence);
> if (fobj) {
> struct fence **nshared;
> +size_t sz;
>
> shared_count = ACCESS_ONCE(fobj->shared_count);
> -nshared = krealloc(shared, sizeof(*shared) *
> shared_count, GFP_KERNEL);
> +sz = sizeof(*shared) * shared_count;
> +
> +nshared = krealloc(shared, sz,
> + GFP_NOWAIT | __GFP_NOWARN);
> if (!nshared) {
> +rcu_read_unlock();
> +nshared = krealloc(shared, sz, GFP_KERNEL)
> +if (nshared) {
> +shared = nshared;
> +continue;
> +}
> +
> ret = -ENOMEM;
> -shared_count = retry = 0;
> -goto unlock;
> +shared_count = 0;
> +break;
> }
> shared = nshared;
> -memcpy(shared, fobj->shared, sizeof(*shared) *
> shared_count);
> +memcpy(shared, fobj->shared, sz);
> } else
> shared_count = 0;
> fence_excl = rcu_dereference(obj->fence_excl);
>
>
>>> +
>>> +/*
>>> + * There could be a read_seqcount_retry here, but nothing
>>> cares
>>> + * about whether it's the old or newer fence pointers that are
>>> + * signale. That race could still have happened after checking
>> Typo.
> Oops
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 2/2 with seqcount v3] reservation: add suppport for read-only access using rcu
op 19-05-14 15:42, Thomas Hellstrom schreef:
Hi, Maarten!
Some nitpicks, and that krealloc within rcu lock still worries me.
Otherwise looks good.
/Thomas
On 04/23/2014 12:15 PM, Maarten Lankhorst wrote:
@@ -55,8 +60,8 @@ int reservation_object_reserve_shared(struct
reservation_object *obj)
kfree(obj->staged);
obj->staged = NULL;
return 0;
-}
-max = old->shared_max * 2;
+} else
+max = old->shared_max * 2;
Perhaps as a separate reformatting patch?
I'll fold it in to the patch that added reservation_object_reserve_shared.
+
+int reservation_object_get_fences_rcu(struct reservation_object *obj,
+ struct fence **pfence_excl,
+ unsigned *pshared_count,
+ struct fence ***pshared)
+{
+unsigned shared_count = 0;
+unsigned retry = 1;
+struct fence **shared = NULL, *fence_excl = NULL;
+int ret = 0;
+
+while (retry) {
+struct reservation_object_list *fobj;
+unsigned seq;
+
+seq = read_seqcount_begin(&obj->seq);
+
+rcu_read_lock();
+
+fobj = rcu_dereference(obj->fence);
+if (fobj) {
+struct fence **nshared;
+
+shared_count = ACCESS_ONCE(fobj->shared_count);
ACCESS_ONCE() shouldn't be needed inside the seqlock?
Yes it is, shared_count may be increased, leading to potential different sizes
for krealloc and memcpy
if the ACCESS_ONCE is removed. I could use shared_max here instead, which stays
the same,
but it would waste more memory.
+nshared = krealloc(shared, sizeof(*shared) *
shared_count, GFP_KERNEL);
Again, krealloc should be a sleeping function, and not suitable within a
RCU read lock? I still think this krealloc should be moved to the start
of the retry loop, and we should start with a suitable guess of
shared_count (perhaps 0?) It's not like we're going to waste a lot of
memory
But shared_count is only known when holding the rcu lock.
What about this change?
@@ -254,16 +254,27 @@ int reservation_object_get_fences_rcu(struct
reservation_object *obj,
fobj = rcu_dereference(obj->fence);
if (fobj) {
struct fence **nshared;
+ size_t sz;
shared_count = ACCESS_ONCE(fobj->shared_count);
- nshared = krealloc(shared, sizeof(*shared) *
shared_count, GFP_KERNEL);
+ sz = sizeof(*shared) * shared_count;
+
+ nshared = krealloc(shared, sz,
+ GFP_NOWAIT | __GFP_NOWARN);
if (!nshared) {
+ rcu_read_unlock();
+ nshared = krealloc(shared, sz, GFP_KERNEL)
+ if (nshared) {
+ shared = nshared;
+ continue;
+ }
+
ret = -ENOMEM;
- shared_count = retry = 0;
- goto unlock;
+ shared_count = 0;
+ break;
}
shared = nshared;
- memcpy(shared, fobj->shared, sizeof(*shared) *
shared_count);
+ memcpy(shared, fobj->shared, sz);
} else
shared_count = 0;
fence_excl = rcu_dereference(obj->fence_excl);
+
+/*
+ * There could be a read_seqcount_retry here, but nothing cares
+ * about whether it's the old or newer fence pointers that are
+ * signale. That race could still have happened after checking
Typo.
Oops.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 2/2 with seqcount v3] reservation: add suppport for read-only access using rcu
Hi, Maarten!
Some nitpicks, and that krealloc within rcu lock still worries me.
Otherwise looks good.
/Thomas
On 04/23/2014 12:15 PM, Maarten Lankhorst wrote:
> This adds 4 more functions to deal with rcu.
>
> reservation_object_get_fences_rcu() will obtain the list of shared
> and exclusive fences without obtaining the ww_mutex.
>
> reservation_object_wait_timeout_rcu() will wait on all fences of the
> reservation_object, without obtaining the ww_mutex.
>
> reservation_object_test_signaled_rcu() will test if all fences of the
> reservation_object are signaled without using the ww_mutex.
>
> reservation_object_get_excl() is added because touching the fence_excl
> member directly will trigger a sparse warning.
>
> Signed-off-by: Maarten Lankhorst
> ---
> Using seqcount and fixing some lockdep bugs.
> Changes since v2:
> - Fix some crashes, remove some unneeded barriers when provided by
> seqcount writes
> - Fix code to work correctly with sparse's RCU annotations.
> - Create a global string for the seqcount lock to make lockdep happy.
>
> Can I get this version reviewed? If it looks correct I'll mail the
> full series
> because it's intertwined with the TTM conversion to use this code.
>
> See
> https://urldefense.proofpoint.com/v1/url?u=http://cgit.freedesktop.org/~mlankhorst/linux/log/?h%3Dvmwgfx_wip&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=l5Ago9ekmVFZ3c4M6eauqrJWGwjf6fTb%2BP3CxbBFkVM%3D%0A&m=SvhtRcVv7pxLHaMBAL4xy2Rr2XDJ%2B95q18FDS13r8FQ%3D%0A&s=07fbe960788dca202856a797e8c915c44f05746a04d899c76459653042ea0112
> ---
> diff --git a/drivers/base/dma-buf.c b/drivers/base/dma-buf.c
> index d89a98d2c37b..0df673f812eb 100644
> --- a/drivers/base/dma-buf.c
> +++ b/drivers/base/dma-buf.c
> @@ -137,7 +137,7 @@ static unsigned int dma_buf_poll(struct file
> *file, poll_table *poll)
> struct reservation_object_list *fobj;
> struct fence *fence_excl;
> unsigned long events;
> -unsigned shared_count;
> +unsigned shared_count, seq;
>
> dmabuf = file->private_data;
> if (!dmabuf || !dmabuf->resv)
> @@ -151,14 +151,20 @@ static unsigned int dma_buf_poll(struct file
> *file, poll_table *poll)
> if (!events)
> return 0;
>
> -ww_mutex_lock(&resv->lock, NULL);
> +retry:
> +seq = read_seqcount_begin(&resv->seq);
> +rcu_read_lock();
>
> -fobj = resv->fence;
> -if (!fobj)
> -goto out;
> -
> -shared_count = fobj->shared_count;
> -fence_excl = resv->fence_excl;
> +fobj = rcu_dereference(resv->fence);
> +if (fobj)
> +shared_count = fobj->shared_count;
> +else
> +shared_count = 0;
> +fence_excl = rcu_dereference(resv->fence_excl);
> +if (read_seqcount_retry(&resv->seq, seq)) {
> +rcu_read_unlock();
> +goto retry;
> +}
>
> if (fence_excl && (!(events & POLLOUT) || shared_count == 0)) {
> struct dma_buf_poll_cb_t *dcb = &dmabuf->cb_excl;
> @@ -176,14 +182,20 @@ static unsigned int dma_buf_poll(struct file
> *file, poll_table *poll)
> spin_unlock_irq(&dmabuf->poll.lock);
>
> if (events & pevents) {
> -if (!fence_add_callback(fence_excl, &dcb->cb,
> +if (!fence_get_rcu(fence_excl)) {
> +/* force a recheck */
> +events &= ~pevents;
> +dma_buf_poll_cb(NULL, &dcb->cb);
> +} else if (!fence_add_callback(fence_excl, &dcb->cb,
> dma_buf_poll_cb)) {
> events &= ~pevents;
> +fence_put(fence_excl);
> } else {
> /*
> * No callback queued, wake up any additional
> * waiters.
> */
> +fence_put(fence_excl);
> dma_buf_poll_cb(NULL, &dcb->cb);
> }
> }
> @@ -205,13 +217,26 @@ static unsigned int dma_buf_poll(struct file
> *file, poll_table *poll)
> goto out;
>
> for (i = 0; i < shared_count; ++i) {
> -struct fence *fence = fobj->shared[i];
> +struct fence *fence = rcu_dereference(fobj->shared[i]);
>
> +if (!fence_get_rcu(fence)) {
> +/*
> + * fence refcount dropped to zero, this means
> + * that fobj has been freed
> + *
> + * call dma_buf_poll_cb and force a recheck!
> + */
> +events &= ~POLLOUT;
> +dma_buf_poll_cb(NULL, &dcb->cb);
> +break;
> +}
> if (!fence_add_callback(fence, &dcb->cb,
> dma_buf_poll_cb)) {
> +fence_put(fence);
> events &= ~POLLOUT;
> break;
> }
> +fence_put(fence);
> }
>
> /* No callback queued, wake up any additional waiters. */
> @@ -220,7 +245,7 @@ static unsigned int dma_buf_poll(struct file
> *file, poll_table *poll)
Re: [RFC PATCH 2/2 with seqcount v3] reservation: add suppport for read-only access using rcu
On 04/29/2014 04:32 PM, Maarten Lankhorst wrote: > op 23-04-14 13:15, Maarten Lankhorst schreef: >> This adds 4 more functions to deal with rcu. >> >> reservation_object_get_fences_rcu() will obtain the list of shared >> and exclusive fences without obtaining the ww_mutex. >> >> reservation_object_wait_timeout_rcu() will wait on all fences of the >> reservation_object, without obtaining the ww_mutex. >> >> reservation_object_test_signaled_rcu() will test if all fences of the >> reservation_object are signaled without using the ww_mutex. >> >> reservation_object_get_excl() is added because touching the fence_excl >> member directly will trigger a sparse warning. >> >> Signed-off-by: Maarten Lankhorst >> --- >> Using seqcount and fixing some lockdep bugs. >> Changes since v2: >> - Fix some crashes, remove some unneeded barriers when provided by >> seqcount writes >> - Fix code to work correctly with sparse's RCU annotations. >> - Create a global string for the seqcount lock to make lockdep happy. >> >> Can I get this version reviewed? If it looks correct I'll mail the >> full series >> because it's intertwined with the TTM conversion to use this code. > Ping, can anyone review this? Hi, Maarten. It's on my todo-list. Been away from office for a while. /Thomas -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 2/2 with seqcount v3] reservation: add suppport for read-only access using rcu
op 23-04-14 13:15, Maarten Lankhorst schreef: This adds 4 more functions to deal with rcu. reservation_object_get_fences_rcu() will obtain the list of shared and exclusive fences without obtaining the ww_mutex. reservation_object_wait_timeout_rcu() will wait on all fences of the reservation_object, without obtaining the ww_mutex. reservation_object_test_signaled_rcu() will test if all fences of the reservation_object are signaled without using the ww_mutex. reservation_object_get_excl() is added because touching the fence_excl member directly will trigger a sparse warning. Signed-off-by: Maarten Lankhorst --- Using seqcount and fixing some lockdep bugs. Changes since v2: - Fix some crashes, remove some unneeded barriers when provided by seqcount writes - Fix code to work correctly with sparse's RCU annotations. - Create a global string for the seqcount lock to make lockdep happy. Can I get this version reviewed? If it looks correct I'll mail the full series because it's intertwined with the TTM conversion to use this code. Ping, can anyone review this? -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
