Updates to fence api

This adds 3 more functions to deal with rcu.

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 <[email protected]>

---
 drivers/base/dma-buf.c      |   46 +++++++++++--
 drivers/base/reservation.c  |  147 +++++++++++++++++++++++++++++++++++++++++--
 include/linux/fence.h       |   22 ++++++
 include/linux/reservation.h |   40 ++++++++----
 4 files changed, 224 insertions(+), 31 deletions(-)

Hi!

Ugh. This became more complicated than I thought, but I'm OK with moving TTM over to fence while we sort out
how / if we're going to use this.

While reviewing, it struck me that this is kind of error-prone, and hard to follow since we're operating on a structure that may be continually updated under us, needing a lot of RCU-specific macros and barriers.

Also the rcu wait appears to not complete until there are no busy fences left (new ones can be added while we wait) rather than waiting on a snapshot of busy fences.

I wonder if these issues can be addressed by having a function that provides a snapshot of all busy fences: This can be accomplished either by including the exclusive fence in the fence_list structure and allocate a new such structure each time it is updated. The RCU reader could then just make a copy of the current fence_list structure pointed to by &obj->fence, but I'm not sure we want to reallocate *each* time we update the fence pointer.

The other approach uses a seqlock to obtain a consistent snapshot, and I've attached an incomplete outline, and I'm not 100% whether it's OK to combine RCU and seqlocks in this way...

Both these approaches have the benefit of hiding the RCU snapshotting in a single function, that can then be used by any waiting
or polling function.

Hey,

op 10-04-14 10:46, Thomas Hellstrom schreef:

Quoted 10 lines

Hi!

Ugh. This became more complicated than I thought, but I'm OK with moving TTM over to fence while we sort out
how / if we're going to use this.

While reviewing, it struck me that this is kind of error-prone, and hard to follow since we're operating on a structure that may be continually updated under us, needing a lot of RCU-specific macros and barriers.

Yeah, but with the exception of dma_buf_poll I don't think there is anything else
outside drivers/base/reservation.c has to deal with rcu.

Quoted 3 lines

Also the rcu wait appears to not complete until there are no busy fences left (new ones can be added while we wait) rather than waiting on a snapshot of busy fences.

This has been by design, because 'wait for bo idle' type of functions only care
if the bo is completely idle or not.

It would be easy to make a snapshot even without seqlocks, just copy reservation_object_test_signaled_rcu to return a shared list if test_all is set, or return pointer to exclusive otherwise.

Quoted 7 lines

I wonder if these issues can be addressed by having a function that provides a snapshot of all busy fences: This can be accomplished either by including the exclusive fence in the fence_list structure and allocate a new such structure each time it is updated. The RCU reader could then just make a copy of the current fence_list structure pointed to by &obj->fence, but I'm not sure we want to reallocate *each* time we update the fence pointer.

No, the most common operation is updating fence pointers, which is why the current design makes that cheap. It's also why doing rcu reads is more expensive.

Quoted 8 lines

The other approach uses a seqlock to obtain a consistent snapshot, and I've attached an incomplete outline, and I'm not 100% whether it's OK to combine RCU and seqlocks in this way...

Both these approaches have the benefit of hiding the RCU snapshotting in a single function, that can then be used by any waiting
or polling function.

I think the middle way with using seqlocks to protect the fence_excl pointer and shared list combination,
and using RCU to protect the refcounts for fences and the availability of the list could work for our usecase
and might remove a bunch of memory barriers. But yeah that depends on layering rcu and seqlocks.
No idea if that is allowed. But I suppose it is.

Also, you're being overly paranoid with seqlock reading, we would only need something like this:

rcu_read_lock()
preempt_disable()
seq = read_seqcount_begin()
read fence_excl, shared_count = ACCESS_ONCE(fence->shared_count) copy shared to a struct.
if (read_seqcount_retry()) { unlock and retry }
preempt_enable();
use fence_get_rcu() to bump refcount on everything, if that fails
unlock, put, and retry
rcu_read_unlock()

But the shared list would still need to be RCU'd, to make sure we're not reading freed garbage.

~Maarten

Quoted 17 lines · Maarten Lankhorst

Hey,

op 10-04-14 10:46, Thomas Hellstrom schreef:

Quoted 10 lines

Hi!

Ugh. This became more complicated than I thought, but I'm OK with moving TTM over to fence while we sort out
how / if we're going to use this.

While reviewing, it struck me that this is kind of error-prone, and hard to follow since we're operating on a structure that may be continually updated under us, needing a lot of RCU-specific macros and barriers.

Yeah, but with the exception of dma_buf_poll I don't think there is anything else
outside drivers/base/reservation.c has to deal with rcu.

Quoted 6 lines

Quoted 3 lines

Also the rcu wait appears to not complete until there are no busy fences left (new ones can be added while we wait) rather than waiting on a snapshot of busy fences.

This has been by design, because 'wait for bo idle' type of functions only care
if the bo is completely idle or not.

No, not when using RCU, because the bo may be busy again before the function returns :)
Complete idleness can only be guaranteed if holding the reservation, or otherwise making sure
that no new rendering is submitted to the buffer, so it's an overkill to wait for complete idleness here.

Quoted 17 lines

Hey,

op 10-04-14 10:46, Thomas Hellstrom schreef:

Quoted 10 lines

Hi!

Ugh. This became more complicated than I thought, but I'm OK with moving TTM over to fence while we sort out
how / if we're going to use this.

While reviewing, it struck me that this is kind of error-prone, and hard to follow since we're operating on a structure that may be continually updated under us, needing a lot of RCU-specific macros and barriers.

Yeah, but with the exception of dma_buf_poll I don't think there is anything else
outside drivers/base/reservation.c has to deal with rcu.

Quoted 6 lines

Quoted 3 lines

Also the rcu wait appears to not complete until there are no busy fences left (new ones can be added while we wait) rather than waiting on a snapshot of busy fences.

This has been by design, because 'wait for bo idle' type of functions only care
if the bo is completely idle or not.

No, not when using RCU, because the bo may be busy again before the function returns :)
Complete idleness can only be guaranteed if holding the reservation, or otherwise making sure
that no new rendering is submitted to the buffer, so it's an overkill to wait for complete idleness here.

Quoted 44 lines

It would be easy to make a snapshot even without seqlocks, just copy reservation_object_test_signaled_rcu to return a shared list if test_all is set, or return pointer to exclusive otherwise.

Quoted 7 lines

I wonder if these issues can be addressed by having a function that provides a snapshot of all busy fences: This can be accomplished either by including the exclusive fence in the fence_list structure and allocate a new such structure each time it is updated. The RCU reader could then just make a copy of the current fence_list structure pointed to by &obj->fence, but I'm not sure we want to reallocate *each* time we update the fence pointer.

No, the most common operation is updating fence pointers, which is why the current design makes that cheap. It's also why doing rcu reads is more expensive.

Quoted 8 lines

The other approach uses a seqlock to obtain a consistent snapshot, and I've attached an incomplete outline, and I'm not 100% whether it's OK to combine RCU and seqlocks in this way...

