The C dma_fence contract demands that a fence's data disappears no earlier than 1 RCU grace period after the DriverFence was signaled.
So far, we achieved this with dropping the DriverFence's data in a deferred manner with a manual, raw binding call to call_rcu(). However, this call_rcu() does not solve one problem: It is conceivable that some driver data must not drop in atomic context; but call_rcu()'s payload *can* be executed in atomic context. Moreover, the current developments in drm::JobQueue, where a DriverFence is always coupled 1:1 with a Job, allows for dropping job and fence through a work item. The undesirable blocking behavior of synchronize_rcu() would, thus, not be annoying any thread anymore. The direct users of DmaFence could achieve the same by dropping their DriverFences through work items. The only conceivable other solution, queue_rcu_work(), would, for many users, schedule a work_item from another work_item, which seems undesirable. Additionally, using the existing synchronize_rcu() abstraction has the advantage of us getting rid of a raw bindings call. Replace DriverFence::drop()'s call_rcu() with synchronize_rcu(). Signed-off-by: Philipp Stanner <[email protected]> --- rust/kernel/dma_buf/dma_fence.rs | 71 +++++++------------------------- 1 file changed, 14 insertions(+), 57 deletions(-) diff --git a/rust/kernel/dma_buf/dma_fence.rs b/rust/kernel/dma_buf/dma_fence.rs index 4e9b4a6a9471..cefcde80bf09 100644 --- a/rust/kernel/dma_buf/dma_fence.rs +++ b/rust/kernel/dma_buf/dma_fence.rs @@ -42,7 +42,8 @@ Atomic, Relaxed, // }, - rcu::rcu_barrier, // + rcu::rcu_barrier, + rcu::synchronize_rcu, // }, // }; @@ -150,7 +151,6 @@ pub fn new_fence_allocation( data: T::FenceDataType, ) -> Result<DriverFenceAllocation<'_, T>> { let fence_data = DriverFenceData { - rcu_head: Default::default(), // `inner` remains uninitialized until a `DriverFence` takes over. inner: Fence { inner: Opaque::uninit(), @@ -640,8 +640,6 @@ struct DriverFenceData<'a, T: Send + Sync + FenceContextOps> { // necessary so that the C backend can free the allocation (coming from our // Rust code) with kfree_rcu(). inner: Fence, - /// Callback head for dropping this in a deferred manner through RCU. - rcu_head: bindings::callback_head, /// Reference to access the FenceContext. fctx: &'a FenceContext<T>, /// The API user's data. It is essential that the data only performs @@ -1022,59 +1020,18 @@ fn drop(&mut self) { return; } - // SAFETY: Valid because `self` is valid. - let rcu_head_ptr = unsafe { &raw mut (*self.data.as_ptr()).rcu_head }; + // Make sure none of the fence backend_ops can access data anymore. + // + // TODO: + // This would not be necessary if the C dma_fence backend were using a + // spinlock to properly synchronize its signaled state. Fix it in C and + // then remove synchronize_rcu(). + synchronize_rcu(); - // SAFETY: `call_rcu()` is always safe to be called. `rcu_head_ptr` was - // created validly above. The module must perform a `synchronize_rcu()` - // or `rcu_barrier()` call to guard against module unload. - unsafe { bindings::call_rcu(rcu_head_ptr, Some(drop_driver_fence_data::<T>)) }; + // SAFETY: Valid because `self` is valid. + unsafe { drop_in_place(&raw mut self.data) }; + + // SAFETY: The `synchronize_rcu()` above ensures all accessors are gone. + unsafe { bindings::dma_fence_put(self.as_raw()) }; } } - -// TODO: -// The entire call_rcu() mechanism in the drop above and the code below would be -// unnecessary if C's dma_fence_signal() could be reworked in a way that after it -// ran, the caller knows that no fence_ops callbacks can be running anymore. -// In other words, if the dma_fence backend would use its spinlock for full -// synchronization. -// -// Then we could move the drop_in_place() and dma_fence_put() upwards into the -// drop() implementation and call it a day. - -/// Finally really drop this `DriverFence<T>` -/// -/// # Safety -/// -/// `head` references the `rcu_head` field of an `DriverFenceData<T>`. All -/// accessors to that `DriverFenceData<T>` must be gone by now. This must be -/// ensured by signalling the associated `DriverFence<T>` and then waiting -/// for a grace period until calling this function here. -unsafe extern "C" fn drop_driver_fence_data<T: Send + Sync + FenceContextOps>( - head: *mut bindings::callback_head, -) { - // SAFETY: Caller provides a pointer to the `rcu_head` field of a `DriverFenceData<C>`. - let fence_data = unsafe { container_of!(head, DriverFenceData<'_, T>, rcu_head) }; - - // SAFETY: `fence_data` was created validly above. All the fence's data will - // only drop below, but the raw pointer to the raw C `dma_fence` remains - // valid because the reference count is only decremented at the end of the - // function. - let fence = unsafe { (*fence_data).inner.inner.get() }; - - // SAFETY: `fence_data` was created validly above. The user has already - // dropped the only conventional accessor to the user data, the `DriverFence`, - // one grace period ago. All accessors are gone now. - unsafe { drop_in_place(&raw mut (*fence_data).data) }; - - // The inner `Fence` explicitly does not get dropped because there may be - // many more users / consumers, each holding their own reference. - - // SAFETY: Once a `DriverFence` is initialized, the inner `fence` is valid - // and initialized. It is valid until the refcount drops to 0, which can - // earliest happen once we drop the `DriverFence`'s reference here. - unsafe { bindings::dma_fence_put(fence) }; - - // The actual memory the data associated with a `DriverFence` lives in - // gets freed by the C dma_fence backend once the fence's refcount reaches 0. -} -- 2.55.0
