Re: [Intel-gfx] How to convert drivers/gpu/drm/i915/ to use local workqueue?

[Jani Nikula <jani.nikula@linux.intel.com>]

On Thu, 30 Jun 2022, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> On Thu, Jun 30, 2022 at 02:09:02PM +0100, Tvrtko Ursulin wrote:
>> 
>> On 30/06/2022 12:19, Tetsuo Handa wrote:
>> > On 2022/06/30 19:17, Tvrtko Ursulin wrote:
>> > > Could you give more context on reasoning here please? What is the difference
>> > > between using the system_wq and flushing it from a random context? Or concern
>> > > is about flushing from specific contexts?
>> > 
>> > Excuse me, but I couldn't catch what you want. Thus, I show three patterns of
>> > problems with an example.
>> > 
>> > Commit c4f135d643823a86 ("workqueue: Wrap flush_workqueue() using a macro") says:
>> > 
>> >    Tejun Heo commented that it makes no sense at all to call flush_workqueue()
>> >    on the shared WQs as the caller has no idea what it's gonna end up waiting for.
>> > 
>> >    The "Think twice before calling this function! It's very easy to get into trouble
>> >    if you don't take great care." warning message does not help avoiding problems.
>> > 
>> >    Let's change the direction to that developers had better use their local WQs
>> >    if flush_scheduled_work()/flush_workqueue(system_*_wq) is inevitable.
>> > 
>> > Three patterns of problems are:
>> > 
>> >    (a) Flushing from specific contexts (e.g. GFP_NOFS/GFP_NOIO) can cause deadlock
>> >        (circular locking dependency) problem.
>> > 
>> >    (b) Flushing with specific locks (e.g. module_mutex) held can cause deadlock
>> >        (circular locking dependency) problem.
>> > 
>> >    (c) Even if there is no possibility of deadlock, flushing with specific locks
>> >        held can cause khungtaskd to complain.
>> > 
>> > An example of (a):
>> > 
>> >    ext4 filesystem called flush_scheduled_work(), which meant to wait for only
>> >    work item scheduled by ext4 filesystem, tried to also wait for work item
>> >    scheduled by 9p filesystem.
>> >    https://syzkaller.appspot.com/bug?extid=bde0f89deacca7c765b8
>> > 
>> >    Fixed by reverting the problematic patch.
>> > 
>> > An example of (b):
>> > 
>> >    It is GFP_KERNEL context when module's __exit function is called. But whether
>> >    flush_workqueue() is called from restricted context depends on what locks does
>> >    the module's __exit function hold.
>> > 
>> >    If request_module() is called from some work function using one of system-wide WQs,
>> >    and flush_workqueue() is called on that WQ from module's __exit function, the kernel
>> >    might deadlock on module_mutex lock. Making sure that flush_workqueue() is not called
>> >    on system-wide WQs is the safer choice.
>> > 
>> >    Commit 1b3ce51dde365296 ("Input: psmouse-smbus - avoid flush_scheduled_work() usage")
>> >    is for drivers/input/mouse/psmouse-smbus.c .
>> > 
>> > An example of (c):
>> > 
>> >    ath9k driver calls schedule_work() via request_firmware_nowait().
>> >    https://syzkaller.appspot.com/bug?id=78a242c8f1f4d15752c8ef4efc22974e2c52c833
>> > 
>> >    ath6kl driver calls flush_scheduled_work() which needlessly waits for completion
>> >    of works scheduled by ath9k driver (i.e. loading firmware used by ath9k driver).
>> >    https://syzkaller.appspot.com/bug?id=10a1cba59c42d11e12f897644341156eac9bb7ee
>> > 
>> >    Commit 4b2b8e748467985c ("ath6kl: avoid flush_scheduled_work() usage") in linux-next.git
>> >    might be able to mitigate these problems. (Waiting for next merge window...)
>> 
>> Okay, from 1b3ce51dde365296:
>> 
>>  "Flushing system-wide workqueues is dangerous and will be forbidden."
>> 
>> Thank you, this exactly explains the motivation which is what I was after. I
>> certainly agree there is a possibility for lock coupling via the shared wq
>> so that is fine by me.
>> 
>> > > On the i915 specifics, the caller in drivers/gpu/drm/i915/gt/selftest_execlists.c
>> > > I am pretty sure can be removed. It is synchronized with the error capture side of
>> > > things which is not required for the test to work.
>> > > 
>> > > I can send a patch for that or you can, as you prefer?
>> > 
>> > OK. Please send a patch for that, for that patch will go linux-next.git tree via
>> > a tree for gpu/drm/i915 driver.
>> 
>> Patch sent. If I am right the easiest solution was just to remove the flush.
>> If I was wrong though I'll need to create a dedicated wq so we will see what
>> our automated CI will say.
>
> But besides of flush_scheduled_work() it looks like
> we also need to take care of the flush_workqueue() calls, no?!
>
> * i915_gem_drain_workqueue()
> * intel_ggtt.c:   flush_workqueue(ggtt->vm.i915->wq);
> * i915_gem_pm.c: flush_workqueue(i915->wq);
>
> and the display ones for
> dev_priv->modeset_wq
> i915->flip_wq
>
> besides the flush_scheduled_work in intel_modeset_driver_remove_noirq

I thought the problem was flushing the system-wide workqueues. The above
calls flush our own.

As to removing flush_scheduled_work() from
intel_modeset_driver_remove_noirq(), I think we'll need to account for
all the work and delayed work we've scheduled on the system workqueue,
i.e. we need to cancel or flush each of them individually, as
necessary. Some of them we do already, but some others, not really.

For example we never cancel the fbc underrun work on the driver remove
path AFAICT. And it's not even as simple as just adding the
cancel_work_sync(&fbc->underrun_work) call in intel_fbc_cleanup(),
because intel_fbc_cleanup() is called *after*
intel_mode_config_cleanup(), i.e. the work function might get called
after the crtc it accesses has been destroyed. So we're going to need to
change the cleanup order too.

Things have changed considerably since the flush was added in
1630fe754c83 ("drm/i915: Perform intel_enable_fbc() from a delayed
task").

I suppose the alternative is to have a local i915 display workqueue,
schedule all the random display works and delayed works on that, and
then flush that wq instead of the system wq in
intel_modeset_driver_remove_noirq().

IIUC, anyway.


BR,
Jani.


-- 
Jani Nikula, Intel Open Source Graphics Center