Both these approaches have the benefit of hiding the RCU snapshotting in a single function, that can then be used by any waiting
or polling function.

I think the middle way with using seqlocks to protect the fence_excl pointer and shared list combination,
and using RCU to protect the refcounts for fences and the availability of the list could work for our usecase
and might remove a bunch of memory barriers. But yeah that depends on layering rcu and seqlocks.
No idea if that is allowed. But I suppose it is.

Also, you're being overly paranoid with seqlock reading, we would only need something like this:

rcu_read_lock()
preempt_disable()
seq = read_seqcount_begin()
read fence_excl, shared_count = ACCESS_ONCE(fence->shared_count) copy shared to a struct.
if (read_seqcount_retry()) { unlock and retry }
preempt_enable();
use fence_get_rcu() to bump refcount on everything, if that fails
unlock, put, and retry
rcu_read_unlock()

Quoted 2 lines

But the shared list would still need to be RCU'd, to make sure we're not reading freed garbage.

Ah. OK,
But I think we should use rcu inside seqcount, because read_seqcount_begin() may spin for a long time if there are many writers. Also I don't think the preempt_disable() is needed for read_seq critical sections other than they might
decrease the risc of retries..

op 10-04-14 13:08, Thomas Hellstrom schreef:

Quoted 27 lines · Maarten Lankhorst

Quoted 19 lines

Hey,

op 10-04-14 10:46, Thomas Hellstrom schreef:

Quoted 7 lines

Hi!

Ugh. This became more complicated than I thought, but I'm OK with moving
TTM over to fence while we sort out
how / if we're going to use this.

Quoted 5 lines

While reviewing, it struck me that this is kind of error-prone, and hard
to follow since we're operating on a structure that may be continually updated under us, needing a lot of RCU-specific macros and barriers.

Yeah, but with the exception of dma_buf_poll I don't think there is anything else
outside drivers/base/reservation.c has to deal with rcu.

Quoted 7 lines

Quoted 4 lines

Also the rcu wait appears to not complete until there are no busy fences
left (new ones can be added while we wait) rather than waiting on a snapshot of busy fences.

This has been by design, because 'wait for bo idle' type of functions only care
if the bo is completely idle or not.

Quoted 6 lines

No, not when using RCU, because the bo may be busy again before the function returns :)
Complete idleness can only be guaranteed if holding the reservation, or otherwise making sure
that no new rendering is submitted to the buffer, so it's an overkill to wait for complete idleness here.

You're probably right, but it makes waiting a lot easier if I don't have to deal with memory allocations. :P

Quoted 49 lines

Quoted 49 lines

It would be easy to make a snapshot even without seqlocks, just copy reservation_object_test_signaled_rcu to return a shared list if test_all is set, or return pointer to exclusive otherwise.

Quoted 10 lines

I wonder if these issues can be addressed by having a function that provides a snapshot of all busy fences: This can be accomplished either by including the exclusive fence in the fence_list structure and
allocate a new such structure each time it is updated. The RCU reader could then just make a copy of the current fence_list structure pointed
to by &obj->fence, but I'm not sure we want to reallocate *each* time we
update the fence pointer.

No, the most common operation is updating fence pointers, which is why the current design makes that cheap. It's also why doing rcu reads is more expensive.

Quoted 10 lines

The other approach uses a seqlock to obtain a consistent snapshot, and I've attached an incomplete outline, and I'm not 100% whether it's OK to
combine RCU and seqlocks in this way...

Both these approaches have the benefit of hiding the RCU snapshotting in
a single function, that can then be used by any waiting or polling function.

I think the middle way with using seqlocks to protect the fence_excl pointer and shared list combination,
and using RCU to protect the refcounts for fences and the availability of the list could work for our usecase
and might remove a bunch of memory barriers. But yeah that depends on layering rcu and seqlocks.
No idea if that is allowed. But I suppose it is.

Also, you're being overly paranoid with seqlock reading, we would only need something like this:

rcu_read_lock()
preempt_disable()
seq = read_seqcount_begin()
read fence_excl, shared_count = ACCESS_ONCE(fence->shared_count) copy shared to a struct.
if (read_seqcount_retry()) { unlock and retry }
preempt_enable();
use fence_get_rcu() to bump refcount on everything, if that fails
unlock, put, and retry
rcu_read_unlock()

Quoted 9 lines

Quoted 2 lines

But the shared list would still need to be RCU'd, to make sure we're not reading freed garbage.

Ah. OK,
But I think we should use rcu inside seqcount, because read_seqcount_begin() may spin for a long time if there are many writers. Also I don't think the preempt_disable() is needed for read_seq critical sections other than they might
decrease the risc of retries..

Reading the seqlock code makes me suspect that's the case too. The lockdep code calls
local_irq_disable, so it's probably safe without preemption disabled.

~Maarten

I like the ability of not allocating memory, so I kept reservation_object_wait_timeout_rcu mostly
the way it was. This code appears to fail on nouveau when using the shared members,
but I'm not completely sure whether the error is in nouveau or this code yet.

--8<--------
[RFC v2] reservation: add suppport for read-only access using rcu

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 <[email protected]>

drivers/base/dma-buf.c +3 -0

index d89a98d2c37b..ca6ef0c4b358 100644
 
+int reservation_object_get_fences_rcu(struct reservation_object *obj,
+                      struct fence **pfence_excl,
+                      unsigned *pshared_count,

drivers/base/dma-buf.c +11 -0

+
+        seq = read_seqcount_begin(&obj->seq);
+
+        rcu_read_lock();
+
+        fobj = ACCESS_ONCE(obj->fence);
+        if (fobj) {
+            struct fence **nshared;
+
+            shared_count = ACCESS_ONCE(fobj->shared_count);
+            nshared = krealloc(shared, sizeof(*shared) *

shared_count, GFP_KERNEL);

drivers/base/dma-buf.c +7 -0

+            if (!nshared) {
+                ret = -ENOMEM;
+                shared_count = retry = 0;
+                goto unlock;
+            }
+            shared = nshared;
+            memcpy(shared, fobj->shared, sizeof(*shared) *

shared_count);

drivers/base/dma-buf.c +36 -0

+        } else
+            shared_count = 0;
+        fence_excl = obj->fence_excl;
+
+        retry = read_seqcount_retry(&obj->seq, seq);
+        if (retry)
+            goto unlock;
+
+        if (!fence_excl || fence_get_rcu(fence_excl)) {
+            unsigned i;
+
+            for (i = 0; i < shared_count; ++i) {
+                if (fence_get_rcu(shared[i]))
+                    continue;
+
+                /* uh oh, refcount failed, abort and retry */
+                while (i--)
+                    fence_put(shared[i]);
+
+                if (fence_excl) {
+                    fence_put(fence_excl);
+                    fence_excl = NULL;
+                }
+
+                retry = 1;
+                break;
+            }
+        } else
+            retry = 1;
+
+unlock:
+        rcu_read_unlock();
+    }
+    *pshared_count = shared_count;
+    if (shared_count)
+        *pshared = shared;

Hi, Maarten.

Here I believe we encounter a lot of locking inconsistencies.

