Re: [Intel-gfx] [PATCH 01/26] dma-buf: add dma_resv_for_each_fence_unlocked v2
Am 20.09.21 um 10:43 schrieb Tvrtko Ursulin:
On 17/09/2021 14:23, Daniel Vetter wrote:
On Fri, Sep 17, 2021 at 02:34:48PM +0200, Christian König wrote:
Abstract the complexity of iterating over all the fences
in a dma_resv object.
The new loop handles the whole RCU and retry dance and
returns only fences where we can be sure we grabbed the
right one.
v2: fix accessing the shared fences while they might be freed,
improve kerneldoc, rename _cursor to _iter, add
dma_resv_iter_is_exclusive, add dma_resv_iter_begin/end
Signed-off-by: Christian König
---
drivers/dma-buf/dma-resv.c | 61 +++
include/linux/dma-resv.h | 84
++
2 files changed, 145 insertions(+)
diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c
index 84fbe60629e3..3e77cad2c9d4 100644
--- a/drivers/dma-buf/dma-resv.c
+++ b/drivers/dma-buf/dma-resv.c
@@ -323,6 +323,67 @@ void dma_resv_add_excl_fence(struct dma_resv
*obj, struct dma_fence *fence)
}
EXPORT_SYMBOL(dma_resv_add_excl_fence);
+/**
+ * dma_resv_iter_walk_unlocked - walk over fences in a dma_resv obj
+ * @cursor: cursor to record the current position
+ * @first: if we should start over
+ *
+ * Return all the fences in the dma_resv object which are not yet
signaled.
+ * The returned fence has an extra local reference so will stay alive.
+ * If a concurrent modify is detected the whole iterration is
started over again.
+ */
+struct dma_fence *dma_resv_iter_walk_unlocked(struct dma_resv_iter
*cursor,
Bit ocd, but I'd still just call that iter_next.
+ bool first)
Hm I'd put all the init code into iter_begin ...
@Christian:
Could you engineer something in here which would, at least in debug
builds, catch failures to call "iter begin" before using the iterator
macro?
Yeah, I've already played with the thought of somehow teaching lockdep
that. But then abandoned this as abusive of lockdep.
+{
+ struct dma_resv *obj = cursor->obj;
Aren't we missing rcu_read_lock() around the entire thing here?
+
+ first |= read_seqcount_retry(>seq, cursor->seq);
+ do {
+ /* Drop the reference from the previous round */
+ dma_fence_put(cursor->fence);
+
+ cursor->is_first = first;
+ if (first) {
+ cursor->seq = read_seqcount_begin(>seq);
+ cursor->index = -1;
+ cursor->fences = dma_resv_shared_list(obj);
And then also call iter_begin from here. That way we guarantee that
read_seqcount_begin is always called before _retry(). It's not a problem
with the seqcount implementation (I think at least), but it definitely
looks funny.
Calling iter_begin here also makes it clear that we're essentially
restarting.
+
+ cursor->fence = dma_resv_excl_fence(obj);
+ if (cursor->fence &&
+ test_bit(DMA_FENCE_FLAG_SIGNALED_BIT,
Please use the right dma_fence wrapper here for this and don't look
at the
bits/flags outside of dma_fence.[hc] code. I just realized that we don't
have the right amount of barriers in there for the fastpath, i.e. if we
have:
x = 0; /* static initializer */
thread a
x = 1;
dma_fence_signal(fence);
thread b;
if (dma_fence_is_signalled(fence))
printk("%i\n", x);
Then you might actually be able to observe x == 0 in thread b. Which is
not what we want at all.
@Daniel:
What do you mean here - in terms of if 'x' is "external" (not part of
dma-fence), then are you suggesting dma-fence code should serialise it
by using barriers?
That would sound incorrect to me, or in other words, I think it's fine
if x == 0 is observed in your example thread B since that code is
mixing external data with dma-fence.
No, Daniel is right. The problem is that on architectures other than x86
barriers are per memory address (or rather cache line in practice).
So you need to be really careful that you see the fully consistent state
and not just one variable but others in the old state.
But this was buggy before as well. I'm just pulling the existing test
into the new iterator.
Hm also, there is that annoying bit where by using
dma_fence_is_signaled any code becomes a fence signaling critical
path, which I never bought into. There should be a way to test the
signaled status without actually doing the signaling. Or I am
misunderstanding something so badly that is really really has to be
like this?
You are mixing things up. Testing is unproblematic, signaling is the
problematic one.
So no open-coding of dma_fence flag bits code outside of drm_fence.[hc]
please. And yes i915-gem code is unfortunately a disaster.
Don't even miss an opportunity for some good trashing no? :D
But yes, deconstructed dma_fence_signal I thought we were supposed to
add to core. Or at least propose, don't exactly remember how that went.
The problem is that you need to grab a reference to call
dma_fence_signal while testing the
Re: [Intel-gfx] [PATCH 01/26] dma-buf: add dma_resv_for_each_fence_unlocked v2
On 20/09/2021 11:09, Christian König wrote:
Am 20.09.21 um 10:43 schrieb Tvrtko Ursulin:
On 17/09/2021 14:23, Daniel Vetter wrote:
On Fri, Sep 17, 2021 at 02:34:48PM +0200, Christian König wrote:
Abstract the complexity of iterating over all the fences
in a dma_resv object.
The new loop handles the whole RCU and retry dance and
returns only fences where we can be sure we grabbed the
right one.
v2: fix accessing the shared fences while they might be freed,
improve kerneldoc, rename _cursor to _iter, add
dma_resv_iter_is_exclusive, add dma_resv_iter_begin/end
Signed-off-by: Christian König
---
drivers/dma-buf/dma-resv.c | 61 +++
include/linux/dma-resv.h | 84
++
2 files changed, 145 insertions(+)
diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c
index 84fbe60629e3..3e77cad2c9d4 100644
--- a/drivers/dma-buf/dma-resv.c
+++ b/drivers/dma-buf/dma-resv.c
@@ -323,6 +323,67 @@ void dma_resv_add_excl_fence(struct dma_resv
*obj, struct dma_fence *fence)
}
EXPORT_SYMBOL(dma_resv_add_excl_fence);
+/**
+ * dma_resv_iter_walk_unlocked - walk over fences in a dma_resv obj
+ * @cursor: cursor to record the current position
+ * @first: if we should start over
+ *
+ * Return all the fences in the dma_resv object which are not yet
signaled.
+ * The returned fence has an extra local reference so will stay alive.
+ * If a concurrent modify is detected the whole iterration is
started over again.
+ */
+struct dma_fence *dma_resv_iter_walk_unlocked(struct dma_resv_iter
*cursor,
Bit ocd, but I'd still just call that iter_next.
+ bool first)
Hm I'd put all the init code into iter_begin ...
@Christian:
Could you engineer something in here which would, at least in debug
builds, catch failures to call "iter begin" before using the iterator
macro?
Yeah, I've already played with the thought of somehow teaching lockdep
that. But then abandoned this as abusive of lockdep.
Yes probably not lockdep but would need to be a separate build time
option akin to DEBUG_WW_MUTEXES and similar.
+{
+ struct dma_resv *obj = cursor->obj;
Aren't we missing rcu_read_lock() around the entire thing here?
+
+ first |= read_seqcount_retry(>seq, cursor->seq);
+ do {
+ /* Drop the reference from the previous round */
+ dma_fence_put(cursor->fence);
+
+ cursor->is_first = first;
+ if (first) {
+ cursor->seq = read_seqcount_begin(>seq);
+ cursor->index = -1;
+ cursor->fences = dma_resv_shared_list(obj);
And then also call iter_begin from here. That way we guarantee that
read_seqcount_begin is always called before _retry(). It's not a problem
with the seqcount implementation (I think at least), but it definitely
looks funny.
Calling iter_begin here also makes it clear that we're essentially
restarting.
+
+ cursor->fence = dma_resv_excl_fence(obj);
+ if (cursor->fence &&
+ test_bit(DMA_FENCE_FLAG_SIGNALED_BIT,
Please use the right dma_fence wrapper here for this and don't look
at the
bits/flags outside of dma_fence.[hc] code. I just realized that we don't
have the right amount of barriers in there for the fastpath, i.e. if we
have:
x = 0; /* static initializer */
thread a
x = 1;
dma_fence_signal(fence);
thread b;
if (dma_fence_is_signalled(fence))
printk("%i\n", x);
Then you might actually be able to observe x == 0 in thread b. Which is
not what we want at all.
@Daniel:
What do you mean here - in terms of if 'x' is "external" (not part of
dma-fence), then are you suggesting dma-fence code should serialise it
by using barriers?
That would sound incorrect to me, or in other words, I think it's fine
if x == 0 is observed in your example thread B since that code is
mixing external data with dma-fence.
No, Daniel is right. The problem is that on architectures other than x86
barriers are per memory address (or rather cache line in practice).
So you need to be really careful that you see the fully consistent state
and not just one variable but others in the old state.
I don't see it yet - what are the variables we are talking about here?
Ordering relating to the iterator code in here or something truly external?
Iterator can obviously race and "return" and already signaled fence
(transitioned from unsignaled to signaled between iterator checking and
deciding to walk it). But that I don't think you can, or plan to, fix.
But this was buggy before as well. I'm just pulling the existing test
into the new iterator.
Okay.
Hm also, there is that annoying bit where by using
dma_fence_is_signaled any code becomes a fence signaling critical
path, which I never bought into. There should be a way to test the
signaled status without actually doing the signaling. Or I am
misunderstanding something so badly that is really really has to be
Re: [Intel-gfx] [PATCH 01/26] dma-buf: add dma_resv_for_each_fence_unlocked v2
On 17/09/2021 14:23, Daniel Vetter wrote:
On Fri, Sep 17, 2021 at 02:34:48PM +0200, Christian König wrote:
Abstract the complexity of iterating over all the fences
in a dma_resv object.
The new loop handles the whole RCU and retry dance and
returns only fences where we can be sure we grabbed the
right one.
v2: fix accessing the shared fences while they might be freed,
improve kerneldoc, rename _cursor to _iter, add
dma_resv_iter_is_exclusive, add dma_resv_iter_begin/end
Signed-off-by: Christian König
---
drivers/dma-buf/dma-resv.c | 61 +++
include/linux/dma-resv.h | 84 ++
2 files changed, 145 insertions(+)
diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c
index 84fbe60629e3..3e77cad2c9d4 100644
--- a/drivers/dma-buf/dma-resv.c
+++ b/drivers/dma-buf/dma-resv.c
@@ -323,6 +323,67 @@ void dma_resv_add_excl_fence(struct dma_resv *obj, struct
dma_fence *fence)
}
EXPORT_SYMBOL(dma_resv_add_excl_fence);
+/**
+ * dma_resv_iter_walk_unlocked - walk over fences in a dma_resv obj
+ * @cursor: cursor to record the current position
+ * @first: if we should start over
+ *
+ * Return all the fences in the dma_resv object which are not yet signaled.
+ * The returned fence has an extra local reference so will stay alive.
+ * If a concurrent modify is detected the whole iterration is started over
again.
+ */
+struct dma_fence *dma_resv_iter_walk_unlocked(struct dma_resv_iter *cursor,
Bit ocd, but I'd still just call that iter_next.
+ bool first)
Hm I'd put all the init code into iter_begin ...
@Christian:
Could you engineer something in here which would, at least in debug
builds, catch failures to call "iter begin" before using the iterator macro?
+{
+ struct dma_resv *obj = cursor->obj;
Aren't we missing rcu_read_lock() around the entire thing here?
+
+ first |= read_seqcount_retry(>seq, cursor->seq);
+ do {
+ /* Drop the reference from the previous round */
+ dma_fence_put(cursor->fence);
+
+ cursor->is_first = first;
+ if (first) {
+ cursor->seq = read_seqcount_begin(>seq);
+ cursor->index = -1;
+ cursor->fences = dma_resv_shared_list(obj);
And then also call iter_begin from here. That way we guarantee that
read_seqcount_begin is always called before _retry(). It's not a problem
with the seqcount implementation (I think at least), but it definitely
looks funny.
Calling iter_begin here also makes it clear that we're essentially
restarting.
+
+ cursor->fence = dma_resv_excl_fence(obj);
+ if (cursor->fence &&
+ test_bit(DMA_FENCE_FLAG_SIGNALED_BIT,
Please use the right dma_fence wrapper here for this and don't look at the
bits/flags outside of dma_fence.[hc] code. I just realized that we don't
have the right amount of barriers in there for the fastpath, i.e. if we
have:
x = 0; /* static initializer */
thread a
x = 1;
dma_fence_signal(fence);
thread b;
if (dma_fence_is_signalled(fence))
printk("%i\n", x);
Then you might actually be able to observe x == 0 in thread b. Which is
not what we want at all.
@Daniel:
What do you mean here - in terms of if 'x' is "external" (not part of
dma-fence), then are you suggesting dma-fence code should serialise it
by using barriers?
That would sound incorrect to me, or in other words, I think it's fine
if x == 0 is observed in your example thread B since that code is mixing
external data with dma-fence.
Hm also, there is that annoying bit where by using dma_fence_is_signaled
any code becomes a fence signaling critical path, which I never bought
into. There should be a way to test the signaled status without actually
doing the signaling. Or I am misunderstanding something so badly that is
really really has to be like this?
So no open-coding of dma_fence flag bits code outside of drm_fence.[hc]
please. And yes i915-gem code is unfortunately a disaster.
Don't even miss an opportunity for some good trashing no? :D
But yes, deconstructed dma_fence_signal I thought we were supposed to
add to core. Or at least propose, don't exactly remember how that went.
+>fence->flags))
+ cursor->fence = NULL;
+ } else {
+ cursor->fence = NULL;
+ }
+
+ if (cursor->fence) {
+ cursor->fence = dma_fence_get_rcu(cursor->fence);
+ } else if (cursor->all_fences && cursor->fences) {
+ struct dma_resv_list *fences = cursor->fences;
+
+ while (++cursor->index < fences->shared_count) {
+ cursor->fence = rcu_dereference(
+
Re: [Intel-gfx] [PATCH 01/26] dma-buf: add dma_resv_for_each_fence_unlocked v2
On Fri, Sep 17, 2021 at 02:34:48PM +0200, Christian König wrote:
> Abstract the complexity of iterating over all the fences
> in a dma_resv object.
>
> The new loop handles the whole RCU and retry dance and
> returns only fences where we can be sure we grabbed the
> right one.
>
> v2: fix accessing the shared fences while they might be freed,
> improve kerneldoc, rename _cursor to _iter, add
> dma_resv_iter_is_exclusive, add dma_resv_iter_begin/end
>
> Signed-off-by: Christian König
> ---
> drivers/dma-buf/dma-resv.c | 61 +++
> include/linux/dma-resv.h | 84 ++
> 2 files changed, 145 insertions(+)
>
> diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c
> index 84fbe60629e3..3e77cad2c9d4 100644
> --- a/drivers/dma-buf/dma-resv.c
> +++ b/drivers/dma-buf/dma-resv.c
> @@ -323,6 +323,67 @@ void dma_resv_add_excl_fence(struct dma_resv *obj,
> struct dma_fence *fence)
> }
> EXPORT_SYMBOL(dma_resv_add_excl_fence);
>
> +/**
> + * dma_resv_iter_walk_unlocked - walk over fences in a dma_resv obj
> + * @cursor: cursor to record the current position
> + * @first: if we should start over
> + *
> + * Return all the fences in the dma_resv object which are not yet signaled.
> + * The returned fence has an extra local reference so will stay alive.
> + * If a concurrent modify is detected the whole iterration is started over
> again.
> + */
> +struct dma_fence *dma_resv_iter_walk_unlocked(struct dma_resv_iter *cursor,
Bit ocd, but I'd still just call that iter_next.
> + bool first)
Hm I'd put all the init code into iter_begin ...
> +{
> + struct dma_resv *obj = cursor->obj;
Aren't we missing rcu_read_lock() around the entire thing here?
> +
> + first |= read_seqcount_retry(>seq, cursor->seq);
> + do {
> + /* Drop the reference from the previous round */
> + dma_fence_put(cursor->fence);
> +
> + cursor->is_first = first;
> + if (first) {
> + cursor->seq = read_seqcount_begin(>seq);
> + cursor->index = -1;
> + cursor->fences = dma_resv_shared_list(obj);
And then also call iter_begin from here. That way we guarantee that
read_seqcount_begin is always called before _retry(). It's not a problem
with the seqcount implementation (I think at least), but it definitely
looks funny.
Calling iter_begin here also makes it clear that we're essentially
restarting.
> +
> + cursor->fence = dma_resv_excl_fence(obj);
> + if (cursor->fence &&
> + test_bit(DMA_FENCE_FLAG_SIGNALED_BIT,
Please use the right dma_fence wrapper here for this and don't look at the
bits/flags outside of dma_fence.[hc] code. I just realized that we don't
have the right amount of barriers in there for the fastpath, i.e. if we
have:
x = 0; /* static initializer */
thread a
x = 1;
dma_fence_signal(fence);
thread b;
if (dma_fence_is_signalled(fence))
printk("%i\n", x);
Then you might actually be able to observe x == 0 in thread b. Which is
not what we want at all.
So no open-coding of dma_fence flag bits code outside of drm_fence.[hc]
please. And yes i915-gem code is unfortunately a disaster.
> + >fence->flags))
> + cursor->fence = NULL;
> + } else {
> + cursor->fence = NULL;
> + }
> +
> + if (cursor->fence) {
> + cursor->fence = dma_fence_get_rcu(cursor->fence);
> + } else if (cursor->all_fences && cursor->fences) {
> + struct dma_resv_list *fences = cursor->fences;
> +
> + while (++cursor->index < fences->shared_count) {
> + cursor->fence = rcu_dereference(
> + fences->shared[cursor->index]);
> + if (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT,
> + >fence->flags))
> + break;
> + }
> + if (cursor->index < fences->shared_count)
> + cursor->fence =
> + dma_fence_get_rcu(cursor->fence);
> + else
> + cursor->fence = NULL;
> + }
The control flow here is very hairy, but I'm not sure how to best do this.
With my suggestion to move the read_seqcount_begin into iter_begin maybe
something like this:
iter_next()
{
do {
dma_fence_put(cursor->fence)
cursor->fence = NULL;
if (cursor->index == -1) { /* reset by iter_begin()
cursor->fence = get_exclusive();
cusor->index++;
} else {
[Intel-gfx] [PATCH 01/26] dma-buf: add dma_resv_for_each_fence_unlocked v2
Abstract the complexity of iterating over all the fences
in a dma_resv object.
The new loop handles the whole RCU and retry dance and
returns only fences where we can be sure we grabbed the
right one.
v2: fix accessing the shared fences while they might be freed,
improve kerneldoc, rename _cursor to _iter, add
dma_resv_iter_is_exclusive, add dma_resv_iter_begin/end
Signed-off-by: Christian König
---
drivers/dma-buf/dma-resv.c | 61 +++
include/linux/dma-resv.h | 84 ++
2 files changed, 145 insertions(+)
diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c
index 84fbe60629e3..3e77cad2c9d4 100644
--- a/drivers/dma-buf/dma-resv.c
+++ b/drivers/dma-buf/dma-resv.c
@@ -323,6 +323,67 @@ void dma_resv_add_excl_fence(struct dma_resv *obj, struct
dma_fence *fence)
}
EXPORT_SYMBOL(dma_resv_add_excl_fence);
+/**
+ * dma_resv_iter_walk_unlocked - walk over fences in a dma_resv obj
+ * @cursor: cursor to record the current position
+ * @first: if we should start over
+ *
+ * Return all the fences in the dma_resv object which are not yet signaled.
+ * The returned fence has an extra local reference so will stay alive.
+ * If a concurrent modify is detected the whole iterration is started over
again.
+ */
+struct dma_fence *dma_resv_iter_walk_unlocked(struct dma_resv_iter *cursor,
+ bool first)
+{
+ struct dma_resv *obj = cursor->obj;
+
+ first |= read_seqcount_retry(>seq, cursor->seq);
+ do {
+ /* Drop the reference from the previous round */
+ dma_fence_put(cursor->fence);
+
+ cursor->is_first = first;
+ if (first) {
+ cursor->seq = read_seqcount_begin(>seq);
+ cursor->index = -1;
+ cursor->fences = dma_resv_shared_list(obj);
+
+ cursor->fence = dma_resv_excl_fence(obj);
+ if (cursor->fence &&
+ test_bit(DMA_FENCE_FLAG_SIGNALED_BIT,
+>fence->flags))
+ cursor->fence = NULL;
+ } else {
+ cursor->fence = NULL;
+ }
+
+ if (cursor->fence) {
+ cursor->fence = dma_fence_get_rcu(cursor->fence);
+ } else if (cursor->all_fences && cursor->fences) {
+ struct dma_resv_list *fences = cursor->fences;
+
+ while (++cursor->index < fences->shared_count) {
+ cursor->fence = rcu_dereference(
+ fences->shared[cursor->index]);
+ if (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT,
+ >fence->flags))
+ break;
+ }
+ if (cursor->index < fences->shared_count)
+ cursor->fence =
+ dma_fence_get_rcu(cursor->fence);
+ else
+ cursor->fence = NULL;
+ }
+
+ /* For the eventually next round */
+ first = true;
+ } while (read_seqcount_retry(>seq, cursor->seq));
+
+ return cursor->fence;
+}
+EXPORT_SYMBOL_GPL(dma_resv_iter_walk_unlocked);
+
/**
* dma_resv_copy_fences - Copy all fences from src to dst.
* @dst: the destination reservation object
diff --git a/include/linux/dma-resv.h b/include/linux/dma-resv.h
index 9100dd3dc21f..693d16117153 100644
--- a/include/linux/dma-resv.h
+++ b/include/linux/dma-resv.h
@@ -149,6 +149,90 @@ struct dma_resv {
struct dma_resv_list __rcu *fence;
};
+/**
+ * struct dma_resv_iter - current position into the dma_resv fences
+ *
+ * Don't touch this directly in the driver, use the accessor function instead.
+ */
+struct dma_resv_iter {
+ /** @obj: The dma_resv object we iterate over */
+ struct dma_resv *obj;
+
+ /** @all_fences: If all fences should be returned */
+ bool all_fences;
+
+ /** @fence: the currently handled fence */
+ struct dma_fence *fence;
+
+ /** @seq: sequence number to check for modifications */
+ unsigned int seq;
+
+ /** @index: index into the shared fences */
+ unsigned int index;
+
+ /** @fences: the shared fences */
+ struct dma_resv_list *fences;
+
+ /** @is_first: true if this is the first returned fence */
+ bool is_first;
+};
+
+struct dma_fence *dma_resv_iter_walk_unlocked(struct dma_resv_iter *cursor,
+ bool first);
+
+/**
+ * dma_resv_iter_begin - initialize a dma_resv_iter object
+ * @cursor: The dma_resv_iter object to initialize
+ * @obj: The dma_resv object which we want to iterator over
+ * @all_fences: If all
Re: [Intel-gfx] [PATCH 01/26] dma-buf: add dma_resv_for_each_fence_unlocked v2
On Thu, Sep 16, 2021 at 01:30:17PM +0200, Christian König wrote:
> Abstract the complexity of iterating over all the fences
> in a dma_resv object.
>
> The new loop handles the whole RCU and retry dance and
> returns only fences where we can be sure we grabbed the
> right one.
>
> v2: fix accessing the shared fences while they might be freed,
> improve kerneldoc, rename _cursor to _iter, add
> dma_resv_iter_is_exclusive
>
> Signed-off-by: Christian König
Replied in the other thread with the fully typed out example, this really
needs iter_init/next/end here. Or it's just way too fragile and tricky for
a generic helper that we roll out everywhere.
-Daniel
> ---
> drivers/dma-buf/dma-resv.c | 62 ++
> include/linux/dma-resv.h | 50 ++
> 2 files changed, 112 insertions(+)
>
> diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c
> index 84fbe60629e3..77083170ec3b 100644
> --- a/drivers/dma-buf/dma-resv.c
> +++ b/drivers/dma-buf/dma-resv.c
> @@ -323,6 +323,68 @@ void dma_resv_add_excl_fence(struct dma_resv *obj,
> struct dma_fence *fence)
> }
> EXPORT_SYMBOL(dma_resv_add_excl_fence);
>
> +/**
> + * dma_resv_walk_unlocked - walk over fences in a dma_resv obj
> + * @obj: the dma_resv object
> + * @cursor: cursor to record the current position
> + * @all_fences: true returns also the shared fences
> + * @first: if we should start over
> + *
> + * Return all the fences in the dma_resv object which are not yet signaled.
> + * The returned fence has an extra local reference so will stay alive.
> + * If a concurrent modify is detected the whole iterator is started over
> again.
> + */
> +struct dma_fence *dma_resv_walk_unlocked(struct dma_resv *obj,
> + struct dma_resv_iter *cursor,
> + bool all_fences, bool first)
> +{
> + struct dma_fence *fence = NULL;
> +
> + first |= read_seqcount_retry(>seq, cursor->seq);
> + do {
> + /* Drop the reference from the previous round */
> + dma_fence_put(fence);
> +
> + cursor->is_first = first;
> + if (first) {
> + cursor->seq = read_seqcount_begin(>seq);
> + cursor->index = -1;
> + cursor->fences = dma_resv_shared_list(obj);
> +
> + fence = dma_resv_excl_fence(obj);
> + if (fence && test_bit(DMA_FENCE_FLAG_SIGNALED_BIT,
> + >flags))
> + fence = NULL;
> + } else {
> + fence = NULL;
> + }
> +
> + if (fence) {
> + fence = dma_fence_get_rcu(fence);
> + } else if (all_fences && cursor->fences) {
> + struct dma_resv_list *fences = cursor->fences;
> +
> + while (++cursor->index < fences->shared_count) {
> + fence = rcu_dereference(
> + fences->shared[cursor->index]);
> + if (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT,
> + >flags))
> + break;
> + }
> + if (cursor->index < fences->shared_count)
> + fence = dma_fence_get_rcu(fence);
> + else
> + fence = NULL;
> + }
> +
> + /* For the eventually next round */
> + first = true;
> + } while (read_seqcount_retry(>seq, cursor->seq));
> +
> + return fence;
> +}
> +EXPORT_SYMBOL_GPL(dma_resv_walk_unlocked);
> +
> /**
> * dma_resv_copy_fences - Copy all fences from src to dst.
> * @dst: the destination reservation object
> diff --git a/include/linux/dma-resv.h b/include/linux/dma-resv.h
> index 9100dd3dc21f..1cd686384c71 100644
> --- a/include/linux/dma-resv.h
> +++ b/include/linux/dma-resv.h
> @@ -149,6 +149,53 @@ struct dma_resv {
> struct dma_resv_list __rcu *fence;
> };
>
> +/**
> + * struct dma_resv_iter - current position into the dma_resv fences
> + *
> + * Don't touch this directly in the driver, use the accessor function
> instead.
> + */
> +struct dma_resv_iter {
> + /** @seq: sequence number to check for modifications */
> + unsigned int seq;
> +
> + /** @index: index into the shared fences */
> + unsigned int index;
> +
> + /** @fences: the shared fences */
> + struct dma_resv_list *fences;
> +
> + /** @is_first: true if this is the first returned fence */
> + bool is_first;
> +};
> +
> +/**
> + * dma_resv_for_each_fence_unlocked - fence iterator
> + * @obj: a dma_resv object pointer
> + * @cursor: a struct dma_resv_iter pointer
> + * @all_fences: true if all fences should be returned
> + * @fence: the current fence
> + *
> + * Iterate over the
[Intel-gfx] [PATCH 01/26] dma-buf: add dma_resv_for_each_fence_unlocked v2
Abstract the complexity of iterating over all the fences
in a dma_resv object.
The new loop handles the whole RCU and retry dance and
returns only fences where we can be sure we grabbed the
right one.
v2: fix accessing the shared fences while they might be freed,
improve kerneldoc, rename _cursor to _iter, add
dma_resv_iter_is_exclusive
Signed-off-by: Christian König
---
drivers/dma-buf/dma-resv.c | 62 ++
include/linux/dma-resv.h | 50 ++
2 files changed, 112 insertions(+)
diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c
index 84fbe60629e3..77083170ec3b 100644
--- a/drivers/dma-buf/dma-resv.c
+++ b/drivers/dma-buf/dma-resv.c
@@ -323,6 +323,68 @@ void dma_resv_add_excl_fence(struct dma_resv *obj, struct
dma_fence *fence)
}
EXPORT_SYMBOL(dma_resv_add_excl_fence);
+/**
+ * dma_resv_walk_unlocked - walk over fences in a dma_resv obj
+ * @obj: the dma_resv object
+ * @cursor: cursor to record the current position
+ * @all_fences: true returns also the shared fences
+ * @first: if we should start over
+ *
+ * Return all the fences in the dma_resv object which are not yet signaled.
+ * The returned fence has an extra local reference so will stay alive.
+ * If a concurrent modify is detected the whole iterator is started over again.
+ */
+struct dma_fence *dma_resv_walk_unlocked(struct dma_resv *obj,
+struct dma_resv_iter *cursor,
+bool all_fences, bool first)
+{
+ struct dma_fence *fence = NULL;
+
+ first |= read_seqcount_retry(>seq, cursor->seq);
+ do {
+ /* Drop the reference from the previous round */
+ dma_fence_put(fence);
+
+ cursor->is_first = first;
+ if (first) {
+ cursor->seq = read_seqcount_begin(>seq);
+ cursor->index = -1;
+ cursor->fences = dma_resv_shared_list(obj);
+
+ fence = dma_resv_excl_fence(obj);
+ if (fence && test_bit(DMA_FENCE_FLAG_SIGNALED_BIT,
+ >flags))
+ fence = NULL;
+ } else {
+ fence = NULL;
+ }
+
+ if (fence) {
+ fence = dma_fence_get_rcu(fence);
+ } else if (all_fences && cursor->fences) {
+ struct dma_resv_list *fences = cursor->fences;
+
+ while (++cursor->index < fences->shared_count) {
+ fence = rcu_dereference(
+ fences->shared[cursor->index]);
+ if (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT,
+ >flags))
+ break;
+ }
+ if (cursor->index < fences->shared_count)
+ fence = dma_fence_get_rcu(fence);
+ else
+ fence = NULL;
+ }
+
+ /* For the eventually next round */
+ first = true;
+ } while (read_seqcount_retry(>seq, cursor->seq));
+
+ return fence;
+}
+EXPORT_SYMBOL_GPL(dma_resv_walk_unlocked);
+
/**
* dma_resv_copy_fences - Copy all fences from src to dst.
* @dst: the destination reservation object
diff --git a/include/linux/dma-resv.h b/include/linux/dma-resv.h
index 9100dd3dc21f..1cd686384c71 100644
--- a/include/linux/dma-resv.h
+++ b/include/linux/dma-resv.h
@@ -149,6 +149,53 @@ struct dma_resv {
struct dma_resv_list __rcu *fence;
};
+/**
+ * struct dma_resv_iter - current position into the dma_resv fences
+ *
+ * Don't touch this directly in the driver, use the accessor function instead.
+ */
+struct dma_resv_iter {
+ /** @seq: sequence number to check for modifications */
+ unsigned int seq;
+
+ /** @index: index into the shared fences */
+ unsigned int index;
+
+ /** @fences: the shared fences */
+ struct dma_resv_list *fences;
+
+ /** @is_first: true if this is the first returned fence */
+ bool is_first;
+};
+
+/**
+ * dma_resv_for_each_fence_unlocked - fence iterator
+ * @obj: a dma_resv object pointer
+ * @cursor: a struct dma_resv_iter pointer
+ * @all_fences: true if all fences should be returned
+ * @fence: the current fence
+ *
+ * Iterate over the fences in a struct dma_resv object without holding the
+ * dma_resv::lock. The RCU read side lock must be hold when using this, but can
+ * be dropped and re-taken as necessary inside the loop. @all_fences controls
+ * if the shared fences are returned as well.
+ */
+#define dma_resv_for_each_fence_unlocked(obj, cursor, all_fences, fence)\
+ for (fence = dma_resv_walk_unlocked(obj, cursor, all_fences, true); \
+
