Message-ID: <dde160ab-57bd-4727-be7f-d035cd9e7f19@linux.dev>
Date: Fri, 6 Feb 2026 11:36:45 +0100
From: Marco Pagani <marco.pagani@...ux.dev>
To: Tvrtko Ursulin <tursulin@...ulin.net>
Cc: airlied@...il.com, ckoenig.leichtzumerken@...il.com, dakr@...nel.org,
 dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
 maarten.lankhorst@...ux.intel.com, marpagan@...hat.com,
 matthew.brost@...el.com, mripard@...nel.org, phasta@...nel.org,
 simona@...ll.ch, tzimmermann@...e.de
Subject: Re: [RFC PATCH] drm/sched: Add new KUnit test suite for concurrent
 job submission



On 05/02/2026 10:53, Tvrtko Ursulin wrote:
> 
> On 04/02/2026 16:33, Marco Pagani wrote:
> 
> 8><
> 
>>>>>> +	{
>>>>>> +		.description = "Concurrently submit multiple jobs in a single entity",
>>>>>> +		.job_base_us = 1000,
>>>>>> +		.num_jobs = 10,
>>>>>> +		.num_subs = 64,
>>>>>> +	},
>>>>>> +};
>>>>>> +
>>>>>> +static void
>>>>>> +drm_sched_concurrent_desc(const struct drm_sched_concurrent_params *params, char *desc)
>>>>>> +{
>>>>>> +	strscpy(desc, params->description, KUNIT_PARAM_DESC_SIZE);
>>>>>> +}
>>>>>> +
>>>>>> +KUNIT_ARRAY_PARAM(drm_sched_concurrent, drm_sched_concurrent_cases, drm_sched_concurrent_desc);
>>>>>> +
>>>>>> +struct submitter_data {
>>>>>> +	struct work_struct work;
>>>>>> +	struct sched_concurrent_test_context *ctx;
>>>>>> +	struct drm_mock_sched_entity *entity;
>>>>>> +	struct drm_mock_sched_job **jobs;
>>>>>> +	struct kunit *test;
>>>>>> +	unsigned int id;
>>>>>> +	bool timedout;
>>>>>> +};
>>>>>> +
>>>>>> +static void drm_sched_submitter_worker(struct work_struct *work)
>>>>>> +{
>>>>>> +	const struct drm_sched_concurrent_params *params;
>>>>>> +	struct sched_concurrent_test_context *ctx;
>>>>>> +	struct submitter_data *sub_data;
>>>>>> +	unsigned int i, duration_us;
>>>>>> +	unsigned long timeout_jiffies;
>>>>>> +	bool done;
>>>>>> +
>>>>>> +	sub_data = container_of(work, struct submitter_data, work);
>>>>>> +	ctx = sub_data->ctx;
>>>>>> +	params = sub_data->test->param_value;
>>>>>> +
>>>>>> +	wait_for_completion(&ctx->wait_go);
>>>>>> +
>>>>>> +	for (i = 0; i < params->num_jobs; i++) {
>>>>>> +		duration_us = params->job_base_us + (sub_data->id * 10);
>>>>>
>>>>> Why is job duration dependent by the submitter id?
>>>>
>>>> Just a simple way to have a deterministic distribution of durations.
>>>> I can change it if it doesn't fit.
>>>>
>>>>> Would it be feasiable to not use auto-completing jobs and instead
>>>>> advance the timeline manually? Given how the premise of the test seems
>>>>> to be about concurrent submission it sounds plausible that what happens
>>>>> after submission maybe isn't very relevant.
>>>>
>>>> Good idea! I'll run some experiments and see if it works.
>>>
>>> Cool, I will await your findings in v2. :)
>>
>>
>> After fiddling a bit with the code, I came to the conclusion that
>> changing the design to use manual timeline advancement is doable, but
>> not beneficial, since it would require serializing job submission into
>> "batches" using a two-step process, i.e., (i) workers submit jobs, and
>> (ii) the main thread waits for all submissions, advances the timeline,
>> and then releases the workers for the next iteration.
> 
> What do you mean by next iteration?
> 
> In the patch you have each worker submit all jobs in one go.

I mean, if I change the code to use manual timeline advancement, then I
must introduce some synchronization logic that makes the main thread
advance the timeline only after the workers have submitted their jobs.
Since workers submit multiple jobs, I was thinking it would be better to
have the workers submit jobs in batches instead of all in one go.

>> This approach would partially defeat the purpose of a concurrency test
>> as it would not allow job submission (KUnit process context) to overlap
>> with job completion (hrtimer callback interrupt context) that models
>> asynchronous hardware in the mock scheduler, limiting contention on the
>> DRM scheduler internal locking. Moreover, while manual advancement might
>> appear to make the test deterministic, it does not since the order in
>> which the workers are scheduled will still be non-deterministic.
> 
> Ah, so it depends what is the test actually wanting to test. In my view 
> exercising parallel submit is one thing, while interleaving submission 
> with completion is something else.
> 
> In the test as written I think there is very little of the latter. Each 
> worker submits all their jobs in one tight loop. Jobs you set to be 1ms 
> so first job completion is 1ms away from when workers are released. A 
> lot of the jobs can be submitted in 1ms and it would be interesting to 
> see exactly how much, from how many workers.
> 
> If desire is to interleave completion and submission I think that should 
> be made more explicit (less depending on how fast is the underlying 
> machine). For example adding delays into the submit loop to actually 
> guarantee that.

Fair point.

> But I would also recommend parallel submit and parallel submit vs 
> completions are tested in separate test cases. It should be easy to do 
> with some flags and almost no new code. I was suggesting flags for some 
> other thing in the initial review as well. Right, for auto-complete. So 
> flag could be something like:
> 
> +static const struct drm_sched_concurrent_params 
> drm_sched_concurrent_cases[] = {
> +	{
> +		.description = "Concurrently submit a single job in a single entity",
> +		.job_base_us = 1000,
> +		.num_jobs = 1,
> +		.num_subs = 32,
> 		.flags = INTERLEAVE_SUBMIT_AND_COMPLETE,
> +	},
> 
> In the submit loop:
> 
> +	for (i = 0; i < params->num_jobs; i++) {
> +		duration_us = params->job_base_us + (sub_data->id * 10);
> 		if (flags & INTERLEAVE_SUBMIT_AND_COMPLETE) {
> 			drm_mock_sched_job_set_duration_us(sub_data->jobs[i], duration_us);
> 			// Add a delay based on time elapse and job duration to guarantee job 
> completions start arriving
> 		}
> +		drm_mock_sched_job_submit(sub_data->jobs[i]);
> +	}
> 
> 
> And of course handle the job waiting stage appropriately depending on 
> auto-complete or not.
> 
> Anyway, testing as little as possible at a time is a best practice I 
> recommend, but if you insist, there is also nothing fundamentally wrong 
> with the test as you have it so I won't block it.

Agreed. Unit tests should test one functionality at a time and be clear
about which one. I'll follow your suggestions and have two separate test
cases: a basic one for concurrent submissions with manual timeline
advancement (no batches, workers submit all jobs at once) and then a
second one with automatic timeline advancement for testing interleaving
submissions with completions.

At this point though, I think it would be better to move the tests to a
separate source file, as the partial similarity of the first concurrent
test case with drm_sched_basic_submit could create some confusion.

Thanks,
Marco

Powered by blists - more mailing lists