First, it seems you're use a number of pointers as RCU pointers without annotating them as such and use the correct rcu
macros when assigning those pointers.

Some pointers (like the pointers in the shared fence list) are both used as RCU pointers (in dma_buf_poll()) for example,
or considered protected by the seqlock (reservation_object_get_fences_rcu()), which I believe is OK, but then the pointers must
be assigned using the correct rcu macros. In the memcpy in reservation_object_get_fences_rcu() we might get away with an ugly typecast, but with a verbose comment that the pointers are considered protected by the seqlock at that location.

So I've updated (attached) the headers with proper __rcu annotation and locking comments according to how they are being used in the various reading functions.
I believe if we want to get rid of this we need to validate those pointers using the seqlock as well.
This will generate a lot of sparse warnings in those places needing rcu_dereference()
rcu_assign_pointer()
rcu_dereference_protected()

With this I think we can get rid of all ACCESS_ONCE macros: It's not needed when the rcu_x() macros are used, and
it's never needed for the members protected by the seqlock, (provided that the seq is tested). The only place where I think that's *not* the case is at the krealloc in reservation_object_get_fences_rcu().

Also I have some more comments in the reservation_object_get_fences_rcu() function below:

drivers/base/dma-buf.c +13 -0

index d89a98d2c37b..ca6ef0c4b358 100644
 
+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, retry;

You're shadowing retry?

Quoted 12 lines

(quoted diff) +11 -0

