On Thu, 2026-02-19 at 11:23 +0100, Christian König wrote:
> 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.
Alright, thx for the info. Very useful
>
> > 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.
ACK.
But then lets do a normalkill by adding the info you provided above
into the commit message, shall we? ^_^
"At first glance it is counter intuitive to protect a constant function
pointer table by RCU, but this allows modules providing the function
table to unload by waiting for an RCU grace period."
This doesn't reveal what the patch is actually about, just that
something is counter-intuitive to someone already very familiar with
the series' intent and the code's deeper background :)
"This or that about dma_fence shall be cleaned up in subsequent
patches. To prepare for that, add … which allows the RCU checker to
validate …"
*Philipp reads that*: ["Ah, this patch is in preparation and allows the
RCU checker to validate everything!"]
;p
P.
