On Tue, Apr 28, 2020 at 11:04 AM Suren Baghdasaryan <[email protected]> wrote: > > On Tue, Apr 28, 2020 at 9:31 AM Peter Zijlstra <[email protected]> wrote: > > > > On Mon, Apr 27, 2020 at 11:43:58AM -0700, Suren Baghdasaryan wrote: > > > A number of kthread-related functions indirectly take task_struct->pi_lock > > > while holding worker->lock in the call chain like this: > > > spin_lock(&worker->lock) > > > kthread_insert_work > > > wake_up_process > > > try_to_wake_up > > > raw_spin_lock_irqsave(&p->pi_lock, flags) > > > > > > This lock dependency exists whenever kthread_insert_work is called either > > > directly or indirectly via __kthread_queue_delayed_work in the following > > > functions: > > > kthread_queue_work > > > kthread_delayed_work_timer_fn > > > kthread_queue_delayed_work > > > kthread_flush_work > > > kthread_mod_delayed_work > > > > > > This creates possibilities for circular dependencies like the one reported > > > at: https://lkml.org/lkml/2020/4/24/954 > > > > Please, do not use lkml.org links. > > Thanks for the review! Would > https://lore.kernel.org/lkml/cajucfpg4nkhpqvzjgxz_3gm6hf1qgn_euoq8ix9cv1k9whl...@mail.gmail.com > be better or should I just add the body of that report here? Or do not > mention it at all?
Sorry, this time in plain text mode... If there are no more comments on this patch I'll post a v2 with lore.kernel.org instead of lkml.org link. Please let me know if there are more issues that you would like to be addressed. IMHO, taking kthread_queue_delayed_work() out from under rq->lock (if we can figure out how to do that cleanly) can be a separate patch and this one is still useful regardless of that. Thanks! > > > > > Also, ideally, we'd pull that kthread_queue_delayed_work() out from > > under rq->lock. > > I understand but I don't see an easy way to do that. We need to start > PSI polling whenever a monitored PSI state changes: > https://elixir.bootlin.com/linux/v5.6.7/source/kernel/sched/psi.c#L783. > This is happening under rq->lock because PSI accounting is done from > inside enqueue_task/dequeue_task - the call chain is: > > enqueue_task > psi_enqueue > psi_task_change > psi_group_change > > psi_schedule_poll_work > psi_task_change > > IIUC enqueue_task/dequeue_task are called with rq->lock taken, so > moving kthread_queue_delayed_work out is not trivial. > > > > > In fact, looking at it, WTH is the delayed branch of > > kthread_queue_delayed_work() under that lock? That whole > > delayed_work_list thing smells like bong-hits. > > I have the poll_scheduled atomic specifically to ensure that > kthread_queue_delayed_work does not block as commented here: > https://elixir.bootlin.com/linux/v5.7-rc3/source/kernel/sched/psi.c#L551. > I understand this is not ideal. If there is a better way to schedule > that kworker while ensuring it does not block I would be happy to > rework this. Any suggestions? > > > > > -- > > To unsubscribe from this group and stop receiving emails from it, send an > > email to [email protected]. > >