+
+        seq = read_seqcount_begin(&obj->seq);
+
+        rcu_read_lock();
+
+        fobj = ACCESS_ONCE(obj->fence);
+        if (fobj) {
+            struct fence **nshared;
+
+            shared_count = ACCESS_ONCE(fobj->shared_count);
+            nshared = krealloc(shared, sizeof(*shared) *

Quoted 1 line

shared_count, GFP_KERNEL);

krealloc inside rcu_read_lock(). Better to put this first in the loop.

Thanks,
Thomas

op 11-04-14 10:38, Thomas Hellstrom schreef:

Quoted 35 lines

Hi, Maarten.

Here I believe we encounter a lot of locking inconsistencies.

First, it seems you're use a number of pointers as RCU pointers without annotating them as such and use the correct rcu
macros when assigning those pointers.

Some pointers (like the pointers in the shared fence list) are both used as RCU pointers (in dma_buf_poll()) for example,
or considered protected by the seqlock (reservation_object_get_fences_rcu()), which I believe is OK, but then the pointers must
be assigned using the correct rcu macros. In the memcpy in reservation_object_get_fences_rcu() we might get away with an ugly typecast, but with a verbose comment that the pointers are considered protected by the seqlock at that location.

So I've updated (attached) the headers with proper __rcu annotation and locking comments according to how they are being used in the various reading functions.
I believe if we want to get rid of this we need to validate those pointers using the seqlock as well.
This will generate a lot of sparse warnings in those places needing rcu_dereference()
rcu_assign_pointer()
rcu_dereference_protected()

With this I think we can get rid of all ACCESS_ONCE macros: It's not needed when the rcu_x() macros are used, and
it's never needed for the members protected by the seqlock, (provided that the seq is tested). The only place where I think that's *not* the case is at the krealloc in reservation_object_get_fences_rcu().

Quoted 2 lines

Also I have some more comments in the reservation_object_get_fences_rcu() function below:

I felt that the barriers needed for rcu were already provided by checking the seqcount lock.
But looking at rcu_dereference makes it seem harmless to add it in more places, it handles
the ACCESS_ONCE and barrier() for us.

We could probably get away with using RCU_INIT_POINTER on the writer side,
because the smp_wmb is already done by arranging seqcount updates correctly.

Quoted 19 lines

drivers/base/dma-buf.c +13 -0

index d89a98d2c37b..ca6ef0c4b358 100644
 
+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, retry;

Quoted 1 line

You're shadowing retry?

Oops.

Quoted 14 lines

Quoted 12 lines

(quoted diff) +11 -0

+
+        seq = read_seqcount_begin(&obj->seq);
+
+        rcu_read_lock();
+
+        fobj = ACCESS_ONCE(obj->fence);
+        if (fobj) {
+            struct fence **nshared;
+
+            shared_count = ACCESS_ONCE(fobj->shared_count);
+            nshared = krealloc(shared, sizeof(*shared) *

Quoted 1 line

shared_count, GFP_KERNEL);

krealloc inside rcu_read_lock(). Better to put this first in the loop.

Except that shared_count isn't known until the rcu_read_lock is taken.

Quoted 2 lines

Thanks,
Thomas

~Maarten

Quoted 36 lines

op 11-04-14 10:38, Thomas Hellstrom schreef:

Quoted 35 lines

Hi, Maarten.

Here I believe we encounter a lot of locking inconsistencies.

First, it seems you're use a number of pointers as RCU pointers without annotating them as such and use the correct rcu
macros when assigning those pointers.

Some pointers (like the pointers in the shared fence list) are both used as RCU pointers (in dma_buf_poll()) for example,
or considered protected by the seqlock (reservation_object_get_fences_rcu()), which I believe is OK, but then the pointers must
be assigned using the correct rcu macros. In the memcpy in reservation_object_get_fences_rcu() we might get away with an ugly typecast, but with a verbose comment that the pointers are considered protected by the seqlock at that location.

So I've updated (attached) the headers with proper __rcu annotation and locking comments according to how they are being used in the various reading functions.
I believe if we want to get rid of this we need to validate those pointers using the seqlock as well.
This will generate a lot of sparse warnings in those places needing rcu_dereference()
rcu_assign_pointer()
rcu_dereference_protected()

With this I think we can get rid of all ACCESS_ONCE macros: It's not needed when the rcu_x() macros are used, and
it's never needed for the members protected by the seqlock, (provided that the seq is tested). The only place where I think that's *not* the case is at the krealloc in reservation_object_get_fences_rcu().

Quoted 7 lines

Quoted 2 lines

Also I have some more comments in the reservation_object_get_fences_rcu() function below:

I felt that the barriers needed for rcu were already provided by checking the seqcount lock.
But looking at rcu_dereference makes it seem harmless to add it in more places, it handles
the ACCESS_ONCE and barrier() for us.

And it makes the code more maintainable, and helps sparse doing a lot of checking for us. I guess
we can tolerate a couple of extra barriers for that.

Quoted 4 lines

We could probably get away with using RCU_INIT_POINTER on the writer side,
because the smp_wmb is already done by arranging seqcount updates correctly.

Hmm. yes, probably. At least in the replace function. I think if we do it in other places, we should add comments as to where
the smp_wmb() is located, for future reference.

Also I saw in a couple of places where you're checking the shared pointers, you're not checking for NULL pointers, which I guess may happen if shared_count and pointers are not in full sync?

op 11-04-14 12:11, Thomas Hellstrom schreef:

Quoted 55 lines · Maarten Lankhorst

Quoted 39 lines

op 11-04-14 10:38, Thomas Hellstrom schreef:

Quoted 38 lines

Hi, Maarten.

Here I believe we encounter a lot of locking inconsistencies.

First, it seems you're use a number of pointers as RCU pointers without
annotating them as such and use the correct rcu macros when assigning those pointers.

Some pointers (like the pointers in the shared fence list) are both used
as RCU pointers (in dma_buf_poll()) for example,
or considered protected by the seqlock (reservation_object_get_fences_rcu()), which I believe is OK, but then the pointers must
be assigned using the correct rcu macros. In the memcpy in reservation_object_get_fences_rcu() we might get away with an ugly typecast, but with a verbose comment that the pointers are considered protected by the seqlock at that location.

So I've updated (attached) the headers with proper __rcu annotation and
locking comments according to how they are being used in the various reading functions.
I believe if we want to get rid of this we need to validate those pointers using the seqlock as well.
This will generate a lot of sparse warnings in those places needing rcu_dereference()
rcu_assign_pointer()
rcu_dereference_protected()

With this I think we can get rid of all ACCESS_ONCE macros: It's not needed when the rcu_x() macros are used, and
it's never needed for the members protected by the seqlock, (provided that the seq is tested). The only place where I think that's *not* the case is at the krealloc in reservation_object_get_fences_rcu().

Quoted 7 lines

Quoted 2 lines

Also I have some more comments in the reservation_object_get_fences_rcu() function below:

I felt that the barriers needed for rcu were already provided by checking the seqcount lock.
But looking at rcu_dereference makes it seem harmless to add it in more places, it handles
the ACCESS_ONCE and barrier() for us.

And it makes the code more maintainable, and helps sparse doing a lot of checking for us. I guess
we can tolerate a couple of extra barriers for that.

Quoted 4 lines

We could probably get away with using RCU_INIT_POINTER on the writer side,
because the smp_wmb is already done by arranging seqcount updates correctly.

Quoted 9 lines

Hmm. yes, probably. At least in the replace function. I think if we do it in other places, we should add comments as to where
the smp_wmb() is located, for future reference.

Also I saw in a couple of places where you're checking the shared pointers, you're not checking for NULL pointers, which I guess may happen if shared_count and pointers are not in full sync?

No, because shared_count is protected with seqcount. I only allow appending to the array, so when
shared_count is validated by seqcount it means that the [0...shared_count) indexes are valid and non-null.
What could happen though is that the fence at a specific index is updated with another one from the same
context, but that's harmless.

~Maarten

Hi!

Quoted 65 lines · Maarten Lankhorst

op 11-04-14 12:11, Thomas Hellstrom schreef:

Quoted 55 lines · Maarten Lankhorst

Quoted 39 lines

op 11-04-14 10:38, Thomas Hellstrom schreef:

Quoted 38 lines

Hi, Maarten.

Here I believe we encounter a lot of locking inconsistencies.

First, it seems you're use a number of pointers as RCU pointers without
annotating them as such and use the correct rcu macros when assigning those pointers.

Some pointers (like the pointers in the shared fence list) are both used
as RCU pointers (in dma_buf_poll()) for example,
or considered protected by the seqlock (reservation_object_get_fences_rcu()), which I believe is OK, but then the pointers must
be assigned using the correct rcu macros. In the memcpy in reservation_object_get_fences_rcu() we might get away with an ugly typecast, but with a verbose comment that the pointers are considered protected by the seqlock at that location.

So I've updated (attached) the headers with proper __rcu annotation and
locking comments according to how they are being used in the various reading functions.
I believe if we want to get rid of this we need to validate those pointers using the seqlock as well.
This will generate a lot of sparse warnings in those places needing rcu_dereference()
rcu_assign_pointer()
rcu_dereference_protected()

With this I think we can get rid of all ACCESS_ONCE macros: It's not needed when the rcu_x() macros are used, and
it's never needed for the members protected by the seqlock, (provided that the seq is tested). The only place where I think that's *not* the case is at the krealloc in reservation_object_get_fences_rcu().

Quoted 7 lines

Quoted 2 lines

Also I have some more comments in the reservation_object_get_fences_rcu() function below:

I felt that the barriers needed for rcu were already provided by checking the seqcount lock.
But looking at rcu_dereference makes it seem harmless to add it in more places, it handles
the ACCESS_ONCE and barrier() for us.

And it makes the code more maintainable, and helps sparse doing a lot of checking for us. I guess
we can tolerate a couple of extra barriers for that.

Quoted 4 lines

We could probably get away with using RCU_INIT_POINTER on the writer side,
because the smp_wmb is already done by arranging seqcount updates correctly.

Quoted 9 lines

Hmm. yes, probably. At least in the replace function. I think if we do it in other places, we should add comments as to where
the smp_wmb() is located, for future reference.

Also I saw in a couple of places where you're checking the shared pointers, you're not checking for NULL pointers, which I guess may happen if shared_count and pointers are not in full sync?

Quoted 7 lines

No, because shared_count is protected with seqcount. I only allow appending to the array, so when
shared_count is validated by seqcount it means that the [0...shared_count) indexes are valid and non-null.
What could happen though is that the fence at a specific index is updated with another one from the same
context, but that's harmless.

Hmm.
Shouldn't we have a way to clean signaled fences from reservation objects? Perhaps when we attach a new fence, or after a wait with ww_mutex held? Otherwise we'd have a lot of completely unused fence objects hanging around for no reason. I don't think we need to be as picky as TTM, but I think we should do something?

op 11-04-14 12:11, Thomas Hellstrom schreef:

Quoted 55 lines · Maarten Lankhorst

Quoted 39 lines

op 11-04-14 10:38, Thomas Hellstrom schreef:

Quoted 38 lines

Hi, Maarten.

Here I believe we encounter a lot of locking inconsistencies.

First, it seems you're use a number of pointers as RCU pointers without
annotating them as such and use the correct rcu macros when assigning those pointers.

Some pointers (like the pointers in the shared fence list) are both used
as RCU pointers (in dma_buf_poll()) for example,
or considered protected by the seqlock (reservation_object_get_fences_rcu()), which I believe is OK, but then the pointers must
be assigned using the correct rcu macros. In the memcpy in reservation_object_get_fences_rcu() we might get away with an ugly typecast, but with a verbose comment that the pointers are considered protected by the seqlock at that location.

So I've updated (attached) the headers with proper __rcu annotation and
locking comments according to how they are being used in the various reading functions.
I believe if we want to get rid of this we need to validate those pointers using the seqlock as well.
This will generate a lot of sparse warnings in those places needing rcu_dereference()
rcu_assign_pointer()
rcu_dereference_protected()

With this I think we can get rid of all ACCESS_ONCE macros: It's not needed when the rcu_x() macros are used, and
it's never needed for the members protected by the seqlock, (provided that the seq is tested). The only place where I think that's *not* the case is at the krealloc in reservation_object_get_fences_rcu().

Quoted 7 lines

Quoted 2 lines

Also I have some more comments in the reservation_object_get_fences_rcu() function below:

I felt that the barriers needed for rcu were already provided by checking the seqcount lock.
But looking at rcu_dereference makes it seem harmless to add it in more places, it handles
the ACCESS_ONCE and barrier() for us.

And it makes the code more maintainable, and helps sparse doing a lot of checking for us. I guess
we can tolerate a couple of extra barriers for that.

Quoted 4 lines

We could probably get away with using RCU_INIT_POINTER on the writer side,
because the smp_wmb is already done by arranging seqcount updates correctly.

Quoted 9 lines

Hmm. yes, probably. At least in the replace function. I think if we do it in other places, we should add comments as to where
the smp_wmb() is located, for future reference.

Also I saw in a couple of places where you're checking the shared pointers, you're not checking for NULL pointers, which I guess may happen if shared_count and pointers are not in full sync?

No, because shared_count is protected with seqcount. I only allow appending to the array, so when
shared_count is validated by seqcount it means that the [0...shared_count) indexes are valid and non-null.
What could happen though is that the fence at a specific index is updated with another one from the same
context, but that's harmless.

~Maarten
_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Quoted 65 lines

op 11-04-14 12:11, Thomas Hellstrom schreef:

Quoted 55 lines · Maarten Lankhorst

Quoted 39 lines

op 11-04-14 10:38, Thomas Hellstrom schreef:

Quoted 38 lines

Hi, Maarten.

Here I believe we encounter a lot of locking inconsistencies.

First, it seems you're use a number of pointers as RCU pointers without
annotating them as such and use the correct rcu macros when assigning those pointers.

Some pointers (like the pointers in the shared fence list) are both used
as RCU pointers (in dma_buf_poll()) for example,
or considered protected by the seqlock (reservation_object_get_fences_rcu()), which I believe is OK, but then the pointers must
be assigned using the correct rcu macros. In the memcpy in reservation_object_get_fences_rcu() we might get away with an ugly typecast, but with a verbose comment that the pointers are considered protected by the seqlock at that location.

So I've updated (attached) the headers with proper __rcu annotation and
locking comments according to how they are being used in the various reading functions.
I believe if we want to get rid of this we need to validate those pointers using the seqlock as well.
This will generate a lot of sparse warnings in those places needing rcu_dereference()
rcu_assign_pointer()
rcu_dereference_protected()

With this I think we can get rid of all ACCESS_ONCE macros: It's not needed when the rcu_x() macros are used, and
it's never needed for the members protected by the seqlock, (provided that the seq is tested). The only place where I think that's *not* the case is at the krealloc in reservation_object_get_fences_rcu().

Quoted 7 lines

Quoted 2 lines

Also I have some more comments in the reservation_object_get_fences_rcu() function below:

I felt that the barriers needed for rcu were already provided by checking the seqcount lock.
But looking at rcu_dereference makes it seem harmless to add it in more places, it handles
the ACCESS_ONCE and barrier() for us.

And it makes the code more maintainable, and helps sparse doing a lot of checking for us. I guess
we can tolerate a couple of extra barriers for that.

Quoted 4 lines

We could probably get away with using RCU_INIT_POINTER on the writer side,
because the smp_wmb is already done by arranging seqcount updates correctly.

Quoted 9 lines

Hmm. yes, probably. At least in the replace function. I think if we do it in other places, we should add comments as to where
the smp_wmb() is located, for future reference.

Also I saw in a couple of places where you're checking the shared pointers, you're not checking for NULL pointers, which I guess may happen if shared_count and pointers are not in full sync?

Quoted 8 lines

No, because shared_count is protected with seqcount. I only allow appending to the array, so when
shared_count is validated by seqcount it means that the [0...shared_count) indexes are valid and non-null.
What could happen though is that the fence at a specific index is updated with another one from the same
context, but that's harmless.

Hmm, doesn't attaching an exclusive fence clear all shared fence pointers from under a reader?

op 11-04-14 21:35, Thomas Hellstrom schreef:

Quoted 77 lines · Maarten Lankhorst

Quoted 76 lines

op 11-04-14 12:11, Thomas Hellstrom schreef:

Quoted 58 lines · Maarten Lankhorst

Quoted 41 lines

op 11-04-14 10:38, Thomas Hellstrom schreef:

Quoted 40 lines

Hi, Maarten.

Here I believe we encounter a lot of locking inconsistencies.

First, it seems you're use a number of pointers as RCU pointers without
annotating them as such and use the correct rcu macros when assigning those pointers.

Some pointers (like the pointers in the shared fence list) are both used
as RCU pointers (in dma_buf_poll()) for example,
or considered protected by the seqlock (reservation_object_get_fences_rcu()), which I believe is OK, but then
the pointers must
be assigned using the correct rcu macros. In the memcpy in reservation_object_get_fences_rcu() we might get away with an ugly typecast, but with a verbose comment that the pointers are considered protected by the seqlock at that location.

So I've updated (attached) the headers with proper __rcu annotation and
locking comments according to how they are being used in the various reading functions.
I believe if we want to get rid of this we need to validate those pointers using the seqlock as well.
This will generate a lot of sparse warnings in those places needing rcu_dereference()
rcu_assign_pointer()
rcu_dereference_protected()

With this I think we can get rid of all ACCESS_ONCE macros: It's not needed when the rcu_x() macros are used, and
it's never needed for the members protected by the seqlock, (provided
that the seq is tested). The only place where I think that's *not* the case is at the krealloc in reservation_object_get_fences_rcu().

Quoted 7 lines

Quoted 2 lines

Also I have some more comments in the reservation_object_get_fences_rcu() function below:

I felt that the barriers needed for rcu were already provided by checking the seqcount lock.
But looking at rcu_dereference makes it seem harmless to add it in more places, it handles
the ACCESS_ONCE and barrier() for us.

And it makes the code more maintainable, and helps sparse doing a lot of
checking for us. I guess
we can tolerate a couple of extra barriers for that.

Quoted 4 lines

We could probably get away with using RCU_INIT_POINTER on the writer side,
because the smp_wmb is already done by arranging seqcount updates correctly.

Quoted 9 lines

Hmm. yes, probably. At least in the replace function. I think if we do it in other places, we should add comments as to where
the smp_wmb() is located, for future reference.

Also I saw in a couple of places where you're checking the shared pointers, you're not checking for NULL pointers, which I guess may happen if shared_count and pointers are not in full sync?

No, because shared_count is protected with seqcount. I only allow appending to the array, so when
shared_count is validated by seqcount it means that the [0...shared_count) indexes are valid and non-null.
What could happen though is that the fence at a specific index is updated with another one from the same
context, but that's harmless.

Quoted 3 lines

Hmm, doesn't attaching an exclusive fence clear all shared fence pointers from under a reader?

No, for that reason. It only resets shared_count to 0.

~Maarten

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 <[email protected]>

---
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.

op 23-04-14 13:15, Maarten Lankhorst schreef:

Quoted 23 lines

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 <[email protected]>

---
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.

Quoted 3 lines

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?

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 <[email protected]>

---
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
---

drivers/base/dma-buf.c +1 -1

index d89a98d2c37b..0df673f812eb 100644
@@ -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)

