From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-ed1-f54.google.com (mail-ed1-f54.google.com [209.85.208.54]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 67070400DFC for ; Wed, 8 Jul 2026 09:20:59 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.208.54 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1783502461; cv=none; b=ReAc5+vsk8b1eG6i/dydB2KVf9v7qxZbVG2wegLYCGTHzME/h2+fE9BF/JidEcGunKCqjwl754jYvB3f2F34ScOzcKxy1n6is3p8HbdDb1VyeSM4Xee3dtFujOkpbxy1IwC1Q3lDz3D7RTIBk4bAYVXCQXEgVOkPmaRWmxXrbkQ= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1783502461; c=relaxed/simple; bh=fCIKLEC+yY3dvMQv7v6sJn1BUSGaU9FuAkoYPaBDqJs=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=djWDR0zypIZVh95MaB5IWOI0f9eUDZo9lct7GLCYGnf/astgpb150VVsTb3gzKlp+2yvhIg6jbxFefeEdkTzZ56K7xzDRcSC4/Rsnpa8iRMNYbuXJ+Kh5DvTQLr7L6zsL6JSsxVl9WbGM4LtmI89B4JvphPLugcmu0CtWb88AGQ= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=ursulin.net; spf=pass smtp.mailfrom=ursulin.net; dkim=pass (2048-bit key) header.d=ursulin.net header.i=@ursulin.net header.b=g9vr7kXj; arc=none smtp.client-ip=209.85.208.54 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=ursulin.net Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=ursulin.net Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=ursulin.net header.i=@ursulin.net header.b="g9vr7kXj" Received: by mail-ed1-f54.google.com with SMTP id 4fb4d7f45d1cf-698a9f11776so781371a12.1 for ; Wed, 08 Jul 2026 02:20:59 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ursulin.net; s=google; t=1783502458; x=1784107258; darn=vger.kernel.org; h=content-transfer-encoding:content-type:in-reply-to:from :content-language:references:cc:to:subject:user-agent:mime-version :date:message-id:from:to:cc:subject:date:message-id:reply-to :content-type; bh=W7NiDQJoKDQdV8YX3g9oeMoz6qFJFZ46J3+eMQIsCQw=; b=g9vr7kXjB5fAWN+cysCyamjDrZXp28+WrQWioCTiB0AxDU1PjxEt89Z3xj10FE6vqZ bS9YIV2ZlsgZggpAV6pBtK54GEdL+oeMe9ztE0XHTkXZSLMkuf+ePXEeqwS21VCdzpEW xuqwCvD3S8zyirnmUGzQ8sVhY61PGEWbFfzA9x/DX2VDbgam3xIL1Cfdq7Q2/JXLYTEQ FAbummGREJGPQzmPv8wHAdYXreN9AhCB2d5v61EE+8OLcdw3jJ1Zou0PGzDznH7QVPvp Nwce12NotKCuFCRHlqpWvlpcLlomvgjnFsSkdJhYm1I7m6E+5PMhW/tMAOc+J+PCIDJ8 VNIw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1783502458; x=1784107258; h=content-transfer-encoding:content-type:in-reply-to:from :content-language:references:cc:to:subject:user-agent:mime-version :date:message-id:x-gm-gg:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to:content-type; bh=W7NiDQJoKDQdV8YX3g9oeMoz6qFJFZ46J3+eMQIsCQw=; b=RxxExfV0TCI6EwiNe5fr+vp00SgUtPfPA6vQQ5V39gAjBwCCg3oScXzFIoQysouy/N u/ep8jmPRV+ayrkwq0XREKQE4zSaSdcQRivXQKbFSTNcWYdvkeANUSQeBtHZ7S/8Tk5N mEQk1RlpojfFnJrbmUMYELQdSDstRugaZMgY0ykEsu9tfrYeufa11L52lu8MTUsvL3OT 6VFhSIgryNA2VYDPwaJWquQX0jxGOTGZH/8BDNUC/aygTeGcL53uGYo5as7vo6dlO7ua 5r4H+zLXXmfWLqQGw7LKUHqJa/QCshQErwrFxsCeWSgcr1NkZ0q71nRYD2P8nJzNABC6 qEMA== X-Forwarded-Encrypted: i=1; AHgh+RrZnPafCRIEAu3JZxCvt1rAuALsaipkN3VXTJJr4p3voreI44AM3A97Z8vA6UN77gXs6C9AWpzmRvgxXFs=@vger.kernel.org X-Gm-Message-State: AOJu0YyPIISciKzHIW4yTnUoob5TpB9xoKfR5J+r+XreBFz8H2xtfNVW 6jk0JpYeFGhZHV2UHLpHjGR8QZASraBm29u+cb3e5clL4HlHMtN/udK6X3XfypjBS0c= X-Gm-Gg: AfdE7ckXfUfWO0gUpD7KrDX2XiTqFCU8ODv1L3kJaftuP2KZXmZooVBVrbodrsdf5VT CjZsJSlGB/CxUG4/vgLfm+YaXpzQnxFJUoeCxCBo5R1EryxLApCUxes8HTxFBfvx79oG3dtqIQ1 XZBpmLlyTbWY4c5ZJ43hxN1B7ows8gmThdTJKJ91tBiUGzTTVAyqLzMHqigo2brujOauRZ6ZUrR xGnhy10po8cjNam5+tCp8+QPiA47UnrsEpnHKa6+/qD+NmUDD9cuC3Wl20a9fHYh4SITzzeqdVA Drnveb69+SvukSmmoLvdiTeNheg6qRBfRQ+N0bxYFf0aIPtx/oIgN342jY9LrX4osh5aEziWHbv EL44hUkjf4/DS/PjjSbFrG4KjrMRCxqgN8sK2vgS4QiwpzU2gof4eCRhKZ2kdV7v4pAhCwAjVO+ 0NdV1GSgUokPuzzUFWQfNAK6wIiFXLvRBW8A== X-Received: by 2002:a05:6402:388b:b0:68f:cc95:8c10 with SMTP id 4fb4d7f45d1cf-69ab44a07efmr572815a12.27.1783502457112; Wed, 08 Jul 2026 02:20:57 -0700 (PDT) Received: from [192.168.0.116] ([90.240.106.137]) by smtp.gmail.com with ESMTPSA id 4fb4d7f45d1cf-69aa613d4e2sm1678874a12.24.2026.07.08.02.20.56 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 08 Jul 2026 02:20:56 -0700 (PDT) Message-ID: <46ffa75c-83e2-4f4b-9d5b-e43fdfa12176@ursulin.net> Date: Wed, 8 Jul 2026 10:20:55 +0100 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [RFC PATCH] drm/sched/tests: Let the DRM scheduler manage job lifetimes To: Marco Pagani , Matthew Brost , Danilo Krummrich , Philipp Stanner , =?UTF-8?Q?Christian_K=C3=B6nig?= , Maarten Lankhorst , Maxime Ripard , Thomas Zimmermann , David Airlie , Simona Vetter Cc: dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org References: <20260707114807.154572-1-marco.pagani@linux.dev> Content-Language: en-GB From: Tvrtko Ursulin In-Reply-To: <20260707114807.154572-1-marco.pagani@linux.dev> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit On 07/07/2026 12:48, Marco Pagani wrote: > Currently, the mock scheduler uses KUnit-managed memory for jobs. This ties > the job's memory lifetime to the test suite rather than the DRM scheduler's > callbacks. This does not represent real driver behavior and can lead to > potential Use-After-Free bugs in the tests. Since you say use-after-free, that means something belonging to a test wasn't properly cleaned up, right? Could you help me out review by describing one example? Idea was that tests are self contained, ie. when a tests exits it is guaranteed all resources belonging to it are gone. Everything else should be a test bug which IMO means it is fine to crash if a new tests gets that wrong. Just idle at the end of the test and everything should work. > Update the mock scheduler to let the lifetime of jobs be managed by the DRM > scheduler's asynchronous callbacks instead of KUnit managed memory. Add a > kref reference counter to track the job's lifetime between test suites and > the scheduler. > > Finally, to avoid memory leaks in the event of an early test abortion, > register a cleanup KUnit action that automatically puts the reference to > the job. So kunit managed memory, freed on test exit, is replaced by kunit managed actions, which also free on test exit? Some random thoughts below while I wait to understand the motivation and problem statement better: > Signed-off-by: Marco Pagani > --- > .../gpu/drm/scheduler/tests/mock_scheduler.c | 68 ++++++++++++++----- > drivers/gpu/drm/scheduler/tests/sched_tests.h | 6 ++ > 2 files changed, 57 insertions(+), 17 deletions(-) > > diff --git a/drivers/gpu/drm/scheduler/tests/mock_scheduler.c b/drivers/gpu/drm/scheduler/tests/mock_scheduler.c > index 14403a762335..51f81082f37a 100644 > --- a/drivers/gpu/drm/scheduler/tests/mock_scheduler.c > +++ b/drivers/gpu/drm/scheduler/tests/mock_scheduler.c > @@ -96,6 +96,14 @@ drm_mock_sched_job_signal_timer(struct hrtimer *hrtimer) > return HRTIMER_NORESTART; > } > > +static void drm_mock_sched_job_cleanup_action(void *ptr) > +{ > + struct drm_mock_sched_job *job = ptr; > + > + job->test = NULL; > + drm_mock_sched_job_put(job); > +} > + > /** > * drm_mock_sched_job_new - Create a new mock scheduler job > * > @@ -111,23 +119,34 @@ drm_mock_sched_job_new(struct kunit *test, > struct drm_mock_sched_job *job; > int ret; > > - job = kunit_kzalloc(test, sizeof(*job), GFP_KERNEL); > + /* Let the DRM Scheduler manage the lifetime of the job */ However that does not match real driver behaviour either (circling back to the justification from the commit message). It is the drivers which own this memory, not the DRM scheduler. So in this case mock scheduler, which is a kunit test client, etc. > + job = kzalloc_obj(*job, GFP_KERNEL); > KUNIT_ASSERT_NOT_NULL(test, job); > > + kref_init(&job->refcount); > + job->test = test; > + > ret = drm_sched_job_init(&job->base, > &entity->base, > 1, > NULL, > 1); > - KUNIT_ASSERT_EQ(test, ret, 0); > - > - job->test = test; > + if (ret) { > + kfree(job); > + KUNIT_ASSERT_EQ_MSG(test, ret, 0, "drm_sched_job_init failed"); > + } > > init_completion(&job->done); > INIT_LIST_HEAD(&job->link); > hrtimer_setup(&job->timer, drm_mock_sched_job_signal_timer, > CLOCK_MONOTONIC, HRTIMER_MODE_ABS); > > + ret = kunit_add_action(test, drm_mock_sched_job_cleanup_action, job); > + if (ret) { > + drm_mock_sched_job_put(job); > + KUNIT_ASSERT_EQ_MSG(test, ret, 0, "kunit_add_action failed"); > + } > + > return job; > } > > @@ -152,7 +171,7 @@ static void drm_mock_sched_hw_fence_release(struct dma_fence *fence) > > hrtimer_cancel(&job->timer); > > - /* Containing job is freed by the kunit framework */ > + drm_mock_sched_job_put(job); > } > > static const struct dma_fence_ops drm_mock_sched_hw_fence_ops = { > @@ -173,6 +192,8 @@ static struct dma_fence *mock_sched_run_job(struct drm_sched_job *sched_job) > sched->hw_timeline.context, > atomic_inc_return(&sched->hw_timeline.next_seqno)); > > + kref_get(&job->refcount); > + > dma_fence_get(&job->hw_fence); /* Reference for the job_list */ > > spin_lock_irq(&sched->lock); > @@ -200,6 +221,17 @@ static struct dma_fence *mock_sched_run_job(struct drm_sched_job *sched_job) > return &job->hw_fence; > } > > +static void mock_sched_free_job(struct drm_sched_job *sched_job) > +{ Free which is actually a reference put is non-obvious API. > + struct drm_mock_sched_job *job = drm_sched_job_to_mock_job(sched_job); > + > + /* Only if the fence has been successfully initialized */ > + if (job->hw_fence.ops) > + dma_fence_put(&job->hw_fence); > + > + drm_mock_sched_job_put(job); > +} > + > /* > * Normally, drivers would take appropriate measures in this callback, such as > * killing the entity the faulty job is associated with, resetting the hardware > @@ -232,21 +264,26 @@ mock_sched_timedout_job(struct drm_sched_job *sched_job) > } > spin_unlock_irqrestore(&sched->lock, flags); > > - dma_fence_put(&job->hw_fence); > - drm_sched_job_cleanup(sched_job); > - /* Mock job itself is freed by the kunit framework. */ > + mock_sched_free_job(sched_job); > > return DRM_GPU_SCHED_STAT_RESET; > } > > -static void mock_sched_free_job(struct drm_sched_job *sched_job) > +static void drm_mock_sched_job_release(struct kref *ref) > { > - struct drm_mock_sched_job *job = drm_sched_job_to_mock_job(sched_job); > + struct drm_mock_sched_job *job; > + > + job = container_of(ref, struct drm_mock_sched_job, refcount); > > - dma_fence_put(&job->hw_fence); > - drm_sched_job_cleanup(sched_job); > + drm_sched_job_cleanup(&job->base); > > - /* Mock job itself is freed by the kunit framework. */ > + kfree(job); > +} > + > +void drm_mock_sched_job_put(struct drm_mock_sched_job *job) > +{ > + if (job) > + kref_put(&job->refcount, drm_mock_sched_job_release); > } > > static void mock_sched_cancel_job(struct drm_sched_job *sched_job) > @@ -265,10 +302,7 @@ static void mock_sched_cancel_job(struct drm_sched_job *sched_job) > } > spin_unlock_irqrestore(&sched->lock, flags); > > - /* > - * The GPU Scheduler will call drm_sched_backend_ops.free_job(), still. > - * Mock job itself is freed by the kunit framework. > - */ > + /* The GPU Scheduler will call drm_sched_backend_ops.free_job() */ > } > > static const struct drm_sched_backend_ops drm_mock_scheduler_ops = { > diff --git a/drivers/gpu/drm/scheduler/tests/sched_tests.h b/drivers/gpu/drm/scheduler/tests/sched_tests.h > index 553d45abd057..e4d33f0bf935 100644 > --- a/drivers/gpu/drm/scheduler/tests/sched_tests.h > +++ b/drivers/gpu/drm/scheduler/tests/sched_tests.h > @@ -13,6 +13,7 @@ > #include > #include > #include > +#include > > #include > > @@ -90,6 +91,7 @@ struct drm_mock_sched_entity { > */ > struct drm_mock_sched_job { > struct drm_sched_job base; > + struct kref refcount; > > struct completion done; > > @@ -144,6 +146,8 @@ struct drm_mock_sched_job * > drm_mock_sched_job_new(struct kunit *test, > struct drm_mock_sched_entity *entity); > > +void drm_mock_sched_job_put(struct drm_mock_sched_job *job); > + > /** > * drm_mock_sched_job_submit - Arm and submit a job in one go > * > @@ -151,6 +155,8 @@ drm_mock_sched_job_new(struct kunit *test, > */ > static inline void drm_mock_sched_job_submit(struct drm_mock_sched_job *job) > { > + kref_get(&job->refcount); > + This feels fragile because if someone writes a test which uses arm+push directly bad things will happen. What is this extra reference for versus the one obtained by creating a job? Regards, Tvrtko > drm_sched_job_arm(&job->base); > drm_sched_entity_push_job(&job->base); > }