On 25/02/2026 07:47, Philipp Stanner wrote:
On Mon, 2026-02-23 at 16:25 +0000, Tvrtko Ursulin wrote:
On 19/02/2026 14:07, Marco Pagani wrote:
Add a new test suite to simulate concurrent job submissions to the DRM
scheduler. The suite includes two initial test cases: (i) a primary test
case for parallel job submission and (ii) a secondary test case for
interleaved job submission and completion. In the first test case, worker
threads concurrently submit jobs to the scheduler and then the timeline is
manually advanced. In the second test case, worker threads periodically
submit a sequence of jobs to the mock scheduler. Periods are chosen as
harmonic, starting from a base period, to allow interleaving between
submission and completion. To avoid impractically large execution times,
periods are cycled. The timeline is advanced automatically when the jobs
completes. This models how job submission from userspace (in process
context) may interleave with job completion (hrtimer callback interrupt
context, in the test case) by a physical device.
I still maintain the opinion expressed the last time: that the commit
message should make explicit why the patch / test is added. Which this
doesn't do. It just says: "We add X", but not "Currently, the problem
is that YZ, thus we need X".
(also breaking longer commit messages into paragraphs is nice)
Also see my comments about interleaved submits below.
I'll address the ones addressed to me.
8><
+struct drm_sched_parallel_params {
+ const char *description;
+ unsigned int num_jobs;
+ unsigned int num_workers;
+};
+
+static const struct drm_sched_parallel_params drm_sched_parallel_cases[] = {
+ {
+ .description = "Workers submitting multiple jobs against a single
entity",
Each worker has own entity so the description is a bit confusing.
Do you have a suggestion for a better one?
Along the lines of:
"Multiple workers submitting multiple jobs from their entity"
8><
+ }
+
+ for (i = 0; i < params->num_workers; i++) {
+ INIT_WORK(&workers[i].work, drm_sched_parallel_worker);
+ queue_work(ctx->sub_wq, &workers[i].work);
+ }
+
+ complete_all(&ctx->wait_go);
+ flush_workqueue(ctx->sub_wq);
+
+ for (i = 0; i < params->num_workers; i++) {
+ for (j = 0; j < params->num_jobs; j++) {
+ done =
drm_mock_sched_job_wait_scheduled(workers[i].jobs[j],
+ HZ);
same
+ KUNIT_ASSERT_TRUE(test, done);
+
+ done =
drm_mock_sched_job_is_finished(workers[i].jobs[j]);
+ KUNIT_ASSERT_FALSE(test, done);
This second assert does not seem to be bringing much value, but instead
makes the reader ask themselves why it is there. Remove it?
Hmm in fact this whole loop could be removed. All that it is needed
below is to wait for the last job to be completed.
I suppose it's being tested whether all jobs are finished. That sounds
clean and not harmful to me.
No, it is assert false. It is testing the jobs have been scheduled but
not completed before the timeline is manually advanced. Both those
behaviours are already covered by the existing basic test cases.
In my view the general best practice is to focus on the thing being
tested, which in this case is the submission side of things. The rest
can just distract the reader. And in this case that is parallel
submission, which is all done and dusted by the time flush_workqueue
above finishes. Everything after that point is just test cleanup.
So I see it as this:
/* Release parallel submit workers */
complete_all(&ctx->wait_go);
/* Wait for all of them to submit all their jobs */
flush_workqueue(ctx->sub_wq);
/* Testing done, cleanup all jobs before exiting */
completed_jobs = drm_mock_sched_advance(ctx->sched,
params->num_workers * params->num_jobs);
KUNIT_ASSERT_EQ(test, completed_jobs, params->num_workers *
params->num_jobs);
for (i = 0; i < params->num_workers; i++) {
done =
drm_mock_sched_job_wait_finished(workers[i].jobs[params->num_jobs - 1], HZ);
KUNIT_ASSERT_TRUE(test, done);
}
Regards,
Tvrtko