drivers/base/dma-buf.c +13 -7

@@ -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;

drivers/base/dma-buf.c +7 -1

@@ -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);
             }

         }

drivers/base/dma-buf.c +14 -1

@@ -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. */

drivers/base/dma-buf.c +1 -1

@@ -220,7 +245,7 @@ static unsigned int dma_buf_poll(struct file
 *file, poll_table *poll)
     }
 
 out:
-    ww_mutex_unlock(&resv->lock);
+    rcu_read_unlock();
     return events;
 }

drivers/base/reservation.c +5 -0

index b82a5b630a8e..901bbf19c868 100644
@@ -38,6 +38,11 @@
 DEFINE_WW_CLASS(reservation_ww_class);
 EXPORT_SYMBOL(reservation_ww_class);
 
+struct lock_class_key reservation_seqcount_class;
+EXPORT_SYMBOL(reservation_seqcount_class);
+
+const char reservation_seqcount_string[] = "reservation_seqcount";
+EXPORT_SYMBOL(reservation_seqcount_string);
 /*
  * Reserve space to add a shared fence to a reservation_object,
  * must be called with obj->lock held.

drivers/base/reservation.c +19 -9

     } else
         max = 4;

@@ -82,27 +87,37 @@ reservation_object_add_shared_inplace(struct
 reservation_object *obj,
 {
     u32 i;
 
+    fence_get(fence);
+
+    preempt_disable();
+    write_seqcount_begin(&obj->seq);
+
     for (i = 0; i < fobj->shared_count; ++i) {
-        if (fobj->shared[i]->context == fence->context) {
-            struct fence *old_fence = fobj->shared[i];
+        struct fence *old_fence;
 
-            fence_get(fence);
+        old_fence = rcu_dereference_protected(fobj->shared[i],
+                        reservation_object_held(obj));
 
-            fobj->shared[i] = fence;
+        if (old_fence->context == fence->context) {
+            /* memory barrier is added by write_seqcount_begin */
+            RCU_INIT_POINTER(fobj->shared[i], fence);
+            write_seqcount_end(&obj->seq);
+            preempt_enable();
 
             fence_put(old_fence);
             return;
         }
     }
 
