Claude review: drm/sched: Add test suite for concurrent job submissions

[Claude Code Review Bot <claude-review@example.com>]

Patch Review

**Cleanup pattern diverges from existing code**

The existing suites use a simple `init`/`exit` pair with direct calls to `drm_mock_sched_fini()` and `drm_mock_sched_entity_free()`. This patch introduces `KUNIT_DEFINE_ACTION_WRAPPER` and `kunit_add_action_or_reset` for the first time in this file. While the action-based cleanup is appropriate for the concurrent case (where early assertion failures in the test body need safe unwinding), it's worth noting this creates two different cleanup idioms in the same file. Not a blocker, but a comment explaining *why* the action-based approach is used here would help future readers.

**`complete_destroy_workqueue` — the safety net is sound but subtle**

```c
static void complete_destroy_workqueue(void *context)
{
	struct sched_concurrent_context *ctx = context;

	complete_all(&ctx->wait_go);

	destroy_workqueue(ctx->sub_wq);
}
```

The `complete_all()` before `destroy_workqueue()` serves as a safety net: if the test aborts (via `KUNIT_ASSERT_*`) after workers are queued but before `complete_all()` is called in the test body, workers would deadlock waiting on the completion, and `destroy_workqueue()` would hang. This is correct but non-obvious. A brief comment explaining this defensive pattern would be valuable.

In the normal (non-aborting) flow, `complete_all()` is called twice — once in the test body and once in cleanup. This is harmless since `complete_all()` sets `done` to `UINT_MAX/2` idempotently, but a reader might wonder if it's a bug.

**Grammar nit in comment**

```c
	 * Init workers only after all jobs and entities have been successfully
	 * allocated. In this way, the cleanup logic for when an assertion fail
	 * can be simplified.
```

"assertion fail" → "assertion fails"

**Single parameter case for each test**

Both `drm_sched_parallel_cases[]` and `drm_sched_interleaved_cases[]` define only a single parameter set. The parameterized infrastructure is in place but currently exercises only one configuration (16 workers × 8 jobs for parallel; 16 workers, 100µs base, 8 in-flight for interleaved). Consider adding at least one more case for each — e.g., a smaller configuration (fewer workers, fewer jobs) as a quick smoke test, or a stress configuration. This would better justify the parameterized approach and improve coverage. Not a blocker for merging, but something to consider.

**Interleaved worker timeout calculation**

```c
	timeout = msecs_to_jiffies(params->test_duration_ms * 2);
```

The timeout is 2× the total test duration budget (2 seconds). Since the scheduler serializes all jobs across all workers, and the total expected execution time is approximately `test_duration_ms` (1 second), a per-job timeout of 2 seconds is generous. This seems reasonable for avoiding flaky timeouts, but the `* 2` magic number could benefit from a brief comment explaining the rationale.

**No exit function for the concurrent suite**

```c
static struct kunit_suite drm_sched_concurrent = {
	.name = "drm_sched_concurrent_tests",
	.init = drm_sched_concurrent_init,
	.test_cases = drm_sched_concurrent_tests,
};
```

The suite has no `.exit` callback, relying entirely on kunit action-based cleanup. This is consistent with the action-based approach and correct — all resources are freed via actions registered with `kunit_add_action_or_reset`. Just noting this is intentional and fine.

**Entity cleanup actions registered per-worker in the test body**

```c
	for (i = 0; i < params->num_workers; i++) {
		...
		worker->entity = drm_mock_sched_entity_new(test, ...);

		ret = kunit_add_action_or_reset(test, drm_mock_sched_entity_free_wrap,
						worker->entity);
		KUNIT_ASSERT_EQ(test, ret, 0);
```

This means up to 16 cleanup actions are registered for entities alone. Combined with the workqueue and scheduler actions from init, the cleanup stack could get long. This is fine for a test, but worth being aware of if the parameter counts grow significantly.

**`drm_mock_sched_job_is_finished` vs `drm_mock_sched_job_wait_finished` in final check**

In the interleaved test's final verification:
```c
	for (i = 0; i < params->num_workers; i++) {
		...
		for (j = 0; j < worker->job_count; j++) {
			job = worker->jobs[j];
			done = drm_mock_sched_job_is_finished(job);
			KUNIT_EXPECT_TRUE(test, done);
		}
	}
```

This uses the non-waiting `drm_mock_sched_job_is_finished()` rather than `drm_mock_sched_job_wait_finished()` (which the parallel test uses). This is correct: by the time `flush_workqueue()` returns, all workers have completed their wait loops, so all jobs should already be finished. The non-waiting check serves as a pure assertion that nothing went wrong, which is the right semantic here.

**Overall**: This is solid test code that adds meaningful coverage. The main suggestions are cosmetic — adding comments for the `complete_all` safety net, fixing the grammar nit, and considering additional parameter cases. The concurrency design (barrier start, per-worker entities, sliding window) is well thought out.

---
Generated by Claude Code Patch Reviewer