On Thu, Mar 02, 2023 at 09:34:22AM +0100, Thomas Hellström wrote:
> Add a suballocator test to get some test coverage for the new drm
> suballocator, and perform some basic timing (elapsed time).
>
> Signed-off-by: Thomas Hellström <thomas.hellstrom@xxxxxxxxxxxxxxx>
> ---
> drivers/gpu/drm/Kconfig | 1 +
> drivers/gpu/drm/tests/Makefile | 3 +-
> drivers/gpu/drm/tests/drm_suballoc_test.c | 356 ++++++++++++++++++++++
> 3 files changed, 359 insertions(+), 1 deletion(-)
> create mode 100644 drivers/gpu/drm/tests/drm_suballoc_test.c
>
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index 8fbe57407c60..dced53723721 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -78,6 +78,7 @@ config DRM_KUNIT_TEST
> select DRM_LIB_RANDOM
> select DRM_KMS_HELPER
> select DRM_BUDDY
> + select DRM_SUBALLOC_HELPER
> select DRM_EXPORT_FOR_TESTS if m
> select DRM_KUNIT_TEST_HELPERS
> default KUNIT_ALL_TESTS
> diff --git a/drivers/gpu/drm/tests/Makefile b/drivers/gpu/drm/tests/Makefile
> index bca726a8f483..c664944a48ab 100644
> --- a/drivers/gpu/drm/tests/Makefile
> +++ b/drivers/gpu/drm/tests/Makefile
> @@ -17,6 +17,7 @@ obj-$(CONFIG_DRM_KUNIT_TEST) += \
> drm_modes_test.o \
> drm_plane_helper_test.o \
> drm_probe_helper_test.o \
> - drm_rect_test.o
> + drm_rect_test.o \
> + drm_suballoc_test.o
>
> CFLAGS_drm_mm_test.o := $(DISABLE_STRUCTLEAK_PLUGIN)
> diff --git a/drivers/gpu/drm/tests/drm_suballoc_test.c b/drivers/gpu/drm/tests/drm_suballoc_test.c
> new file mode 100644
> index 000000000000..e7303a5505a0
> --- /dev/null
> +++ b/drivers/gpu/drm/tests/drm_suballoc_test.c
> @@ -0,0 +1,356 @@
> +// SPDX-License-Identifier: GPL-2.0 OR MIT
> +/*
> + * Test case for the drm_suballoc suballocator manager
> + * Copyright 2023 Intel Corporation.
> + */
> +
> +#include <kunit/test.h>
> +
> +#include <linux/dma-fence.h>
> +#include <linux/ktime.h>
> +#include <linux/hrtimer.h>
> +#include <linux/sizes.h>
> +#include <linux/slab.h>
> +#include <linux/spinlock.h>
> +#include <linux/delay.h>
> +#include <drm/drm_suballoc.h>
> +
> +#define SA_ITERATIONS 10000
> +#define SA_SIZE SZ_1M
> +#define SA_DEFAULT_ALIGN SZ_4K
> +
> +static bool intr = true;
> +static bool from_reclaim;
> +static bool pre_throttle;
> +static unsigned int num_rings = 4;
> +static unsigned int iterations = SA_ITERATIONS;
> +
> +static atomic64_t free_space;
> +
> +static atomic_t other_id;
> +
> +struct suballoc_fence;
> +
> +/**
> + * struct suballoc_ring - fake gpu engine.
> + * @list: List of fences to signal.
> + * @signal_time: Accumulated fence signal execution time.
> + * @lock: Protects the suballoc ring members. hardirq safe.
> + * @hrtimer: Fake execution time timer.
> + * @active: The currently active fence for which we have pending work or a
> + * timer running.
> + * @seqno: Fence submissin seqno.
> + * @idx: Index for calculation of fake execution time.
> + * @work: Work struct used solely to move the timer start to a different
> + * processor than that used for submission.
> + */
> +struct suballoc_ring {
> + ktime_t signal_time;
> + struct list_head list;
> + /* Protect the ring processing. */
> + spinlock_t lock;
> + struct hrtimer hrtimer;
> + struct suballoc_fence *active;
> + atomic64_t seqno;
> + u32 idx;
> + struct work_struct work;
> +};
> +
> +/**
> + * struct suballoc_fence - Hrtimer-driven fence.
> + * @fence: The base class fence struct.
> + * @link: Link for the ring's fence list.
> + * @size: The size of the suballocator range associated with this fence.
> + * @id: Cpu id likely used by the submission thread for suballoc allocation.
> + */
> +struct suballoc_fence {
> + struct dma_fence fence;
> + struct list_head link;
> + size_t size;
> + unsigned int id;
> +};
> +
> +/* A varying but repeatable fake execution time */
> +static ktime_t ring_next_delay(struct suballoc_ring *ring)
> +{
> + return ns_to_ktime((u64)(++ring->idx % 8) * 200 * NSEC_PER_USEC);
> +}
Is there any way we can avoid using time (and large number of
iterations) here, while keeping the coverage?
drm_suballoc have longest runtime out of all tests in DRM (taking ~60%
of the whole DRM kunit execution, drm_mm being the second and taking
~35%, without those two suites DRM tests execute in milliseconds rather
than tens of seconds),
Building test cases in a way that operate on time basis makes it tricky
to optimize the runtime.
If we extract various parameters from modparams to separate test cases,
it's going to get even worse.
> +
> +/*
> + * Launch from a work item to decrease the likelyhood of the timer expiry
> + * callback getting called from the allocating cpu.
> + * We want to trigger cache-line bouncing between allocating and signalling
> + * cpus.
> + */
> +static void ring_launch_timer_work(struct work_struct *work)
> +{
> + struct suballoc_ring *ring =
> + container_of(work, typeof(*ring), work);
> +
> + spin_lock_irq(&ring->lock);
> + if (ring->active)
> + hrtimer_start_range_ns(&ring->hrtimer, ring_next_delay(ring),
> + 100ULL * NSEC_PER_USEC,
> + HRTIMER_MODE_REL_PINNED);
> +
> + spin_unlock_irq(&ring->lock);
> +}
> +
> +/*
> + * Signal an active fence and pull the next off the list if any and make it
> + * active.
> + */
> +static enum hrtimer_restart ring_hrtimer_expired(struct hrtimer *hrtimer)
> +{
> + struct suballoc_ring *ring =
> + container_of(hrtimer, typeof(*ring), hrtimer);
> + struct suballoc_fence *sfence;
> + ktime_t now, then;
> + unsigned long irqflags;
> +
> + spin_lock_irqsave(&ring->lock, irqflags);
> + sfence = ring->active;
> +
> + if (sfence) {
> + struct dma_fence *fence = &sfence->fence;
> +
> + if (sfence->id != get_cpu())
> + atomic_inc(&other_id);
> + put_cpu();
> +
> + then = ktime_get();
> + dma_fence_signal(fence);
> + now = ktime_get();
> + dma_fence_put(fence);
> + ring->signal_time = ktime_add(ring->signal_time,
> + ktime_sub(now, then));
> + ring->active = NULL;
> + atomic64_add(sfence->size, &free_space);
> + }
> +
> + sfence = list_first_entry_or_null(&ring->list, typeof(*sfence), link);
> + if (sfence) {
> + list_del_init(&sfence->link);
> + ring->active = sfence;
> + spin_unlock_irqrestore(&ring->lock, irqflags);
> + hrtimer_forward_now(&ring->hrtimer, ring_next_delay(ring));
> + return HRTIMER_RESTART;
> + }
> + spin_unlock_irqrestore(&ring->lock, irqflags);
> +
> + return HRTIMER_NORESTART;
> +}
> +
> +/*
> + * Queue a fence on a ring and if it's the first fence, make it active.
> + */
> +static void ring_add_fence(struct suballoc_ring *ring,
> + struct suballoc_fence *sfence)
> +{
> + spin_lock_irq(&ring->lock);
> + if (!ring->active) {
> + ring->active = sfence;
> + queue_work(system_unbound_wq, &ring->work);
> + } else {
> + list_add_tail(&sfence->link, &ring->list);
> + }
> + spin_unlock_irq(&ring->lock);
> +}
> +
> +static void ring_init(struct suballoc_ring *ring)
> +{
> + memset(ring, 0, sizeof(*ring));
> + INIT_LIST_HEAD(&ring->list);
> + spin_lock_init(&ring->lock);
> + hrtimer_init(&ring->hrtimer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
> + ring->hrtimer.function = ring_hrtimer_expired;
> + INIT_WORK(&ring->work, ring_launch_timer_work);
> +}
> +
> +static bool ring_idle(struct suballoc_ring *ring)
> +{
> + bool tmp;
> +
> + spin_lock_irq(&ring->lock);
> + tmp = !ring->active;
> + spin_unlock_irq(&ring->lock);
> +
> + return tmp;
> +}
> +
> +static const char *dma_fence_get_suballoc_name(struct dma_fence *fence)
> +{
> + return "suballoc";
> +}
> +
> +static const struct dma_fence_ops dma_fence_suballoc_ops = {
> + .get_driver_name = dma_fence_get_suballoc_name,
> + .get_timeline_name = dma_fence_get_suballoc_name,
> +};
> +
> +static DEFINE_SPINLOCK(sa_fence_lock);
> +static ktime_t alloctime, freetime;
> +
> +static void drm_test_suballoc(struct kunit *test)
> +{
> + struct suballoc_ring *rings;
> + struct drm_suballoc_manager sa_manager;
> + struct drm_suballoc *sa;
> + struct suballoc_fence *sfence;
> + struct dma_fence *fence;
> + ktime_t then, now, signaltime;
> + int i, ring, iter_tot = 0;
> + size_t size;
> + unsigned int align;
> + unsigned long long soffset;
> + gfp_t gfp;
> +
> + rings = kvmalloc_array(num_rings, sizeof(*rings), GFP_KERNEL);
> + if (!rings) {
> + KUNIT_FAIL(test, "Failed allocating %u rings.\n");
> + return;
> + }
KUNIT_ASSERT_NOT_NULL?
Though we might want to implement a test-resource managed variant
(kunit_kvmalloc_array) to not have to worry about lifecycle and freeing
the resources.
> +
> + for (i = 0; i < num_rings; ++i)
> + ring_init(rings + i);
With resource managed - rings could be allocated and initialized at
.init(). We would then call the flush and wait at .exit(), and as a
result, we would be able to use asserts in test body without worrying
about leaking.
> +
> + atomic64_set(&free_space, SA_SIZE);
> + drm_suballoc_manager_init(&sa_manager, SA_SIZE, SA_DEFAULT_ALIGN);
This could also be moved to .init()
> +
> + if (from_reclaim)
> + gfp = GFP_NOWAIT | __GFP_NOWARN;
> + else
> + gfp = GFP_KERNEL;
> +
> + for (i = 0; i < iterations; ++i) {
> + ring = i % num_rings;
> + size = (ring + 1) * SZ_4K;
> + align = 1 << (ring % const_ilog2(SA_DEFAULT_ALIGN));
> +
> + if (pre_throttle)
> + while (atomic64_read(&free_space) < SA_SIZE / 2)
> + cpu_relax();
> +
> + if (from_reclaim)
> + fs_reclaim_acquire(GFP_KERNEL);
> +
> + then = ktime_get();
> + sa = drm_suballoc_new(&sa_manager, size, gfp, intr, align);
> + now = ktime_get();
> + if (from_reclaim)
> + fs_reclaim_release(GFP_KERNEL);
> +
> + alloctime = ktime_add(alloctime, ktime_sub(now, then));
> +
> + iter_tot++;
> + if (IS_ERR(sa)) {
KUNIT_ASSERT_NOT_ERR_OR_NULL?
> + if (from_reclaim) {
drm_suballoc_new can fail for other reasons than -ENOMEM under memory
pressure, while with from_reclaim we're treating all errors as a
success, is that intentional?
> + iter_tot--;
> + continue;
> + }
> +
> + KUNIT_FAIL(test, "drm_suballoc_new() returned %pe\n",
> + sa);
> + break;
> + }
> +
> + atomic64_sub(size, &free_space);
> + soffset = drm_suballoc_soffset(sa);
> + if (!IS_ALIGNED(soffset, align)) {
> + drm_suballoc_free(sa, NULL);
Do we need to worry about calling free here? We shouldn't leak as long
as we wait upon all fences, as drm_suballoc_manager_fini will do the
clean up for us.
KUNIT_EXPECT_TRUE_MSG(..., IS_ALIGNED(soffset, align), ...)?
> + KUNIT_FAIL(test, "Incorrect alignment: offset %llu align %u rem %llu\n",
> + soffset, align, soffset & (align - 1));
> + break;
> + }
> +
> + if (drm_suballoc_eoffset(sa) > SA_SIZE) {
> + drm_suballoc_free(sa, NULL);
> + KUNIT_FAIL(test, "Allocation beyond end.\n");
> + break;
> + }
KUNIT_EXPECT_LE_MSG?
> +
> + if (drm_suballoc_size(sa) < size ||
> + drm_suballoc_size(sa) >= size + align) {
> + drm_suballoc_free(sa, NULL);
> + KUNIT_FAIL(test, "Incorrect size.\n");
> + break;
> + }
KUNIT_EXPECT_GE and KUNIT_EXPECT_LT?
> +
> + sfence = kmalloc(sizeof(*sfence), GFP_KERNEL);
> + if (unlikely(!sfence)) {
> + drm_suballoc_free(sa, NULL);
> + KUNIT_FAIL(test, "Fence allocation failed.\n");
> + break;
> + }
It looks like sfence is never released. kunit_kmalloc?
KUNIT_ASSERT_NOT_NULL / KUNIT_ASSERT_NOT_ERR_OR_NULL?
> + fence = &sfence->fence;
> + dma_fence_init(fence, &dma_fence_suballoc_ops, &sa_fence_lock,
> + ring + 1,
> + atomic64_inc_return(&rings[ring].seqno));
> + sfence->size = size;
> + sfence->id = get_cpu();
> + put_cpu();
> +
> + ring_add_fence(rings + ring, sfence);
> +
> + then = ktime_get();
> + drm_suballoc_free(sa, fence);
> + now = ktime_get();
> + freetime = ktime_add(freetime, ktime_sub(now, then));
> + }
> +
> + signaltime = ktime_set(0, 0);
> + for (i = 0; i < num_rings; ++i) {
> + struct suballoc_ring *sring = &rings[i];
> +
> + flush_work(&sring->work);
> + while (!ring_idle(sring))
> + schedule();
> + signaltime = ktime_add(signaltime, sring->signal_time);
> + }
This (and drm_suballoc_manager_fini) could be moved to .exit()
> +
> + kvfree(rings);
> +
> + kunit_info(test, "signals on different processor: %d of %d\n",
> + atomic_read(&other_id), iter_tot);
> + drm_suballoc_manager_fini(&sa_manager);
> + kunit_info(test, "Alloc time was %llu ns.\n", (unsigned long long)
> + ktime_to_ns(alloctime) / iter_tot);
> + kunit_info(test, "Free time was %llu ns.\n", (unsigned long long)
> + ktime_to_ns(freetime) / iter_tot);
> + kunit_info(test, "Signal time was %llu ns.\n", (unsigned long long)
> + ktime_to_ns(signaltime) / iter_tot);
Do we need those timings?
If we do expect certain values (probably with some epsilon range), we
should handle it as a separate test.
> +
> + if (atomic64_read(&free_space) != SA_SIZE) {
> + kunit_warn(test, "Test sanity check failed.\n");
> + kunit_warn(test, "Space left at exit is %lld of %d\n",
> + (long long)atomic64_read(&free_space), SA_SIZE);
> + }
If this is an error - let's add it as an "expect".
Otherwise it's not printed if the test PASSes (unless we're running with
raw output).
> +}
> +
> +module_param(intr, bool, 0400);
> +MODULE_PARM_DESC(intr, "Whether to wait interruptible for space.");
This should be a separate test case (or param to a test case), not a
modparam.
> +module_param(from_reclaim, bool, 0400);
> +MODULE_PARM_DESC(from_reclaim, "Whether to suballocate from reclaim context.");
Same here.
> +module_param(pre_throttle, bool, 0400);
> +MODULE_PARM_DESC(pre_throttle, "Whether to have the test throttle for space "
> + "before allocations.");
And here.
> +module_param(num_rings, uint, 0400);
> +MODULE_PARM_DESC(num_rings, "Number of rings signalling fences in order.\n");
> +module_param(iterations, uint, 0400);
> +MODULE_PARM_DESC(iterations, "Number of allocations to perform.\n");
Do we expect any difference in coverage for different number of rings /
iterations? What's the relation here? Would it be possible to extract
specific values (for which we expect different behavior) to separate
testcases?
-Michał
> +
> +static struct kunit_case drm_suballoc_tests[] = {
> + KUNIT_CASE(drm_test_suballoc),
> + {}
> +};
> +
> +static struct kunit_suite drm_suballoc_test_suite = {
> + .name = "drm_suballoc",
> + .test_cases = drm_suballoc_tests,
> +};
> +
> +kunit_test_suite(drm_suballoc_test_suite);
> +
> +MODULE_AUTHOR("Intel Corporation");
> +MODULE_DESCRIPTION("DRM suballocator Kunit test");
> +MODULE_LICENSE("Dual MIT/GPL");
> --
> 2.34.1
>
