Nouveau - Feb 2024 - [PATCH] nouveau: offload fence uevents work to workqueue

On Fri, Feb 09, 2024 at 06:41:32PM +0100, Danilo Krummrich
wrote:
> On 2/6/24 15:03, Daniel Vetter wrote:
> > On Mon, Feb 05, 2024 at 11:00:04PM +0100, Danilo Krummrich wrote:
> > > On 2/5/24 22:08, Dave Airlie wrote:
> > > > On Tue, 6 Feb 2024 at 02:22, Danilo Krummrich <dakr at
redhat.com> wrote:
> > > > > 
> > > > > On 1/29/24 02:50, Dave Airlie wrote:
> > > > > > From: Dave Airlie <airlied at redhat.com>
> > > > > > 
> > > > > > This should break the deadlock between the fctx
lock and the irq lock.
> > > > > > 
> > > > > > This offloads the processing off the work from the
irq into a workqueue.
> > > > > > 
> > > > > > Signed-off-by: Dave Airlie <airlied at
redhat.com>
> > > > > 
> > > > > Nouveau's scheduler uses a dedicated wq, hence from
this perspective it's
> > > > > safe deferring fence signalling to the kernel global
wq. However, I wonder
> > > > > if we could create deadlocks by building dependency
chains into other
> > > > > drivers / kernel code that, by chance, makes use of the
kernel global wq as
> > > > > well.
> > > > > 
> > > > > Admittedly, even if, it's gonna be extremely
unlikely given that
> > > > > WQ_MAX_ACTIVE == 512. But maybe it'd be safer to
use a dedicated wq.
> > > > > 
> > > > > Also, do we need to CC stable?
> > > > 
> > > > I pushed this to Linus at the end of last week, since the
hangs in 6.7
> > > > take out the complete system and I wanted it in stable.
> > > > 
> > > > It might be safer to use a dedicated wq, is the concern
someone is
> > > > waiting on a fence in a workqueue somewhere else so we will
never
> > > > signal it?
> > > 
> > > Yes, if some other work is waiting for this fence (or something
else in the same
> > > dependency chain) to signal it can prevent executing the work
signaling this fence,
> > > in case both are scheduled on the same wq. As mentioned, with the
kernel global wq
> > > this would be rather unlikely to lead to an actual stall with
WQ_MAX_ACTIVE == 512,
> > > but formally the race condition exists. I guess a malicious
attacker could try to
> > > intentionally push jobs directly or indirectly depending on this
fence to a driver
> > > which queues them up on a scheduler using the kernel global wq.
> > 
> > I think if you add dma_fence_signalling annotations (aside,
there's some
> > patch from iirc Thomas Hellstrom to improve them and cut down on some
> > false positives, but I lost track) then I think you won't get any
splats
> > because the wq subsystem assumes that WC_MAX_ACTIVE is close enough to
> > infinity to not matter.
> 
> As mentioned, for the kernel global wq it's 512. (Intentionally)
feeding the kernel
> with enough jobs to to provoke a deadlock doesn't seem impossible to
me.
> 
> I think it'd be safer to just establish not to use the kernel global wq
for executing
> work in the fence signalling critical path.
> 
> We could also run into similar problems with a dedicated wq, e.g. when
drivers share
> a wq between drm_gpu_scheduler instances (see [1]), however, I'm not
sure we can catch
> that with lockdep.
I think if you want to fix it perfectly you'd need to set the max number
of wq to the number of engines (or for dynamic/fw scheduled engines to the
number of context) you have. Or whatever limit to the number of parallel
timelines there is.

I guess this would need a new wq function to update? drm/sched code could
be able to set that for drivers, so drivers cannot get this wrong.

If we don't do something like that then I'm not sure there's really
much
benefit - instead of carefully timing 512 dma_fence on the global wq you
just need to create a pile of context (at least on intel's guc that's
absolutely no issue) and then careful time them.

I still feel like we have bigger fish to fry ... But might be worth the
effort to at least make the parallel timeline limit a lot more explicit?

Cheers, Sima
> 
> [1]
https://elixir.bootlin.com/linux/v6.8-rc3/source/drivers/gpu/drm/nouveau/nouveau_drm.c#L313
> 
> > 
> > I'm not sure we should care differently, but I guess it'd be
good to
> > annotate it all in case the wq subsystem's idea of how much such
deadlocks
> > are real changes.
> > 
> > Also Teo is on a mission to get rid of all the global wq flushes, so
there
> > should also be no source of deadlocks from that kind of cross-driver
> > dependency. Or at least shouldn't be in the future, I'm not
sure it all
> > landed.
> > -Sima
> 
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

