Nouveau - Apr 2025 - [PATCH 1/2] dma-fence: Rename dma_fence_is_signaled()

Hi Philipp,

On Wed,  9 Apr 2025 14:06:37 +0200
Philipp Stanner <phasta at kernel.org> wrote:
> dma_fence_is_signaled()'s name strongly reads as if this function were
> intended for checking whether a fence is already signaled. Also the
> boolean it returns hints at that.
> 
> The function's behavior, however, is more complex: it can check with a
> driver callback whether the hardware's sequence number indicates that
> the fence can already be treated as signaled, although the hardware's /
> driver's interrupt handler has not signaled it yet. If that's the
case,
> the function also signals the fence.
> 
> (Presumably) this has caused a bug in Nouveau (unknown commit), where
> nouveau_fence_done() uses the function to check a fence, which causes a
> race.
> 
> Give the function a more obvious name.
This is just my personal view on this, but I find the new name just as
confusing as the old one. It sounds like something is checked, but it's
clear what, and then the fence is forcibly signaled like it would be if
you call drm_fence_signal(). Of course, this clarified by the doc, but
given the goal was to make the function name clearly reflect what it
does, I'm not convinced it's significantly better.

Maybe dma_fence_check_hw_state_and_propagate(), though it might be
too long of name. Oh well, feel free to ignore this comments if a
majority is fine with the new name.

Regards,

Boris

On Wed, 2025-04-09 at 14:39 +0200, Boris Brezillon
wrote:
> Hi Philipp,
> 
> On Wed,? 9 Apr 2025 14:06:37 +0200
> Philipp Stanner <phasta at kernel.org> wrote:
> 
> > dma_fence_is_signaled()'s name strongly reads as if this function
> > were
> > intended for checking whether a fence is already signaled. Also the
> > boolean it returns hints at that.
> > 
> > The function's behavior, however, is more complex: it can check
> > with a
> > driver callback whether the hardware's sequence number indicates
> > that
> > the fence can already be treated as signaled, although the
> > hardware's /
> > driver's interrupt handler has not signaled it yet. If that's
the
> > case,
> > the function also signals the fence.
> > 
> > (Presumably) this has caused a bug in Nouveau (unknown commit),
> > where
> > nouveau_fence_done() uses the function to check a fence, which
> > causes a
> > race.
> > 
> > Give the function a more obvious name.
> 
> This is just my personal view on this, but I find the new name just
> as
> confusing as the old one. It sounds like something is checked, but
> it's
> clear what, and then the fence is forcibly signaled like it would be
> if
> you call drm_fence_signal(). Of course, this clarified by the doc,
> but
> given the goal was to make the function name clearly reflect what it
> does, I'm not convinced it's significantly better.
> 
> Maybe dma_fence_check_hw_state_and_propagate(), though it might be
> too long of name. Oh well, feel free to ignore this comments if a
> majority is fine with the new name.
Yoa, the name isn't perfect (the perfect name describing the whole
behavior would be
dma_fence_check_if_already_signaled_then_check_hardware_state_and_propa
gate() ^^'

My intention here is to have the reader realize "watch out, the fence
might get signaled here!", which is probably the most important event
regarding fences, which can race, invoke the callbacks and so on.

For details readers will then check the documentation.

But I'm of course open to see if there's a majority for this or that
name.

P.

> 
> Regards,
> 
> Boris

On Wed, 2025-04-09 at 14:51 +0200, Philipp Stanner
wrote:
> On Wed, 2025-04-09 at 14:39 +0200, Boris Brezillon wrote:
> > Hi Philipp,
> > 
> > On Wed,? 9 Apr 2025 14:06:37 +0200
> > Philipp Stanner <phasta at kernel.org> wrote:
> > 
> > > dma_fence_is_signaled()'s name strongly reads as if this
function
> > > were
> > > intended for checking whether a fence is already signaled. Also
> > > the
> > > boolean it returns hints at that.
> > > 
> > > The function's behavior, however, is more complex: it can
check
> > > with a
> > > driver callback whether the hardware's sequence number
indicates
> > > that
> > > the fence can already be treated as signaled, although the
> > > hardware's /
> > > driver's interrupt handler has not signaled it yet. If
that's the
> > > case,
> > > the function also signals the fence.
> > > 
> > > (Presumably) this has caused a bug in Nouveau (unknown commit),
> > > where
> > > nouveau_fence_done() uses the function to check a fence, which
> > > causes a
> > > race.
> > > 
> > > Give the function a more obvious name.
> > 
> > This is just my personal view on this, but I find the new name just
> > as
> > confusing as the old one. It sounds like something is checked, but
> > it's
> > clear what, and then the fence is forcibly signaled like it would
> > be
> > if
> > you call drm_fence_signal(). Of course, this clarified by the doc,
> > but
> > given the goal was to make the function name clearly reflect what
> > it
> > does, I'm not convinced it's significantly better.
> > 
> > Maybe dma_fence_check_hw_state_and_propagate(), though it might be
> > too long of name. Oh well, feel free to ignore this comments if a
> > majority is fine with the new name.
> 
> Yoa, the name isn't perfect (the perfect name describing the whole
> behavior would be
> dma_fence_check_if_already_signaled_then_check_hardware_state_and_pro
> pa
> gate() ^^'
> 
> My intention here is to have the reader realize "watch out, the fence
> might get signaled here!", which is probably the most important event
> regarding fences, which can race, invoke the callbacks and so on.
> 
> For details readers will then check the documentation.
> 
> But I'm of course open to see if there's a majority for this or
that
> name.
how about:

dma_fence_check_hw_and_signal() ?

P.
> 
> P.
> 
> 
> > 
> > Regards,
> > 
> > Boris
>