This is the new dma_fence_array based container for shared fences in the dma_resv object.
Advantage of this approach is that you can grab a reference to the current set of shared fences at any time, which allows us to drop the sequence number increment and makes the whole RCU handling much more easier.
Disadvantage is that RCU users now have to grab a reference instead of using the sequence counter. As far as I can see i915 was actually the only driver doing this.
So we optimize for adding more fences instead of reading them now.
Another behavior change worth noting is that the shared fences are now only visible after unlocking the dma_resv object or calling dma_resv_fences_commit() manually.
Please review and/or comment,
Christian.
Hi everyone,
In previous discussion it surfaced that different drivers use the shared and explicit fences in the dma_resv object with different meanings.
This is problematic when we share buffers between those drivers and requirements for implicit and explicit synchronization leaded to quite a number of workarounds related to this.
So I started an effort to get all drivers back to a common understanding of what the fences in the dma_resv object mean and be able to use the object for different kind of workloads independent of the classic DRM command submission interface.
The result is this patch set which modifies the dma_resv API to get away from a single explicit fence and multiple shared fences, towards a notation where we have explicit categories for writers, readers and others.
To do this I came up with a new container called dma_resv_fences which can store both a single fence as well as multiple fences in a dma_fence_array.
This turned out to actually be even be quite a bit simpler, since we don't need any complicated dance between RCU and sequence count protected updates any more.
Instead we can just grab a reference to the dma_fence_array under RCU and so keep the current state of synchronization alive until we are done with it.
This results in both a small performance improvement since we don't need so many barriers any more, as well as fewer lines of code in the actual implementation.
Please review and/or comment,
Christian.
We clear the callback list on kref_put so that by the time we
release the fence it is unused. No one should be adding to the cb_list
that they don't themselves hold a reference for.
This small change is actually making the structure 16% smaller.
v2: add the comment to the code as well.
Signed-off-by: Christian König <christian.koenig(a)amd.com>
Reviewed-by: Chris Wilson <chris(a)chris-wilson.co.uk>
---
include/linux/dma-fence.h | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
index 05d29dbc7e62..bea1d05cf51e 100644
--- a/include/linux/dma-fence.h
+++ b/include/linux/dma-fence.h
@@ -65,8 +65,14 @@ struct dma_fence_cb;
struct dma_fence {
struct kref refcount;
const struct dma_fence_ops *ops;
- struct rcu_head rcu;
- struct list_head cb_list;
+ /* We clear the callback list on kref_put so that by the time we
+ * release the fence it is unused. No one should be adding to the cb_list
+ * that they don't themselves hold a reference for.
+ */
+ union {
+ struct rcu_head rcu;
+ struct list_head cb_list;
+ };
spinlock_t *lock;
u64 context;
u64 seqno;
--
2.17.1
We clear the callback list on kref_put so that by the time we
release the fence it is unused. No one should be adding to the cb_list
that they don't themselves hold a reference for.
This small change is actually making the structure 16% smaller.
Signed-off-by: Christian König <christian.koenig(a)amd.com>
Reviewed-by: Chris Wilson <chris(a)chris-wilson.co.uk>
---
include/linux/dma-fence.h | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
index 05d29dbc7e62..3985c72cd0c2 100644
--- a/include/linux/dma-fence.h
+++ b/include/linux/dma-fence.h
@@ -65,8 +65,10 @@ struct dma_fence_cb;
struct dma_fence {
struct kref refcount;
const struct dma_fence_ops *ops;
- struct rcu_head rcu;
- struct list_head cb_list;
+ union {
+ struct rcu_head rcu;
+ struct list_head cb_list;
+ };
spinlock_t *lock;
u64 context;
u64 seqno;
--
2.17.1
When reservation_object_add_shared_fence is replacing an old fence with a new
one we should not drop the old one before the new one is in place.
Otherwise other cores can busy wait for the new one to appear.
Signed-off-by: Christian König <christian.koenig(a)amd.com>
---
drivers/dma-buf/reservation.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/dma-buf/reservation.c b/drivers/dma-buf/reservation.c
index c71b85c8c159..d59207ca72d2 100644
--- a/drivers/dma-buf/reservation.c
+++ b/drivers/dma-buf/reservation.c
@@ -196,6 +196,7 @@ void reservation_object_add_shared_fence(struct reservation_object *obj,
struct dma_fence *fence)
{
struct reservation_object_list *fobj;
+ struct dma_fence *old;
unsigned int i, count;
dma_fence_get(fence);
@@ -209,18 +210,16 @@ void reservation_object_add_shared_fence(struct reservation_object *obj,
write_seqcount_begin(&obj->seq);
for (i = 0; i < count; ++i) {
- struct dma_fence *old_fence;
- old_fence = rcu_dereference_protected(fobj->shared[i],
- reservation_object_held(obj));
- if (old_fence->context == fence->context ||
- dma_fence_is_signaled(old_fence)) {
- dma_fence_put(old_fence);
+ old = rcu_dereference_protected(fobj->shared[i],
+ reservation_object_held(obj));
+ if (old->context == fence->context ||
+ dma_fence_is_signaled(old))
goto replace;
- }
}
BUG_ON(fobj->shared_count >= fobj->shared_max);
+ old = NULL;
count++;
replace:
@@ -230,6 +229,7 @@ void reservation_object_add_shared_fence(struct reservation_object *obj,
write_seqcount_end(&obj->seq);
preempt_enable();
+ dma_fence_put(old);
}
EXPORT_SYMBOL(reservation_object_add_shared_fence);
--
2.17.1
The reservation object should be capable of handling its internal memory
management itself. And since we search for a free slot to add the fence
from the beginning this is actually a waste of time and only minimal helpful.
Drop it to allow removal of the seqno handling in the reservation object.
This essentially reverts commit "drm/i915: Remove completed fences after a wait".
Signed-off-by: Christian König <christian.koenig(a)amd.com>
---
drivers/gpu/drm/i915/gem/i915_gem_wait.c | 27 ------------------------
1 file changed, 27 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_wait.c b/drivers/gpu/drm/i915/gem/i915_gem_wait.c
index 26ec6579b7cd..bb64ec6bef8e 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_wait.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_wait.c
@@ -35,9 +35,7 @@ i915_gem_object_wait_reservation(struct reservation_object *resv,
unsigned int flags,
long timeout)
{
- unsigned int seq = __read_seqcount_begin(&resv->seq);
struct dma_fence *excl;
- bool prune_fences = false;
if (flags & I915_WAIT_ALL) {
struct dma_fence **shared;
@@ -61,17 +59,6 @@ i915_gem_object_wait_reservation(struct reservation_object *resv,
for (; i < count; i++)
dma_fence_put(shared[i]);
kfree(shared);
-
- /*
- * If both shared fences and an exclusive fence exist,
- * then by construction the shared fences must be later
- * than the exclusive fence. If we successfully wait for
- * all the shared fences, we know that the exclusive fence
- * must all be signaled. If all the shared fences are
- * signaled, we can prune the array and recover the
- * floating references on the fences/requests.
- */
- prune_fences = count && timeout >= 0;
} else {
excl = reservation_object_get_excl_rcu(resv);
}
@@ -80,20 +67,6 @@ i915_gem_object_wait_reservation(struct reservation_object *resv,
timeout = i915_gem_object_wait_fence(excl, flags, timeout);
dma_fence_put(excl);
-
- /*
- * Opportunistically prune the fences iff we know they have *all* been
- * signaled and that the reservation object has not been changed (i.e.
- * no new fences have been added).
- */
- if (prune_fences && !__read_seqcount_retry(&resv->seq, seq)) {
- if (reservation_object_trylock(resv)) {
- if (!__read_seqcount_retry(&resv->seq, seq))
- reservation_object_add_excl_fence(resv, NULL);
- reservation_object_unlock(resv);
- }
- }
-
return timeout;
}
--
2.17.1
This is a note to let you know that I've just added the patch titled
dma-buf: balance refcount inbalance
to the 5.2-stable tree which can be found at:
http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=sum…
The filename of the patch is:
dma-buf-balance-refcount-inbalance.patch
and it can be found in the queue-5.2 subdirectory.
If you, or anyone else, feels it should not be added to the stable tree,
please let <stable(a)vger.kernel.org> know about it.
>From 5e383a9798990c69fc759a4930de224bb497e62c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Glisse?= <jglisse(a)redhat.com>
Date: Thu, 6 Dec 2018 11:18:40 -0500
Subject: dma-buf: balance refcount inbalance
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
From: Jérôme Glisse <jglisse(a)redhat.com>
commit 5e383a9798990c69fc759a4930de224bb497e62c upstream.
The debugfs take reference on fence without dropping them.
Signed-off-by: Jérôme Glisse <jglisse(a)redhat.com>
Cc: Christian König <christian.koenig(a)amd.com>
Cc: Daniel Vetter <daniel.vetter(a)ffwll.ch>
Cc: Sumit Semwal <sumit.semwal(a)linaro.org>
Cc: linux-media(a)vger.kernel.org
Cc: dri-devel(a)lists.freedesktop.org
Cc: linaro-mm-sig(a)lists.linaro.org
Cc: Stéphane Marchesin <marcheu(a)chromium.org>
Cc: stable(a)vger.kernel.org
Reviewed-by: Christian König <christian.koenig(a)amd.com>
Signed-off-by: Sumit Semwal <sumit.semwal(a)linaro.org>
Link: https://patchwork.freedesktop.org/patch/msgid/20181206161840.6578-1-jglisse…
Signed-off-by: Greg Kroah-Hartman <gregkh(a)linuxfoundation.org>
---
drivers/dma-buf/dma-buf.c | 1 +
1 file changed, 1 insertion(+)
--- a/drivers/dma-buf/dma-buf.c
+++ b/drivers/dma-buf/dma-buf.c
@@ -1057,6 +1057,7 @@ static int dma_buf_debug_show(struct seq
fence->ops->get_driver_name(fence),
fence->ops->get_timeline_name(fence),
dma_fence_is_signaled(fence) ? "" : "un");
+ dma_fence_put(fence);
}
rcu_read_unlock();
Patches currently in stable-queue which might be from jglisse(a)redhat.com are
queue-5.2/dma-buf-balance-refcount-inbalance.patch
This is a note to let you know that I've just added the patch titled
dma-buf: balance refcount inbalance
to the 5.1-stable tree which can be found at:
http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=sum…
The filename of the patch is:
dma-buf-balance-refcount-inbalance.patch
and it can be found in the queue-5.1 subdirectory.
If you, or anyone else, feels it should not be added to the stable tree,
please let <stable(a)vger.kernel.org> know about it.
>From 5e383a9798990c69fc759a4930de224bb497e62c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Glisse?= <jglisse(a)redhat.com>
Date: Thu, 6 Dec 2018 11:18:40 -0500
Subject: dma-buf: balance refcount inbalance
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
From: Jérôme Glisse <jglisse(a)redhat.com>
commit 5e383a9798990c69fc759a4930de224bb497e62c upstream.
The debugfs take reference on fence without dropping them.
Signed-off-by: Jérôme Glisse <jglisse(a)redhat.com>
Cc: Christian König <christian.koenig(a)amd.com>
Cc: Daniel Vetter <daniel.vetter(a)ffwll.ch>
Cc: Sumit Semwal <sumit.semwal(a)linaro.org>
Cc: linux-media(a)vger.kernel.org
Cc: dri-devel(a)lists.freedesktop.org
Cc: linaro-mm-sig(a)lists.linaro.org
Cc: Stéphane Marchesin <marcheu(a)chromium.org>
Cc: stable(a)vger.kernel.org
Reviewed-by: Christian König <christian.koenig(a)amd.com>
Signed-off-by: Sumit Semwal <sumit.semwal(a)linaro.org>
Link: https://patchwork.freedesktop.org/patch/msgid/20181206161840.6578-1-jglisse…
Signed-off-by: Greg Kroah-Hartman <gregkh(a)linuxfoundation.org>
---
drivers/dma-buf/dma-buf.c | 1 +
1 file changed, 1 insertion(+)
--- a/drivers/dma-buf/dma-buf.c
+++ b/drivers/dma-buf/dma-buf.c
@@ -1068,6 +1068,7 @@ static int dma_buf_debug_show(struct seq
fence->ops->get_driver_name(fence),
fence->ops->get_timeline_name(fence),
dma_fence_is_signaled(fence) ? "" : "un");
+ dma_fence_put(fence);
}
rcu_read_unlock();
Patches currently in stable-queue which might be from jglisse(a)redhat.com are
queue-5.1/dma-buf-balance-refcount-inbalance.patch
This is a note to let you know that I've just added the patch titled
dma-buf: balance refcount inbalance
to the 4.19-stable tree which can be found at:
http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=sum…
The filename of the patch is:
dma-buf-balance-refcount-inbalance.patch
and it can be found in the queue-4.19 subdirectory.
If you, or anyone else, feels it should not be added to the stable tree,
please let <stable(a)vger.kernel.org> know about it.
>From 5e383a9798990c69fc759a4930de224bb497e62c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Glisse?= <jglisse(a)redhat.com>
Date: Thu, 6 Dec 2018 11:18:40 -0500
Subject: dma-buf: balance refcount inbalance
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
From: Jérôme Glisse <jglisse(a)redhat.com>
commit 5e383a9798990c69fc759a4930de224bb497e62c upstream.
The debugfs take reference on fence without dropping them.
Signed-off-by: Jérôme Glisse <jglisse(a)redhat.com>
Cc: Christian König <christian.koenig(a)amd.com>
Cc: Daniel Vetter <daniel.vetter(a)ffwll.ch>
Cc: Sumit Semwal <sumit.semwal(a)linaro.org>
Cc: linux-media(a)vger.kernel.org
Cc: dri-devel(a)lists.freedesktop.org
Cc: linaro-mm-sig(a)lists.linaro.org
Cc: Stéphane Marchesin <marcheu(a)chromium.org>
Cc: stable(a)vger.kernel.org
Reviewed-by: Christian König <christian.koenig(a)amd.com>
Signed-off-by: Sumit Semwal <sumit.semwal(a)linaro.org>
Link: https://patchwork.freedesktop.org/patch/msgid/20181206161840.6578-1-jglisse…
Signed-off-by: Greg Kroah-Hartman <gregkh(a)linuxfoundation.org>
---
drivers/dma-buf/dma-buf.c | 1 +
1 file changed, 1 insertion(+)
--- a/drivers/dma-buf/dma-buf.c
+++ b/drivers/dma-buf/dma-buf.c
@@ -1069,6 +1069,7 @@ static int dma_buf_debug_show(struct seq
fence->ops->get_driver_name(fence),
fence->ops->get_timeline_name(fence),
dma_fence_is_signaled(fence) ? "" : "un");
+ dma_fence_put(fence);
}
rcu_read_unlock();
Patches currently in stable-queue which might be from jglisse(a)redhat.com are
queue-4.19/dma-buf-balance-refcount-inbalance.patch
This is a note to let you know that I've just added the patch titled
dma-buf: balance refcount inbalance
to the 4.14-stable tree which can be found at:
http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=sum…
The filename of the patch is:
dma-buf-balance-refcount-inbalance.patch
and it can be found in the queue-4.14 subdirectory.
If you, or anyone else, feels it should not be added to the stable tree,
please let <stable(a)vger.kernel.org> know about it.
>From 5e383a9798990c69fc759a4930de224bb497e62c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Glisse?= <jglisse(a)redhat.com>
Date: Thu, 6 Dec 2018 11:18:40 -0500
Subject: dma-buf: balance refcount inbalance
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
From: Jérôme Glisse <jglisse(a)redhat.com>
commit 5e383a9798990c69fc759a4930de224bb497e62c upstream.
The debugfs take reference on fence without dropping them.
Signed-off-by: Jérôme Glisse <jglisse(a)redhat.com>
Cc: Christian König <christian.koenig(a)amd.com>
Cc: Daniel Vetter <daniel.vetter(a)ffwll.ch>
Cc: Sumit Semwal <sumit.semwal(a)linaro.org>
Cc: linux-media(a)vger.kernel.org
Cc: dri-devel(a)lists.freedesktop.org
Cc: linaro-mm-sig(a)lists.linaro.org
Cc: Stéphane Marchesin <marcheu(a)chromium.org>
Cc: stable(a)vger.kernel.org
Reviewed-by: Christian König <christian.koenig(a)amd.com>
Signed-off-by: Sumit Semwal <sumit.semwal(a)linaro.org>
Link: https://patchwork.freedesktop.org/patch/msgid/20181206161840.6578-1-jglisse…
Signed-off-by: Greg Kroah-Hartman <gregkh(a)linuxfoundation.org>
---
drivers/dma-buf/dma-buf.c | 1 +
1 file changed, 1 insertion(+)
--- a/drivers/dma-buf/dma-buf.c
+++ b/drivers/dma-buf/dma-buf.c
@@ -1115,6 +1115,7 @@ static int dma_buf_debug_show(struct seq
fence->ops->get_driver_name(fence),
fence->ops->get_timeline_name(fence),
dma_fence_is_signaled(fence) ? "" : "un");
+ dma_fence_put(fence);
}
rcu_read_unlock();
Patches currently in stable-queue which might be from jglisse(a)redhat.com are
queue-4.14/dma-buf-balance-refcount-inbalance.patch
queue-4.14/libnvdimm-pfn-fix-fsdax-mode-namespace-info-block-zero-fields.patch
On 7/17/19 12:31 PM, Alexander Popov wrote:
> Hello!
>
> The syzkaller [1] has a trouble with fuzzing the Linux kernel with ION Memory
> Allocator.
>
> Syzkaller uses several methods [2] to limit memory consumption of the userspace
> processes calling the syscalls for testing the kernel:
> - setrlimit(),
> - cgroups,
> - various sysctl.
> But these methods don't work for ION Memory Allocator, so any userspace process
> that has access to /dev/ion can bring the system to the out-of-memory state.
>
> An example of a program doing that:
>
>
> #include <sys/types.h>
> #include <sys/stat.h>
> #include <fcntl.h>
> #include <stdio.h>
> #include <linux/types.h>
> #include <sys/ioctl.h>
>
> #define ION_IOC_MAGIC 'I'
> #define ION_IOC_ALLOC _IOWR(ION_IOC_MAGIC, 0, \
> struct ion_allocation_data)
>
> struct ion_allocation_data {
> __u64 len;
> __u32 heap_id_mask;
> __u32 flags;
> __u32 fd;
> __u32 unused;
> };
>
> int main(void)
> {
> unsigned long i = 0;
> int fd = -1;
> struct ion_allocation_data data = {
> .len = 0x13f65d8c,
> .heap_id_mask = 1,
> .flags = 0,
> .fd = -1,
> .unused = 0
> };
>
> fd = open("/dev/ion", 0);
> if (fd == -1) {
> perror("[-] open /dev/ion");
> return 1;
> }
>
> while (1) {
> printf("iter %lu\n", i);
> ioctl(fd, ION_IOC_ALLOC, &data);
> i++;
> }
>
> return 0;
> }
>
>
> I looked through the code of ion_alloc() and didn't find any limit checks.
> Is it currently possible to limit ION kernel allocations for some process?
>
> If not, is it a right idea to do that?
> Thanks!
>
Yes, I do think that's the right approach. We're working on moving Ion
out of staging and this is something I mentioned to John Stultz. I don't
think we've thought too hard about how to do the actual limiting so
suggestions are welcome.
Thanks,
Laura
> Best regards,
> Alexander
>
>
> [1]: https://github.com/google/syzkaller
> [2]: https://github.com/google/syzkaller/blob/master/executor/common_linux.h
>