Re: [PATCH 1/8] dma-buf: protected fence ops by RCU v5

["Christian König" <christian.koenig@amd.com>]

On 2/12/26 09:56, Philipp Stanner wrote:
>>>> @@ -454,13 +465,19 @@ dma_fence_test_signaled_flag(struct dma_fence *fence)
>>>>  static inline bool
>>>>  dma_fence_is_signaled_locked(struct dma_fence *fence)
>>>>  {
>>>> +	const struct dma_fence_ops *ops;
>>>> +
>>>>  	if (dma_fence_test_signaled_flag(fence))
>>>>  		return true;
>>>>  
>>>> -	if (fence->ops->signaled && fence->ops->signaled(fence)) {
>>>> +	rcu_read_lock();
>>>> +	ops = rcu_dereference(fence->ops);
>>>> +	if (ops->signaled && ops->signaled(fence)) {
>>>
>>> Maybe you can educate me a bit about RCU here – couldn't this still
>>> race? If the ops were unloaded before you take rcu_read_lock(),
>>> rcu_dereference() would give you an invalid pointer here since you
>>> don't check for !ops, no?
>>
>> Perfectly correct thinking, yes.
>>
>> But the check for !ops is added in patch #2 when we actually start to set ops = NULL when the fence signals.
>>
>> I intentionally separated that because it is basically the second step in making the solution to detach the fence ops from the module by RCU work.
>>
>> We could merge the two patches together, but I think the separation actually makes sense should anybody start to complain about the additional RCU overhead.
>>
> 
> Alright, makes sense. However the above does not read correct..
> 
> But then my question would be: What's the purpose of this patch, what
> does it solve or address atomically?

Adding the RCU annotation and related logic, e.g. rcu_read_lock()/rcu_read_unlock()/rcu_dereference() etc...

This allows the automated statically RCU checker to validate what we do here and point out potential mistakes.

Additional to that should adding the rcu_read_lock() protection cause performance problems it will bisect to this patch here alone.

> Adding RCU here does not yet change behavior and it does not solve the
> unloading problem, does it?

Nope, no functional behavior change. It's purely to get the automated checkers going.

> If it's a mere preperational step and the patches should not be merged,
> I'd guard the above with a simple comment like "Cleanup preparation.
> 'ops' can yet not be NULL, but this will be the case subsequently."

A comment added in this patch and removed in the next one? Na, that sounds like overkill to me.

Christian.

> 
> 
> P.
>