Re: [PATCH 3/3] dma-buf/selftests: Add locking selftests for sw_sync

From: Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl>
To: Chris Wilson <chris@chris-wilson.co.uk>
Cc: intel-gfx@lists.freedesktop.org,
	ML dri-devel <dri-devel@lists.freedesktop.org>
Subject: Re: [PATCH 3/3] dma-buf/selftests: Add locking selftests for sw_sync
Date: Tue, 14 Jul 2020 22:29:59 +0200	[thread overview]
Message-ID: <CAP+8YyF0bZbj_ESm3apcy=0wE7rCu2JEyjGS8P=TCntGQgVyKg@mail.gmail.com> (raw)
In-Reply-To: <20200714200646.14041-3-chris@chris-wilson.co.uk>

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
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel