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.
>