-    fence_get(fence);
-    fobj->shared[fobj->shared_count] = fence;
     /*
-     * make the new fence visible before incrementing
-     * fobj->shared_count
+     * memory barrier is added by write_seqcount_begin,
+     * fobj->shared_count is protected by this lock too
      */
-    smp_wmb();
+    RCU_INIT_POINTER(fobj->shared[fobj->shared_count], fence);
     fobj->shared_count++;
+
+    write_seqcount_end(&obj->seq);
+    preempt_enable();
 }
 

static void

drivers/base/reservation.c +2 -1

@@ -112,11 +127,12 @@ reservation_object_add_shared_replace(struct
 reservation_object *obj,
                       struct fence *fence)
 {
     unsigned i;
+    struct fence *old_fence = NULL;
 
     fence_get(fence);
 
     if (!old) {
-        fobj->shared[0] = fence;
+        RCU_INIT_POINTER(fobj->shared[0], fence);
         fobj->shared_count = 1;
         goto done;

     }

drivers/base/reservation.c +28 -9

@@ -130,19 +146,38 @@ reservation_object_add_shared_replace(struct
 reservation_object *obj,
     fobj->shared_count = old->shared_count;
 
     for (i = 0; i < old->shared_count; ++i) {
-        if (fence && old->shared[i]->context == fence->context) {
-            fence_put(old->shared[i]);
-            fobj->shared[i] = fence;
-            fence = NULL;
+        struct fence *check;
+
+        check = rcu_dereference_protected(old->shared[i],
+                        reservation_object_held(obj));
+
+        if (!old_fence && check->context == fence->context) {
+            old_fence = check;
+            RCU_INIT_POINTER(fobj->shared[i], fence);
         } else
-            fobj->shared[i] = old->shared[i];
+            RCU_INIT_POINTER(fobj->shared[i], check);
+    }
+    if (!old_fence) {
+        RCU_INIT_POINTER(fobj->shared[fobj->shared_count], fence);
+        fobj->shared_count++;
     }
-    if (fence)
-        fobj->shared[fobj->shared_count++] = fence;
 
 done:
-    obj->fence = fobj;
-    kfree(old);
+    preempt_disable();
+    write_seqcount_begin(&obj->seq);
+    /*
+     * RCU_INIT_POINTER can be used here,
+     * seqcount provides the necessary barriers
+     */
+    RCU_INIT_POINTER(obj->fence, fobj);
+    write_seqcount_end(&obj->seq);
+    preempt_enable();
+
+    if (old)
+        kfree_rcu(old, rcu);
+
+    if (old_fence)
+        fence_put(old_fence);
 }
 

/*

drivers/base/reservation.c +1 -1

@@ -158,7 +193,7 @@ void reservation_object_add_shared_fence(struct
 reservation_object *obj,
     obj->staged = NULL;
 
     if (!fobj) {
-        BUG_ON(old->shared_count == old->shared_max);
+        BUG_ON(old->shared_count >= old->shared_max);
         reservation_object_add_shared_inplace(obj, old, fence);
     } else

         reservation_object_add_shared_replace(obj, old, fobj, fence);

drivers/base/reservation.c +26 -6

@@ -168,26 +203,266 @@
 EXPORT_SYMBOL(reservation_object_add_shared_fence);
 void reservation_object_add_excl_fence(struct reservation_object *obj,
                        struct fence *fence)
 {
-    struct fence *old_fence = obj->fence_excl;
+    struct fence *old_fence = reservation_object_get_excl(obj);
     struct reservation_object_list *old;
     u32 i = 0;
 
     old = reservation_object_get_list(obj);
-    if (old) {
+    if (old)
         i = old->shared_count;
-        old->shared_count = 0;
-    }
 
     if (fence)
         fence_get(fence);
 
-    obj->fence_excl = fence;
+    preempt_disable();
+    write_seqcount_begin(&obj->seq);
+    /* write_seqcount_begin provides the necessary memory barrier */
+    RCU_INIT_POINTER(obj->fence_excl, fence);
+    if (old)
+        old->shared_count = 0;
+    write_seqcount_end(&obj->seq);
+    preempt_enable();
 
     /* inplace update, no shared fences */
     while (i--)
-        fence_put(old->shared[i]);
+        fence_put(rcu_dereference_protected(old->shared[i],
+                        reservation_object_held(obj)));
 
     if (old_fence)
         fence_put(old_fence);
 }
 EXPORT_SYMBOL(reservation_object_add_excl_fence);
+
+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;

+ nshared = krealloc(shared, sizeof(*shared) *
shared_count, GFP_KERNEL);

drivers/base/reservation.c +7 -0

+            if (!nshared) {
+                ret = -ENOMEM;
+                shared_count = retry = 0;
+                goto unlock;
+            }
+            shared = nshared;
+            memcpy(shared, fobj->shared, sizeof(*shared) *

shared_count);

drivers/base/reservation.c +62 -0

+        } else
+            shared_count = 0;
+        fence_excl = rcu_dereference(obj->fence_excl);
+
+        retry = read_seqcount_retry(&obj->seq, seq);
+        if (retry)
+            goto unlock;
+
+        if (!fence_excl || fence_get_rcu(fence_excl)) {
+            unsigned i;
+
+            for (i = 0; i < shared_count; ++i) {
+                if (fence_get_rcu(shared[i]))
+                    continue;
+
+                /* uh oh, refcount failed, abort and retry */
+                while (i--)
+                    fence_put(shared[i]);
+
+                if (fence_excl) {
+                    fence_put(fence_excl);
+                    fence_excl = NULL;
+                }
+
+                retry = 1;
+                break;
+            }
+        } else
+            retry = 1;
+
+unlock:
+        rcu_read_unlock();
+    }
+    *pshared_count = shared_count;
+    if (shared_count)
+        *pshared = shared;
+    else {
+        *pshared = NULL;
+        kfree(shared);
+    }
+    *pfence_excl = fence_excl;
+
+    return ret;
+}
+EXPORT_SYMBOL_GPL(reservation_object_get_fences_rcu);
+
+long reservation_object_wait_timeout_rcu(struct reservation_object *obj,
+                     bool wait_all, bool intr,
+                     unsigned long timeout)
+{
+    struct fence *fence;
+    unsigned seq, shared_count, i = 0;
+    long ret = timeout;
+
+retry:
+    fence = NULL;
+    shared_count = 0;
+    seq = read_seqcount_begin(&obj->seq);
+    rcu_read_lock();
+
+    if (wait_all) {
+        struct reservation_object_list *fobj =

rcu_dereference(obj->fence);

drivers/base/reservation.c +80 -0

+
+        if (fobj)
+            shared_count = fobj->shared_count;
+
+        if (read_seqcount_retry(&obj->seq, seq))
+            goto unlock_retry;
+
+        for (i = 0; i < shared_count; ++i) {
+            struct fence *lfence = rcu_dereference(fobj->shared[i]);
+
+            if (test_bit(FENCE_FLAG_SIGNALED_BIT, &lfence->flags))
+                continue;
+
+            if (!fence_get_rcu(lfence))
+                goto unlock_retry;
+
+            if (fence_is_signaled(lfence)) {
+                fence_put(lfence);
+                continue;
+            }
+
+            fence = lfence;
+            break;
+        }
+    }
+
+    if (!shared_count) {
+        struct fence *fence_excl = rcu_dereference(obj->fence_excl);
+
+        if (read_seqcount_retry(&obj->seq, seq))
+            goto unlock_retry;
+
+        if (fence_excl &&
+            !test_bit(FENCE_FLAG_SIGNALED_BIT, &fence_excl->flags)) {
+            if (!fence_get_rcu(fence_excl))
+                goto unlock_retry;
+
+            if (fence_is_signaled(fence_excl))
+                fence_put(fence_excl);
+            else
+                fence = fence_excl;
+        }
+    }
+
+    rcu_read_unlock();
+    if (fence) {
+        ret = fence_wait_timeout(fence, intr, ret);
+        fence_put(fence);
+        if (ret > 0 && wait_all && (i + 1 < shared_count))
+            goto retry;
+    }
+    return ret;
+
+unlock_retry:
+    rcu_read_unlock();
+    goto retry;
+}
+EXPORT_SYMBOL_GPL(reservation_object_wait_timeout_rcu);
+
+
+static inline int
+reservation_object_test_signaled_single(struct fence *passed_fence)
+{
+    struct fence *fence, *lfence = passed_fence;
+    int ret = 1;
+
+    if (!test_bit(FENCE_FLAG_SIGNALED_BIT, &lfence->flags)) {
+        int ret;
+
+        fence = fence_get_rcu(lfence);
+        if (!fence)
+            return -1;
+
+        ret = !!fence_is_signaled(fence);
+        fence_put(fence);
+    }
+    return ret;
+}
+
+bool reservation_object_test_signaled_rcu(struct reservation_object

*obj,

drivers/base/reservation.c +14 -0

+                      bool test_all)
+{
+    unsigned seq, shared_count;
+    int ret = true;
+
+retry:
+    shared_count = 0;
+    seq = read_seqcount_begin(&obj->seq);
+    rcu_read_lock();
+
+    if (test_all) {
+        unsigned i;
+
+        struct reservation_object_list *fobj =

rcu_dereference(obj->fence);

drivers/base/reservation.c +11 -0

+
+        if (fobj)
+            shared_count = fobj->shared_count;
+
+        if (read_seqcount_retry(&obj->seq, seq))
+            goto unlock_retry;
+
+        for (i = 0; i < shared_count; ++i) {
+            struct fence *fence = rcu_dereference(fobj->shared[i]);
+
+            ret = reservation_object_test_signaled_single(fence);

drivers/base/reservation.c +25 -0

+         * read_seqcount_retry. If you care, use ww_mutex_lock.
+         */
+    }
+
+    if (!shared_count) {
+        struct fence *fence_excl = rcu_dereference(obj->fence_excl);
+
+        if (read_seqcount_retry(&obj->seq, seq))
+            goto unlock_retry;
+
+        if (fence_excl) {
+            ret = reservation_object_test_signaled_single(fence_excl);
+            if (ret < 0)
+                goto unlock_retry;
+        }
+    }
+
+    rcu_read_unlock();
+    return ret;
+
+unlock_retry:
+    rcu_read_unlock();
+    goto retry;
+}
+EXPORT_SYMBOL_GPL(reservation_object_test_signaled_rcu);

include/linux/fence.h +5 -1

index d13b5ab61726..4b7002457af0 100644
@@ -31,7 +31,7 @@
 #include <linux/kref.h>
 #include <linux/sched.h>
 #include <linux/printk.h>
-#include <linux/slab.h>
+#include <linux/rcupdate.h>
 
 struct fence;
 struct fence_ops;

@@ -41,6 +41,7 @@ struct fence_cb;
  * struct fence - software synchronization primitive
  * @refcount: refcount for this fence
  * @ops: fence_ops associated with this fence
+ * @rcu: used for releasing fence with kfree_rcu
  * @cb_list: list of all callbacks to call
  * @lock: spin_lock_irqsave used for locking
  * @context: execution context this fence belongs to, returned by

@@ -74,6 +75,7 @@ struct fence_cb;
 struct fence {
     struct kref refcount;
     const struct fence_ops *ops;
+    struct rcu_head rcu;
     struct list_head cb_list;
     spinlock_t *lock;
     unsigned context, seqno;

@@ -190,11 +192,25 @@ static inline void fence_get(struct fence *fence)
     kref_get(&fence->refcount);
 }
 
+/**
+ * fence_get_rcu - get a fence from a reservation_object_list with

rcu read lock
+ * @fence: [in] fence to increase refcount of
+ *
+ * Function returns NULL if no refcount could be obtained, or the fence.
+ */

include/linux/fence.h +9 -1

+static inline struct fence *fence_get_rcu(struct fence *fence)
+{
+    if (kref_get_unless_zero(&fence->refcount))
+        return fence;
+    else
+        return NULL;
+}
+
 extern void release_fence(struct kref *kref);
 
 static inline void free_fence(struct fence *fence)
 {
-    kfree(fence);
+    kfree_rcu(fence, rcu);
 }
 
 /**

include/linux/reservation.h +20 -10

index 2affe67dea6e..b73d3df5a8e8 100644
@@ -42,22 +42,29 @@
 #include <linux/ww_mutex.h>
 #include <linux/fence.h>
 #include <linux/slab.h>
+#include <linux/seqlock.h>
+#include <linux/rcupdate.h>
 
 extern struct ww_class reservation_ww_class;
+extern struct lock_class_key reservation_seqcount_class;
+extern const char reservation_seqcount_string[];
 
 struct reservation_object_list {
+    struct rcu_head rcu;
     u32 shared_count, shared_max;
-    struct fence *shared[];
+    struct fence __rcu *shared[];
 };
 
 struct reservation_object {
     struct ww_mutex lock;
+    seqcount_t seq;
 
-    struct fence *fence_excl;
-    struct reservation_object_list *fence;
+    struct fence __rcu *fence_excl;
+    struct reservation_object_list __rcu *fence;
     struct reservation_object_list *staged;
 };
 
+#define reservation_object_held(obj) lockdep_is_held(&(obj)->lock.base)
 #define reservation_object_assert_held(obj) \
     lockdep_assert_held(&(obj)->lock.base)
 

@@ -66,8 +73,9 @@ reservation_object_init(struct reservation_object *obj)
 {
     ww_mutex_init(&obj->lock, &reservation_ww_class);
 
-    obj->fence_excl = NULL;
-    obj->fence = NULL;
+    __seqcount_init(&obj->seq, reservation_seqcount_string,
 &reservation_seqcount_class);
+    RCU_INIT_POINTER(obj->fence, NULL);
+    RCU_INIT_POINTER(obj->fence_excl, NULL);
     obj->staged = NULL;
 }

@@ -76,18 +84,20 @@ reservation_object_fini(struct reservation_object
 *obj)
 {
     int i;
     struct reservation_object_list *fobj;
+    struct fence *excl;
 
     /*
      * This object should be dead and all references must have
-     * been released to it.
+     * been released to it, so no need to be protected with rcu.
      */
-    if (obj->fence_excl)
-        fence_put(obj->fence_excl);
+    excl = rcu_dereference_protected(obj->fence_excl, 1);
+    if (excl)
+        fence_put(excl);
 
-    fobj = obj->fence;
+    fobj = rcu_dereference_protected(obj->fence, 1);
     if (fobj) {
         for (i = 0; i < fobj->shared_count; ++i)
-            fence_put(fobj->shared[i]);
+            fence_put(rcu_dereference_protected(fobj->shared[i], 1));
 
         kfree(fobj);

     }

include/linux/reservation.h +4 -6

@@ -99,17 +109,15 @@ reservation_object_fini(struct reservation_object
 *obj)
 static inline struct reservation_object_list *
 reservation_object_get_list(struct reservation_object *obj)
 {
-    reservation_object_assert_held(obj);
-
-    return obj->fence;
+    return rcu_dereference_check(obj->fence,
+                     reservation_object_held(obj));
 }
 
 static inline struct fence *
 reservation_object_get_excl(struct reservation_object *obj)
 {
-    reservation_object_assert_held(obj);
-
-    return obj->fence_excl;
+    return rcu_dereference_check(obj->fence_excl,
+                     reservation_object_held(obj));
 }
 

int reservation_object_reserve_shared(struct reservation_object *obj);

include/linux/reservation.h +10 -0

@@ -119,4 +127,16 @@ void reservation_object_add_shared_fence(struct
 reservation_object *obj,
 void reservation_object_add_excl_fence(struct reservation_object *obj,
                        struct fence *fence);
 
+int reservation_object_get_fences_rcu(struct reservation_object *obj,
+                      struct fence **pfence_excl,
+                      unsigned *pshared_count,
+                      struct fence ***pshared);
+
+long reservation_object_wait_timeout_rcu(struct reservation_object *obj,
+                     bool wait_all, bool intr,
+                     unsigned long timeout);
+
+bool reservation_object_test_signaled_rcu(struct reservation_object

*obj,

include/linux/reservation.h +2 -0

+                      bool test_all);
+
 #endif /* _LINUX_RESERVATION_H */

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:

drivers/base/reservation.c int reservation_object_reserve_shared(struct · line 65

Quoted 9 lines

drivers/base/reservation.c +2 -2

@@ -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?

Quoted 17 lines

drivers/base/reservation.c +17 -0

+
+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);
+

drivers/base/reservation.c EXPORT_SYMBOL(reservation_object_add_shared_fence); · line 258

Quoted 7 lines

drivers/base/reservation.c +7 -0

+        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?

Quoted 2 lines

+ 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....

drivers/base/reservation.c EXPORT_SYMBOL(reservation_object_add_shared_fence); · line 443

Quoted 5 lines

drivers/base/reservation.c +5 -0

+
+        /*
+         * 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.

op 19-05-14 15:42, Thomas Hellstrom schreef:

Quoted 10 lines

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:

Quoted 10 lines

drivers/base/reservation.c int reservation_object_reserve_shared(struct · line 65

Quoted 9 lines

drivers/base/reservation.c +2 -2

@@ -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.

Quoted 17 lines

Quoted 17 lines

drivers/base/reservation.c +17 -0

+
+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);
+

Quoted 8 lines

drivers/base/reservation.c EXPORT_SYMBOL(reservation_object_add_shared_fence); · line 258

Quoted 7 lines

drivers/base/reservation.c +7 -0

+        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.

Quoted 8 lines

Quoted 3 lines

+ 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?

(quoted diff) +15 -4

@@ -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);

Quoted 7 lines

drivers/base/reservation.c EXPORT_SYMBOL(reservation_object_add_shared_fence); · line 443

Quoted 6 lines

drivers/base/reservation.c +3 -0

+
+        /*
+         * There could be a read_seqcount_retry here, but nothing

cares

drivers/base/reservation.c +2 -0

+         * about whether it's the old or newer fence pointers that are
+         * signale. That race could still have happened after checking

Typo.

Oops

op 19-05-14 15:42, Thomas Hellstrom schreef:

Quoted 10 lines

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:

Quoted 10 lines

drivers/base/reservation.c int reservation_object_reserve_shared(struct · line 65

Quoted 9 lines

drivers/base/reservation.c +2 -2

@@ -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.

Quoted 17 lines

Quoted 17 lines

drivers/base/reservation.c +17 -0

+
+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);
+

Quoted 8 lines

drivers/base/reservation.c EXPORT_SYMBOL(reservation_object_add_shared_fence); · line 258

Quoted 7 lines

drivers/base/reservation.c +7 -0

+        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.

Quoted 48 lines

op 19-05-14 15:42, Thomas Hellstrom schreef:

Quoted 10 lines

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:

Quoted 10 lines

drivers/base/reservation.c int reservation_object_reserve_shared(struct · line 65

Quoted 9 lines

drivers/base/reservation.c +2 -2

@@ -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.

Quoted 25 lines

Quoted 14 lines

drivers/base/reservation.c +14 -0

+
+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;

drivers/base/reservation.c EXPORT_SYMBOL(reservation_object_add_shared_fence); · line 258

Quoted 10 lines

drivers/base/reservation.c +10 -0

+
+        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?

Quoted 5 lines

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?