On 2/9/24 19:52, Daniel Vetter wrote:
> On Fri, Feb 09, 2024 at 06:41:32PM +0100, Danilo Krummrich wrote:
>> On 2/6/24 15:03, Daniel Vetter wrote:
>>> On Mon, Feb 05, 2024 at 11:00:04PM +0100, Danilo Krummrich wrote:
>>>> On 2/5/24 22:08, Dave Airlie wrote:
>>>>> On Tue, 6 Feb 2024 at 02:22, Danilo Krummrich <dakr at
redhat.com> wrote:
>>>>>>
>>>>>> On 1/29/24 02:50, Dave Airlie wrote:
>>>>>>> From: Dave Airlie <airlied at redhat.com>
>>>>>>>
>>>>>>> This should break the deadlock between the fctx
lock and the irq lock.
>>>>>>>
>>>>>>> This offloads the processing off the work from the
irq into a workqueue.
>>>>>>>
>>>>>>> Signed-off-by: Dave Airlie <airlied at
redhat.com>
>>>>>>
>>>>>> Nouveau's scheduler uses a dedicated wq, hence from
this perspective it's
>>>>>> safe deferring fence signalling to the kernel global
wq. However, I wonder
>>>>>> if we could create deadlocks by building dependency
chains into other
>>>>>> drivers / kernel code that, by chance, makes use of the
kernel global wq as
>>>>>> well.
>>>>>>
>>>>>> Admittedly, even if, it's gonna be extremely
unlikely given that
>>>>>> WQ_MAX_ACTIVE == 512. But maybe it'd be safer to
use a dedicated wq.
>>>>>>
>>>>>> Also, do we need to CC stable?
>>>>>
>>>>> I pushed this to Linus at the end of last week, since the
hangs in 6.7
>>>>> take out the complete system and I wanted it in stable.
>>>>>
>>>>> It might be safer to use a dedicated wq, is the concern
someone is
>>>>> waiting on a fence in a workqueue somewhere else so we will
never
>>>>> signal it?
>>>>
>>>> Yes, if some other work is waiting for this fence (or something
else in the same
>>>> dependency chain) to signal it can prevent executing the work
signaling this fence,
>>>> in case both are scheduled on the same wq. As mentioned, with
the kernel global wq
>>>> this would be rather unlikely to lead to an actual stall with
WQ_MAX_ACTIVE == 512,
>>>> but formally the race condition exists. I guess a malicious
attacker could try to
>>>> intentionally push jobs directly or indirectly depending on
this fence to a driver
>>>> which queues them up on a scheduler using the kernel global wq.
>>>
>>> I think if you add dma_fence_signalling annotations (aside,
there's some
>>> patch from iirc Thomas Hellstrom to improve them and cut down on
some
>>> false positives, but I lost track) then I think you won't get
any splats
>>> because the wq subsystem assumes that WC_MAX_ACTIVE is close enough
to
>>> infinity to not matter.
>>
>> As mentioned, for the kernel global wq it's 512. (Intentionally)
feeding the kernel
>> with enough jobs to to provoke a deadlock doesn't seem impossible
to me.
>>
>> I think it'd be safer to just establish not to use the kernel
global wq for executing
>> work in the fence signalling critical path.
>>
>> We could also run into similar problems with a dedicated wq, e.g. when
drivers share
>> a wq between drm_gpu_scheduler instances (see [1]), however, I'm
not sure we can catch
>> that with lockdep.
> 
> I think if you want to fix it perfectly you'd need to set the max
number
> of wq to the number of engines (or for dynamic/fw scheduled engines to the
> number of context) you have. Or whatever limit to the number of parallel
> timelines there is.> 
> I guess this would need a new wq function to update? drm/sched code could
> be able to set that for drivers, so drivers cannot get this wrong.
Not sure I can follow. The scheduler instance might be per context and bind
queue. In this case it gets the shared wq passed, but doesn't know how many
other scheduler instances share the same one.

Additionally, there might be drivers not using the DRM scheduler for for bind
queues at all (I think Xe does not).
> 
> If we don't do something like that then I'm not sure there's
really much
> benefit - instead of carefully timing 512 dma_fence on the global wq you
> just need to create a pile of context (at least on intel's guc
that's
> absolutely no issue) and then careful time them.
Well, that's true. I'd still argue that there is a slight difference.
From a
drivers isolated perspective using the global kernel wq might be entirely fine,
as in this patch. However, in combination with another driver doing the same
thing, things can blow up. For the case you illustrated it's at least
possible
to spot it from a driver's perspective.
> 
> I still feel like we have bigger fish to fry ... But might be worth the
> effort to at least make the parallel timeline limit a lot more explicit?
I agree, and it'd be great if we can find a solution such bugs can be
detected
systematically (e.g. through lockdep), but maybe we can start to at least
document that we should never use the kernel global wq and where we need to be
careful in sharing driver wqs.

- Danilo
> 
> Cheers, Sima
> 
>>
>> [1]
https://elixir.bootlin.com/linux/v6.8-rc3/source/drivers/gpu/drm/nouveau/nouveau_drm.c#L313
>>
>>>
>>> I'm not sure we should care differently, but I guess it'd
be good to
>>> annotate it all in case the wq subsystem's idea of how much
such deadlocks
>>> are real changes.
>>>
>>> Also Teo is on a mission to get rid of all the global wq flushes,
so there
>>> should also be no source of deadlocks from that kind of
cross-driver
>>> dependency. Or at least shouldn't be in the future, I'm not
sure it all
>>> landed.
>>> -Sima
>>
>