| Message ID | 20200714200646.14041-3-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] dma-buf/sw_sync: Avoid recursive lock during fence signal. | expand |
Awsome, thanks for adding the tests! Got to say I'm not that familiar with the self-test framework idioms, but from my perspective patches 2 and 3 are Reviewed-by: Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl> as well. On Tue, Jul 14, 2020 at 10:06 PM Chris Wilson <chris@chris-wilson.co.uk> wrote: > > While sw_sync is purely a debug facility for userspace to create fences > and timelines it can control, nevertheless it has some tricky locking > semantics of its own. In particular, Bas Nieuwenhuize reported that we > had reintroduced a deadlock if a signal callback attempted to destroy > the fence. So let's add a few trivial selftests to make sure that once > fixed again, it stays fixed. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl> > --- > drivers/dma-buf/Makefile | 3 +- > drivers/dma-buf/selftests.h | 1 + > drivers/dma-buf/st-sw_sync.c | 279 +++++++++++++++++++++++++++++++++++ > drivers/dma-buf/sw_sync.c | 39 +++++ > drivers/dma-buf/sync_debug.h | 8 + > 5 files changed, 329 insertions(+), 1 deletion(-) > create mode 100644 drivers/dma-buf/st-sw_sync.c > > diff --git a/drivers/dma-buf/Makefile b/drivers/dma-buf/Makefile > index 995e05f609ff..9be4d4611609 100644 > --- a/drivers/dma-buf/Makefile > +++ b/drivers/dma-buf/Makefile > @@ -10,6 +10,7 @@ obj-$(CONFIG_UDMABUF) += udmabuf.o > dmabuf_selftests-y := \ > selftest.o \ > st-dma-fence.o \ > - st-dma-fence-chain.o > + st-dma-fence-chain.o \ > + st-sw_sync.o > > obj-$(CONFIG_DMABUF_SELFTESTS) += dmabuf_selftests.o > diff --git a/drivers/dma-buf/selftests.h b/drivers/dma-buf/selftests.h > index bc8cea67bf1e..232499a24872 100644 > --- a/drivers/dma-buf/selftests.h > +++ b/drivers/dma-buf/selftests.h > @@ -12,3 +12,4 @@ > selftest(sanitycheck, __sanitycheck__) /* keep first (igt selfcheck) */ > selftest(dma_fence, dma_fence) > selftest(dma_fence_chain, dma_fence_chain) > +selftest(sw_sync, sw_sync) > diff --git a/drivers/dma-buf/st-sw_sync.c b/drivers/dma-buf/st-sw_sync.c > new file mode 100644 > index 000000000000..3a21e7717df7 > --- /dev/null > +++ b/drivers/dma-buf/st-sw_sync.c > @@ -0,0 +1,279 @@ > +/* SPDX-License-Identifier: MIT */ > +/* > + * Copyright © 2020 Intel Corporation > + */ > + > +#include <linux/delay.h> > +#include <linux/dma-fence.h> > +#include <linux/kernel.h> > +#include <linux/kthread.h> > +#include <linux/sched/signal.h> > +#include <linux/slab.h> > +#include <linux/spinlock.h> > + > +#include "sync_debug.h" > +#include "selftest.h" > + > +static int sanitycheck(void *arg) > +{ > + struct sync_timeline *tl; > + struct dma_fence *f; > + int err = -ENOMEM; > + > + /* Quick check we can create the timeline and syncpt */ > + > + tl = st_sync_timeline_create("mock"); > + if (!tl) > + return -ENOMEM; > + > + f = st_sync_pt_create(tl, 1); > + if (!f) > + goto out; > + > + dma_fence_signal(f); > + dma_fence_put(f); > + > + err = 0; > +out: > + st_sync_timeline_put(tl); > + return 0; > +} > + > +static int signal(void *arg) > +{ > + struct sync_timeline *tl; > + struct dma_fence *f; > + int err = -EINVAL; > + > + /* Check that the syncpt fence is signaled when the timeline advances */ > + > + tl = st_sync_timeline_create("mock"); > + if (!tl) > + return -ENOMEM; > + > + f = st_sync_pt_create(tl, 1); > + if (!f) { > + err = -ENOMEM; > + goto out; > + } > + > + if (dma_fence_is_signaled(f)) { > + pr_err("syncpt:%lld signaled too early\n", f->seqno); > + goto out_fence; > + } > + > + st_sync_timeline_signal(tl, 1); > + > + if (!dma_fence_is_signaled(f)) { > + pr_err("syncpt:%lld not signaled after increment\n", f->seqno); > + goto out_fence; > + } > + > + err = 0; > +out_fence: > + dma_fence_signal(f); > + dma_fence_put(f); > +out: > + st_sync_timeline_put(tl); > + return 0; > +} > + > +struct cb_destroy { > + struct dma_fence_cb cb; > + struct dma_fence *f; > +}; > + > +static void cb_destroy(struct dma_fence *fence, struct dma_fence_cb *_cb) > +{ > + struct cb_destroy *cb = container_of(_cb, typeof(*cb), cb); > + > + pr_info("syncpt:%llx destroying syncpt:%llx\n", > + fence->seqno, cb->f->seqno); > + dma_fence_put(cb->f); > + cb->f = NULL; > +} > + > +static int cb_autodestroy(void *arg) > +{ > + struct sync_timeline *tl; > + struct cb_destroy cb; > + int err = -EINVAL; > + > + /* Check that we can drop the final syncpt reference from a callback */ > + > + tl = st_sync_timeline_create("mock"); > + if (!tl) > + return -ENOMEM; > + > + cb.f = st_sync_pt_create(tl, 1); > + if (!cb.f) { > + err = -ENOMEM; > + goto out; > + } > + > + if (dma_fence_add_callback(cb.f, &cb.cb, cb_destroy)) { > + pr_err("syncpt:%lld signaled before increment\n", cb.f->seqno); > + goto out; > + } > + > + st_sync_timeline_signal(tl, 1); > + if (cb.f) { > + pr_err("syncpt:%lld callback not run\n", cb.f->seqno); > + dma_fence_put(cb.f); > + goto out; > + } > + > + err = 0; > +out: > + st_sync_timeline_put(tl); > + return 0; > +} > + > +static int cb_destroy_12(void *arg) > +{ > + struct sync_timeline *tl; > + struct cb_destroy cb; > + struct dma_fence *f; > + int err = -EINVAL; > + > + /* Check that we can drop some other syncpt reference from a callback */ > + > + tl = st_sync_timeline_create("mock"); > + if (!tl) > + return -ENOMEM; > + > + f = st_sync_pt_create(tl, 1); > + if (!f) { > + err = -ENOMEM; > + goto out; > + } > + > + cb.f = st_sync_pt_create(tl, 2); > + if (!f) { > + err = -ENOMEM; > + goto out; > + } > + > + if (dma_fence_add_callback(f, &cb.cb, cb_destroy)) { > + pr_err("syncpt:%lld signaled before increment\n", f->seqno); > + goto out; > + } > + > + st_sync_timeline_signal(tl, 1); > + if (cb.f) { > + pr_err("syncpt:%lld callback not run\n", f->seqno); > + dma_fence_put(cb.f); > + goto out_fence; > + } > + > + err = 0; > +out_fence: > + dma_fence_put(f); > +out: > + st_sync_timeline_put(tl); > + return 0; > +} > + > +static int cb_destroy_21(void *arg) > +{ > + struct sync_timeline *tl; > + struct cb_destroy cb; > + struct dma_fence *f; > + int err = -EINVAL; > + > + /* Check that we can drop an earlier syncpt reference from a callback */ > + > + tl = st_sync_timeline_create("mock"); > + if (!tl) > + return -ENOMEM; > + > + cb.f = st_sync_pt_create(tl, 1); > + if (!f) { > + err = -ENOMEM; > + goto out; > + } > + > + f = st_sync_pt_create(tl, 2); > + if (!f) { > + err = -ENOMEM; > + goto out; > + } > + > + if (dma_fence_add_callback(f, &cb.cb, cb_destroy)) { > + pr_err("syncpt:%lld signaled before increment\n", f->seqno); > + goto out; > + } > + > + st_sync_timeline_signal(tl, 2); > + if (cb.f) { > + pr_err("syncpt:%lld callback not run\n", f->seqno); > + dma_fence_put(cb.f); > + goto out_fence; > + } > + > + err = 0; > +out_fence: > + dma_fence_put(f); > +out: > + st_sync_timeline_put(tl); > + return 0; > +} > + > +static int cb_destroy_22(void *arg) > +{ > + struct sync_timeline *tl; > + struct cb_destroy cb; > + struct dma_fence *f; > + int err = -EINVAL; > + > + /* Check that we can drop the later syncpt reference from a callback */ > + > + tl = st_sync_timeline_create("mock"); > + if (!tl) > + return -ENOMEM; > + > + f = st_sync_pt_create(tl, 1); > + if (!f) { > + err = -ENOMEM; > + goto out; > + } > + > + cb.f = st_sync_pt_create(tl, 2); > + if (!f) { > + err = -ENOMEM; > + goto out; > + } > + > + if (dma_fence_add_callback(f, &cb.cb, cb_destroy)) { > + pr_err("syncpt:%lld signaled before increment\n", f->seqno); > + goto out; > + } > + > + st_sync_timeline_signal(tl, 2); > + if (cb.f) { > + pr_err("syncpt:%lld callback not run\n", f->seqno); > + dma_fence_put(cb.f); > + goto out_fence; > + } > + > + err = 0; > +out_fence: > + dma_fence_put(f); > +out: > + st_sync_timeline_put(tl); > + return 0; > +} > + > +int sw_sync(void) > +{ > + static const struct subtest tests[] = { > + SUBTEST(sanitycheck), > + SUBTEST(signal), > + SUBTEST(cb_autodestroy), > + SUBTEST(cb_destroy_12), > + SUBTEST(cb_destroy_21), > + SUBTEST(cb_destroy_22), > + }; > + > + return subtests(tests, NULL); > +} > diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c > index 116dad6c7905..7c229f4a6b56 100644 > --- a/drivers/dma-buf/sw_sync.c > +++ b/drivers/dma-buf/sw_sync.c > @@ -426,3 +426,42 @@ const struct file_operations sw_sync_debugfs_fops = { > .unlocked_ioctl = sw_sync_ioctl, > .compat_ioctl = compat_ptr_ioctl, > }; > + > +#if IS_ENABLED(CONFIG_DMABUF_SELFTESTS) > +struct sync_timeline *st_sync_timeline_create(const char *name) > +{ > + return sync_timeline_create(name); > +} > +EXPORT_SYMBOL_GPL(st_sync_timeline_create); > + > +void st_sync_timeline_get(struct sync_timeline *tl) > +{ > + sync_timeline_get(tl); > +} > +EXPORT_SYMBOL_GPL(st_sync_timeline_get); > + > +void st_sync_timeline_put(struct sync_timeline *tl) > +{ > + sync_timeline_put(tl); > +} > +EXPORT_SYMBOL_GPL(st_sync_timeline_put); > + > +void st_sync_timeline_signal(struct sync_timeline *tl, unsigned int inc) > +{ > + sync_timeline_signal(tl, inc); > +} > +EXPORT_SYMBOL_GPL(st_sync_timeline_signal); > + > +struct dma_fence * > +st_sync_pt_create(struct sync_timeline *tl, unsigned int seqno) > +{ > + struct sync_pt *pt; > + > + pt = sync_pt_create(tl, seqno); > + if (!pt) > + return NULL; > + > + return &pt->base; > +} > +EXPORT_SYMBOL_GPL(st_sync_pt_create); > +#endif > diff --git a/drivers/dma-buf/sync_debug.h b/drivers/dma-buf/sync_debug.h > index fd073fc32329..7c96d9a2c02d 100644 > --- a/drivers/dma-buf/sync_debug.h > +++ b/drivers/dma-buf/sync_debug.h > @@ -71,4 +71,12 @@ void sync_timeline_debug_remove(struct sync_timeline *obj); > void sync_file_debug_add(struct sync_file *fence); > void sync_file_debug_remove(struct sync_file *fence); > > +struct sync_timeline *st_sync_timeline_create(const char *name); > +void st_sync_timeline_get(struct sync_timeline *tl); > +void st_sync_timeline_put(struct sync_timeline *tl); > +void st_sync_timeline_signal(struct sync_timeline *tl, unsigned int inc); > + > +struct dma_fence * > +st_sync_pt_create(struct sync_timeline *tl, unsigned int seqno); > + > #endif /* _LINUX_SYNC_H */ > -- > 2.20.1 >
diff --git a/drivers/dma-buf/Makefile b/drivers/dma-buf/Makefile index 995e05f609ff..9be4d4611609 100644 --- a/drivers/dma-buf/Makefile +++ b/drivers/dma-buf/Makefile @@ -10,6 +10,7 @@ obj-$(CONFIG_UDMABUF) += udmabuf.o dmabuf_selftests-y := \ selftest.o \ st-dma-fence.o \ - st-dma-fence-chain.o + st-dma-fence-chain.o \ + st-sw_sync.o obj-$(CONFIG_DMABUF_SELFTESTS) += dmabuf_selftests.o diff --git a/drivers/dma-buf/selftests.h b/drivers/dma-buf/selftests.h index bc8cea67bf1e..232499a24872 100644 --- a/drivers/dma-buf/selftests.h +++ b/drivers/dma-buf/selftests.h @@ -12,3 +12,4 @@ selftest(sanitycheck, __sanitycheck__) /* keep first (igt selfcheck) */ selftest(dma_fence, dma_fence) selftest(dma_fence_chain, dma_fence_chain) +selftest(sw_sync, sw_sync) diff --git a/drivers/dma-buf/st-sw_sync.c b/drivers/dma-buf/st-sw_sync.c new file mode 100644 index 000000000000..3a21e7717df7 --- /dev/null +++ b/drivers/dma-buf/st-sw_sync.c @@ -0,0 +1,279 @@ +/* SPDX-License-Identifier: MIT */ +/* + * Copyright © 2020 Intel Corporation + */ + +#include <linux/delay.h> +#include <linux/dma-fence.h> +#include <linux/kernel.h> +#include <linux/kthread.h> +#include <linux/sched/signal.h> +#include <linux/slab.h> +#include <linux/spinlock.h> + +#include "sync_debug.h" +#include "selftest.h" + +static int sanitycheck(void *arg) +{ + struct sync_timeline *tl; + struct dma_fence *f; + int err = -ENOMEM; + + /* Quick check we can create the timeline and syncpt */ + + tl = st_sync_timeline_create("mock"); + if (!tl) + return -ENOMEM; + + f = st_sync_pt_create(tl, 1); + if (!f) + goto out; + + dma_fence_signal(f); + dma_fence_put(f); + + err = 0; +out: + st_sync_timeline_put(tl); + return 0; +} + +static int signal(void *arg) +{ + struct sync_timeline *tl; + struct dma_fence *f; + int err = -EINVAL; + + /* Check that the syncpt fence is signaled when the timeline advances */ + + tl = st_sync_timeline_create("mock"); + if (!tl) + return -ENOMEM; + + f = st_sync_pt_create(tl, 1); + if (!f) { + err = -ENOMEM; + goto out; + } + + if (dma_fence_is_signaled(f)) { + pr_err("syncpt:%lld signaled too early\n", f->seqno); + goto out_fence; + } + + st_sync_timeline_signal(tl, 1); + + if (!dma_fence_is_signaled(f)) { + pr_err("syncpt:%lld not signaled after increment\n", f->seqno); + goto out_fence; + } + + err = 0; +out_fence: + dma_fence_signal(f); + dma_fence_put(f); +out: + st_sync_timeline_put(tl); + return 0; +} + +struct cb_destroy { + struct dma_fence_cb cb; + struct dma_fence *f; +}; + +static void cb_destroy(struct dma_fence *fence, struct dma_fence_cb *_cb) +{ + struct cb_destroy *cb = container_of(_cb, typeof(*cb), cb); + + pr_info("syncpt:%llx destroying syncpt:%llx\n", + fence->seqno, cb->f->seqno); + dma_fence_put(cb->f); + cb->f = NULL; +} + +static int cb_autodestroy(void *arg) +{ + struct sync_timeline *tl; + struct cb_destroy cb; + int err = -EINVAL; + + /* Check that we can drop the final syncpt reference from a callback */ + + tl = st_sync_timeline_create("mock"); + if (!tl) + return -ENOMEM; + + cb.f = st_sync_pt_create(tl, 1); + if (!cb.f) { + err = -ENOMEM; + goto out; + } + + if (dma_fence_add_callback(cb.f, &cb.cb, cb_destroy)) { + pr_err("syncpt:%lld signaled before increment\n", cb.f->seqno); + goto out; + } + + st_sync_timeline_signal(tl, 1); + if (cb.f) { + pr_err("syncpt:%lld callback not run\n", cb.f->seqno); + dma_fence_put(cb.f); + goto out; + } + + err = 0; +out: + st_sync_timeline_put(tl); + return 0; +} + +static int cb_destroy_12(void *arg) +{ + struct sync_timeline *tl; + struct cb_destroy cb; + struct dma_fence *f; + int err = -EINVAL; + + /* Check that we can drop some other syncpt reference from a callback */ + + tl = st_sync_timeline_create("mock"); + if (!tl) + return -ENOMEM; + + f = st_sync_pt_create(tl, 1); + if (!f) { + err = -ENOMEM; + goto out; + } + + cb.f = st_sync_pt_create(tl, 2); + if (!f) { + err = -ENOMEM; + goto out; + } + + if (dma_fence_add_callback(f, &cb.cb, cb_destroy)) { + pr_err("syncpt:%lld signaled before increment\n", f->seqno); + goto out; + } + + st_sync_timeline_signal(tl, 1); + if (cb.f) { + pr_err("syncpt:%lld callback not run\n", f->seqno); + dma_fence_put(cb.f); + goto out_fence; + } + + err = 0; +out_fence: + dma_fence_put(f); +out: + st_sync_timeline_put(tl); + return 0; +} + +static int cb_destroy_21(void *arg) +{ + struct sync_timeline *tl; + struct cb_destroy cb; + struct dma_fence *f; + int err = -EINVAL; + + /* Check that we can drop an earlier syncpt reference from a callback */ + + tl = st_sync_timeline_create("mock"); + if (!tl) + return -ENOMEM; + + cb.f = st_sync_pt_create(tl, 1); + if (!f) { + err = -ENOMEM; + goto out; + } + + f = st_sync_pt_create(tl, 2); + if (!f) { + err = -ENOMEM; + goto out; + } + + if (dma_fence_add_callback(f, &cb.cb, cb_destroy)) { + pr_err("syncpt:%lld signaled before increment\n", f->seqno); + goto out; + } + + st_sync_timeline_signal(tl, 2); + if (cb.f) { + pr_err("syncpt:%lld callback not run\n", f->seqno); + dma_fence_put(cb.f); + goto out_fence; + } + + err = 0; +out_fence: + dma_fence_put(f); +out: + st_sync_timeline_put(tl); + return 0; +} + +static int cb_destroy_22(void *arg) +{ + struct sync_timeline *tl; + struct cb_destroy cb; + struct dma_fence *f; + int err = -EINVAL; + + /* Check that we can drop the later syncpt reference from a callback */ + + tl = st_sync_timeline_create("mock"); + if (!tl) + return -ENOMEM; + + f = st_sync_pt_create(tl, 1); + if (!f) { + err = -ENOMEM; + goto out; + } + + cb.f = st_sync_pt_create(tl, 2); + if (!f) { + err = -ENOMEM; + goto out; + } + + if (dma_fence_add_callback(f, &cb.cb, cb_destroy)) { + pr_err("syncpt:%lld signaled before increment\n", f->seqno); + goto out; + } + + st_sync_timeline_signal(tl, 2); + if (cb.f) { + pr_err("syncpt:%lld callback not run\n", f->seqno); + dma_fence_put(cb.f); + goto out_fence; + } + + err = 0; +out_fence: + dma_fence_put(f); +out: + st_sync_timeline_put(tl); + return 0; +} + +int sw_sync(void) +{ + static const struct subtest tests[] = { + SUBTEST(sanitycheck), + SUBTEST(signal), + SUBTEST(cb_autodestroy), + SUBTEST(cb_destroy_12), + SUBTEST(cb_destroy_21), + SUBTEST(cb_destroy_22), + }; + + return subtests(tests, NULL); +} diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c index 116dad6c7905..7c229f4a6b56 100644 --- a/drivers/dma-buf/sw_sync.c +++ b/drivers/dma-buf/sw_sync.c @@ -426,3 +426,42 @@ const struct file_operations sw_sync_debugfs_fops = { .unlocked_ioctl = sw_sync_ioctl, .compat_ioctl = compat_ptr_ioctl, }; + +#if IS_ENABLED(CONFIG_DMABUF_SELFTESTS) +struct sync_timeline *st_sync_timeline_create(const char *name) +{ + return sync_timeline_create(name); +} +EXPORT_SYMBOL_GPL(st_sync_timeline_create); + +void st_sync_timeline_get(struct sync_timeline *tl) +{ + sync_timeline_get(tl); +} +EXPORT_SYMBOL_GPL(st_sync_timeline_get); + +void st_sync_timeline_put(struct sync_timeline *tl) +{ + sync_timeline_put(tl); +} +EXPORT_SYMBOL_GPL(st_sync_timeline_put); + +void st_sync_timeline_signal(struct sync_timeline *tl, unsigned int inc) +{ + sync_timeline_signal(tl, inc); +} +EXPORT_SYMBOL_GPL(st_sync_timeline_signal); + +struct dma_fence * +st_sync_pt_create(struct sync_timeline *tl, unsigned int seqno) +{ + struct sync_pt *pt; + + pt = sync_pt_create(tl, seqno); + if (!pt) + return NULL; + + return &pt->base; +} +EXPORT_SYMBOL_GPL(st_sync_pt_create); +#endif diff --git a/drivers/dma-buf/sync_debug.h b/drivers/dma-buf/sync_debug.h index fd073fc32329..7c96d9a2c02d 100644 --- a/drivers/dma-buf/sync_debug.h +++ b/drivers/dma-buf/sync_debug.h @@ -71,4 +71,12 @@ void sync_timeline_debug_remove(struct sync_timeline *obj); void sync_file_debug_add(struct sync_file *fence); void sync_file_debug_remove(struct sync_file *fence); +struct sync_timeline *st_sync_timeline_create(const char *name); +void st_sync_timeline_get(struct sync_timeline *tl); +void st_sync_timeline_put(struct sync_timeline *tl); +void st_sync_timeline_signal(struct sync_timeline *tl, unsigned int inc); + +struct dma_fence * +st_sync_pt_create(struct sync_timeline *tl, unsigned int seqno); + #endif /* _LINUX_SYNC_H */
While sw_sync is purely a debug facility for userspace to create fences and timelines it can control, nevertheless it has some tricky locking semantics of its own. In particular, Bas Nieuwenhuize reported that we had reintroduced a deadlock if a signal callback attempted to destroy the fence. So let's add a few trivial selftests to make sure that once fixed again, it stays fixed. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl> --- drivers/dma-buf/Makefile | 3 +- drivers/dma-buf/selftests.h | 1 + drivers/dma-buf/st-sw_sync.c | 279 +++++++++++++++++++++++++++++++++++ drivers/dma-buf/sw_sync.c | 39 +++++ drivers/dma-buf/sync_debug.h | 8 + 5 files changed, 329 insertions(+), 1 deletion(-) create mode 100644 drivers/dma-buf/st-sw_sync.c