Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider: - [High] samples: rust: DriverFence drop order in init() sleeps under spinlock - [High] samples: rust: premature job completion and seqno 0 leak queue capacity - [High] rust: dma_fence: DriverFence::drop() drops NonNull instead of payload - [Medium] samples: rust: broken Kconfig select and missing DRM=y dependency - [Medium] samples: rust: vacuous blocked-job checks and late job4/5 submit in init() - [Medium] rust: drm: DependencyWaker::on_signal() runs jobs after dependency error - [Low] samples: rust: missing Signed-off-by and wrong module description --
commit 8d8d7d10602aa63f4a1f88e3bb899d199cf28755 Author: Gary Guo <[email protected]> samples: rust: Add JobQueue Tester This commit adds a sample Rust module to test drm::JobQueue job submission, dependency handling, and completion. > samples: rust: Add JobQueue Tester > > Some test code for the JobQueue [Severity: Low] This isn't a bug, but is the commit message missing a Signed-off-by tag? > diff --git a/samples/rust/Kconfig b/samples/rust/Kconfig > index 31d62533ef25f..a5754fe03fd5c 100644 > --- a/samples/rust/Kconfig > +++ b/samples/rust/Kconfig > @@ -190,4 +190,15 @@ config SAMPLE_RUST_HOSTPROGS > > If unsure, say N. > > +config SAMPLE_RUST_JOBQUEUE_TESTER > + tristate "Jobqueue Tester" > + select JOBQUEUE_TESTER [Severity: Medium] Does this Kconfig entry cause a Kconfig warning and build failure when CONFIG_DRM is disabled or built as a module? No Kconfig symbol named JOBQUEUE_TESTER exists in the tree, so selecting it triggers an undefined symbol warning. In addition, samples/rust/rust_jobqueue_tester.rs unconditionally imports kernel::dma_buf and kernel::drm, which are gated in rust/kernel/lib.rs: #[cfg(CONFIG_DMA_SHARED_BUFFER)] pub mod dma_buf; pub mod driver; #[cfg(CONFIG_DRM = "y")] pub mod drm; Should SAMPLE_RUST_JOBQUEUE_TESTER drop select JOBQUEUE_TESTER and add depends on DRM=y? Furthermore, when CONFIG_DRM=y is enabled, does rust/kernel/drm/job_queue.rs compile cleanly? It currently imports new_scoped_work, ScopedWork, ScopedWorkItem, and ScopedWorkRef and calls Queue::enqueue_scoped(), none of which exist in rust/kernel/workqueue.rs, and implements FenceCallback (which requires Send + 'static in rust/kernel/dma_buf/dma_fence.rs) for non-'static DependencyWaker. [ ... ] > diff --git a/samples/rust/rust_jobqueue_tester.rs > b/samples/rust/rust_jobqueue_tester.rs > new file mode 100644 > index 0000000000000..48dd1188f7dbe > --- /dev/null > +++ b/samples/rust/rust_jobqueue_tester.rs > @@ -0,0 +1,242 @@ > +// SPDX-License-Identifier: GPL-2.0 > + > +//! Small example demonstrating how to use [`drm::JobQueue`]. > + > +use kernel::dma_buf::{FenceContext, FenceContextOps, Fence, FenceStatus}; > +use kernel::drm::{JobQueue, JobQueueOps, JobRunnable}; > +use kernel::prelude::*; > +use kernel::str::CString; > +use kernel::task::might_sleep; > +use kernel::time::{delay::fsleep, Delta}; > + > +module! { > + type: RustJobqueueTester, > + name: "rust_jobqueue_tester", > + authors: ["Philipp Stanner"], > + description: "Rust minimal sample", > + license: "GPL", > +} [Severity: Low] This isn't a bug, but was the description "Rust minimal sample" in module! copied from samples/rust/rust_minimal.rs instead of describing the JobQueue tester? [ ... ] > +struct JobPayload {} > + > +impl Drop for JobPayload { > + fn drop(&mut self) { > + might_sleep(); > + } > +} [Severity: High] Does JobPayload::drop() ever get invoked when jobs and their fences are dropped? JobQueue stores T::JobDataType (JobPayload) as the FenceDataType of its internal FenceContext. In rust/kernel/dma_buf/dma_fence.rs, DriverFence holds data: NonNull<DriverFenceData<'a, T>>, and DriverFence::drop() currently runs: // SAFETY: Valid because `self` is valid. unsafe { drop_in_place(&raw mut self.data) }; Because self.data is a NonNull pointer wrapper with no Drop implementation, drop_in_place(&raw mut self.data) is a no-op instead of dropping (*self.data.as_ptr()).data. As a result, T::FenceDataType is leaked and the sleepable drop test in JobPayload::drop() is bypassed. > + > +impl JobQueueOps for JqData { > + type JobDataType = JobPayload; > + > + fn run_job<'a>(&self, job: JobRunnable<'a, JobPayload>) { > + // Simulate GPU work completing after a short delay. In a real driver > + // this would submit work to the hardware ring. > + pr_info!("run_job: seqno {}\n", job.seqno); > + } [ ... ] > + let driver_name = > CString::try_from_fmt(fmt!("rust-jobqueue-tester"))?; > + let timeline_name = > CString::try_from_fmt(fmt!("rust-jq-tester-hw-timeline"))?; > + let jq = KBox::pin_init( > + JobQueue::new(0, driver_name, timeline_name, 1_000_000, > JqData::new()), > + GFP_KERNEL, > + )?; > + > + pr_info!("Test 1: Test submitting two jobs without dependencies.\n"); > + let job1 = jq.new_job(1, JobPayload {})?; > + let job2 = jq.new_job(1, JobPayload {})?; > + > + let fence1 = job1.submit(); > + let fence2 = job2.submit(); > + > + // Complete both jobs by seqno so their fences get signaled. > + jq.complete_jobs_up_to_seqno(fence2.seqno()); > + > + // Give the submit worker a chance to run. > + fsleep(Delta::from_millis(100)); [Severity: High] Does passing initial_seqno = 0 to JobQueue::new() and calling complete_jobs_up_to_seqno() before sleeping cause premature job completion and leak queue capacity? First, in rust/kernel/drm/job_queue.rs, JobQueue::new() initializes JobQueueInner::last_completed to 0: inner <- new_spinlock_irq!(try_pin_init!(JobQueueInner { capacity, last_completed: 0, waiting_jobs: UniqueList::new(), running_jobs: UniqueList::new(), drop_list: UniqueList::new(), })), When initial_seqno is 0, FenceContext::next_seqno() assigns seqno 0 to the first submitted job, which immediately satisfies jq.last_completed >= seqno (0 >= 0) in SubmitWorker::run() even before complete_jobs_up_to_seqno() is called. Second, in Test 1, Test 2, and Test 5 (phases 1 and 4), jq.complete_jobs_up_to_seqno() is called synchronously right after job.submit() and before fsleep(). Because SubmitWorker runs asynchronously on a workqueue, the jobs are still in waiting_jobs rather than running_jobs when complete_jobs_up_to_seqno() executes, so complete_jobs_up_to_seqno() only updates last_completed = seqno while running_jobs and drop_list are empty. When SubmitWorker::run() later processes those jobs, it subtracts job.cost from jq.capacity and takes the jq.last_completed >= seqno branch: if jq.last_completed >= seqno { fence.signal(Ok(())); // Drop here directly if possible. Otherwise, let the DropWorker // take care of it. if core::mem::needs_drop::<T::JobDataType>() { jq.drop_list.push_back(job); } } else { jq.running_jobs.push_back(job); } Unlike complete_jobs_up_to_seqno(), that branch never restores jq.capacity += u64::from(job.cost), permanently leaking capacity credits, and never calls check_start_drop_worker() after pushing onto jq.drop_list. [ ... ] > + pr_info!("Test 3: Test that a job with unfulfilled dependency does > not get run.\n"); > + let unsignaled_alloc = dep_fctx.new_fence_allocation(())?; > + let unsignaled_fence = unsignaled_alloc.new_fence(); > + > + let mut job4 = jq.new_job(1, JobPayload {})?; > + job4.add_dependency(unsignaled_fence.as_fence())?; > + > + let blocked_job_fence = job4.submit(); > + > + // Give the submit worker a chance to run (it should NOT run job4). > + fsleep(Delta::from_millis(100)); > + > + if is_signaled(&blocked_job_fence) { > + pr_info!("Test 3 failed.\n"); > + return Err(EAGAIN); > + } [Severity: High] If RustJobqueueTester::init() returns an error here in Test 3 or later in Test 5 while an unsignaled dependency DriverFence has a registered callback, will stack unwinding sleep while holding a spinlock with interrupts disabled? Because jq is declared before unsignaled_fence in Test 3 (and before job3 and unsignaled_fence1, unsignaled_fence2, and unsignaled_fence3 in Test 5), Rust drops the unsignaled DriverFence locals before job3 and jq on any early error return. When an unsignaled DriverFence is dropped, DriverFence::drop() in rust/kernel/dma_buf/dma_fence.rs acquires dma_fence_lock_irqsave(), sets ECANCELED, and calls dma_fence_signal_locked(). That synchronously invokes DependencyWaker::on_signal() in rust/kernel/drm/job_queue.rs with Err(ECANCELED): fn on_signal(&mut self, res: Result) { if let Err(err) = res { // Dependencies are unfulfilled and will never be fulfilled. Halt the // submit_worker and tell the driver to tear this JQ down. self.jq.revoked.store(true, Relaxed); self.jq.srcu.synchronize(); Calling self.jq.srcu.synchronize() (synchronize_srcu()) while holding dma_fence_lock_irqsave sleeps in atomic context. [Severity: Medium] Also in DependencyWaker::on_signal(), when res is Err(err), execution does not return after calling T::kill() and instead falls through to: // Last dependency kicks the worker. if self.nr_of_deps.fetch_sub(1, Relaxed) == 1 { // The drop-order of JobQueue ensures that all DependencyWakers are // cancelled before the JobQueue itself is dropped. self.jq.check_start_submit_worker(); } In SubmitWorker::run(), work.revoked.load(Relaxed) is only checked once before the while let Some(job) = jq.as_mut().project().waiting_jobs.pop_front() loop, and is not re-checked after retaking work.inner.lock() across T::run_job(). Could a failed dependency decrement nr_of_deps to 0 and allow SubmitWorker to run a job whose dependency errored? [ ... ] > + pr_info!("Test 5: Submit unblocked jobs, then a blocked one, then > unblocked ones.\n"); > + let job1 = jq.new_job(1, JobPayload {})?; > + let job2 = jq.new_job(1, JobPayload {})?; > + let mut job3 = jq.new_job(1, JobPayload {})?; > + let job4 = jq.new_job(1, JobPayload {})?; > + let job5 = jq.new_job(1, JobPayload {})?; > + > + let unsignaled_alloc1 = dep_fctx.new_fence_allocation(())?; > + let unsignaled_fence1 = unsignaled_alloc1.new_fence(); > + let unsignaled_alloc2 = dep_fctx.new_fence_allocation(())?; > + let unsignaled_fence2 = unsignaled_alloc2.new_fence(); > + let unsignaled_alloc3 = dep_fctx.new_fence_allocation(())?; > + let unsignaled_fence3 = unsignaled_alloc3.new_fence(); > + > + job3.add_dependency(unsignaled_fence1.as_fence())?; > + job3.add_dependency(unsignaled_fence2.as_fence())?; > + job3.add_dependency(unsignaled_fence3.as_fence())?; > + > + let fence1 = job1.submit(); > + let fence2 = job2.submit(); > + let fence3 = job3.submit(); > + > + // Complete unblocked jobs 1 and 2. > + jq.complete_jobs_up_to_seqno(fence2.seqno()); > + fsleep(Delta::from_millis(100)); > + > + if is_signaled(&fence3) || !is_signaled(&fence1) || > !is_signaled(&fence2) { > + pr_info!("Test 5 failed (phase 1).\n"); > + return Err(EAGAIN); > + } > + > + // Signal two of three dependencies — job3 should still be blocked. > + unsignaled_fence1.signal(Ok(())); > + unsignaled_fence3.signal(Ok(())); > + fsleep(Delta::from_millis(200)); > + > + if is_signaled(&fence3) { > + pr_info!("Test 5 failed (phase 2).\n"); > + return Err(EAGAIN); > + } > + > + // Signal the last dependency — job3 should now be runnable. > + unsignaled_fence2.signal(Ok(())); > + fsleep(Delta::from_millis(500)); > + > + // Complete job3 and the remaining jobs. > + jq.complete_jobs_up_to_seqno(fence3.seqno()); > + fsleep(Delta::from_millis(100)); > + > + if !is_signaled(&fence3) { > + pr_info!("Test 5 failed (phase 3).\n"); > + return Err(EAGAIN); > + } > + > + let fence4 = job4.submit(); > + let fence5 = job5.submit(); [Severity: Medium] Are the blocked-job checks in Test 3 and Test 5 (phases 1 and 2) able to detect if a blocked job is run prematurely, and should job4 and job5 be submitted before job3 is unblocked? In SubmitWorker::run(), running a job calls JqData::run_job() and moves the job to running_jobs without signaling the job fence unless jq.last_completed >= seqno. Because JqData::run_job() only prints a message and complete_jobs_up_to_seqno() has not been called for blocked_job_fence in Test 3 or fence3 in Test 5 phases 1 and 2, is_signaled(&blocked_job_fence) and is_signaled(&fence3) will always return false even if SubmitWorker ignored dependencies and called run_job() immediately. Additionally, Test 5 states "Submit unblocked jobs, then a blocked one, then unblocked ones" and creates job1 through job5 up front, with the phase 3 comment saying "Complete job3 and the remaining jobs." However, job4.submit() and job5.submit() are only called here in phase 4 after job3 has already been unblocked and completed, so job4 and job5 are never queued behind the blocked job3. -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=8
