Re: [PATCH 2/2] [RFC v2 with seqcount] reservation: add suppport for read-only access using rcu
op 11-04-14 21:30, Thomas Hellstrom schreef: Hi! On 04/11/2014 08:09 PM, Maarten Lankhorst wrote: op 11-04-14 12:11, Thomas Hellstrom schreef: On 04/11/2014 11:24 AM, Maarten Lankhorst wrote: op 11-04-14 10:38, Thomas Hellstrom schreef: 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: 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. 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? 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? Calling reservation_object_add_excl_fence with a NULL fence works, I do this in ttm_bo_wait(). It requires ww_mutex. ~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: [PATCH 2/2] [RFC v2 with seqcount] reservation: add suppport for read-only access using rcu
op 11-04-14 21:35, Thomas Hellstrom schreef: On 04/11/2014 08:09 PM, Maarten Lankhorst wrote: op 11-04-14 12:11, Thomas Hellstrom schreef: On 04/11/2014 11:24 AM, Maarten Lankhorst wrote: op 11-04-14 10:38, Thomas Hellstrom schreef: 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: 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. 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? 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? No, for that reason. It only resets shared_count to 0. This is harmless because the shared fence pointers are still valid long enough because of RCU delayed deletion. fence_get_rcu will fail when the refcount has dropped to zero. This is enough of a check to prevent errors, so there's no need to explicitly clear the fence pointers. ~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: [PATCH 2/2] [RFC v2 with seqcount] reservation: add suppport for read-only access using rcu
On 04/14/2014 09:42 AM, Maarten Lankhorst wrote: op 11-04-14 21:35, Thomas Hellstrom schreef: On 04/11/2014 08:09 PM, Maarten Lankhorst wrote: op 11-04-14 12:11, Thomas Hellstrom schreef: On 04/11/2014 11:24 AM, Maarten Lankhorst wrote: op 11-04-14 10:38, Thomas Hellstrom schreef: 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: 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. 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? 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? No, for that reason. It only resets shared_count to 0. Ah. OK. I guess I didn't read the code carefully enough. Thanks, Thomas ~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: [PATCH 2/2] [RFC v2 with seqcount] reservation: add suppport for read-only access using rcu
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:
On 04/10/2014 05:00 PM, Maarten Lankhorst wrote:
op 10-04-14 13:08, Thomas Hellstrom schreef:
On 04/10/2014 12:07 PM, Maarten Lankhorst wrote:
Hey,
op 10-04-14 10:46, Thomas Hellstrom schreef:
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.
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.
You're probably right, but it makes waiting a lot easier if I don't
have to deal with memory allocations. :P
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.
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.
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
Re: [PATCH 2/2] [RFC v2 with seqcount] reservation: add suppport for read-only access using rcu
op 11-04-14 10:38, Thomas Hellstrom schreef:
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:
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.
diff --git a/drivers/base/dma-buf.c b/drivers/base/dma-buf.c
index d89a98d2c37b..ca6ef0c4b358 100644
--- a/drivers/base/dma-buf.c
+++ b/drivers/base/dma-buf.c
+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?
Oops.
+
+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);
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.
Thanks,
Thomas
~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: [PATCH 2/2] [RFC v2 with seqcount] reservation: add suppport for read-only access using rcu
On 04/11/2014 11:24 AM, Maarten Lankhorst wrote:
op 11-04-14 10:38, Thomas Hellstrom schreef:
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:
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.
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?
Thanks,
/Thomas
diff --git a/drivers/base/dma-buf.c b/drivers/base/dma-buf.c
index d89a98d2c37b..ca6ef0c4b358 100644
--- a/drivers/base/dma-buf.c
+++ b/drivers/base/dma-buf.c
+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?
Oops.
+
+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);
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.
Thanks,
Thomas
~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: [PATCH 2/2] [RFC v2 with seqcount] reservation: add suppport for read-only access using rcu
op 11-04-14 12:11, Thomas Hellstrom schreef: On 04/11/2014 11:24 AM, Maarten Lankhorst wrote: op 11-04-14 10:38, Thomas Hellstrom schreef: 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: 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. 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? 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 -- 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: [PATCH 2/2] [RFC v2 with seqcount] reservation: add suppport for read-only access using rcu
Hi! On 04/11/2014 08:09 PM, Maarten Lankhorst wrote: op 11-04-14 12:11, Thomas Hellstrom schreef: On 04/11/2014 11:24 AM, Maarten Lankhorst wrote: op 11-04-14 10:38, Thomas Hellstrom schreef: 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: 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. 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? 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? /Thomas ~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: [PATCH 2/2] [RFC v2 with seqcount] reservation: add suppport for read-only access using rcu
On 04/11/2014 08:09 PM, Maarten Lankhorst wrote: op 11-04-14 12:11, Thomas Hellstrom schreef: On 04/11/2014 11:24 AM, Maarten Lankhorst wrote: op 11-04-14 10:38, Thomas Hellstrom schreef: 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: 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. 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? 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? /Thomas ~Maarten ___ dri-devel mailing list dri-de...@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel -- 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
