On Thu, Jun 30, 2022 at 05:34:22PM +0300, Jani Nikula wrote:
> On Thu, 30 Jun 2022, Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx> 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.
oh, indeed, sorry for the noise.
I had ignored the if for the new __warn_flushing...
>
> As to removing flush_scheduled_work() from
>intel_modeset_driver_remove_noirq(),
yeap, this is the only real one...
> 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
