Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <CAJuCfpG4NkhpQvZjgXZ_3gm6Hf1QgN_eUOQ8iX9Cv1k9whLwSQ@mail.gmail.com>
 <CAJuCfpGMdegJvLO-o-96PNf6iV5sWcmj=WUovi9ixRbeiHX70w@mail.gmail.com>
 <20200424174025.GA13592@hirez.programming.kicks-ass.net> <CAJuCfpEEp=UVa=WXtXwdKbUa_yZsrmaE=wqhQL7xEYHXcKbg=w@mail.gmail.com>
 <CALvZod4hNWFMErVSCPnPV4TJAhJHH11ta_c30dhOgaWOesQxaA@mail.gmail.com> <CAJuCfpECDryYL7ia+JH8HZtWRWHtLFFg2ZwtAB4nj=mXDDPENg@mail.gmail.com>
In-Reply-To: <CAJuCfpECDryYL7ia+JH8HZtWRWHtLFFg2ZwtAB4nj=mXDDPENg@mail.gmail.com>
From:   Suren Baghdasaryan <surenb@google.com>
Date:   Mon, 27 Apr 2020 11:47:50 -0700
Message-ID: <CAJuCfpE+qHpOVU9JS+CEVKgdJMgZB8jG3KKq=AMFNZXCgvE69Q@mail.gmail.com>
Subject: Re: lockdep warning about possible circular dependency in PSI
To:     Shakeel Butt <shakeelb@google.com>
Cc:     Peter Zijlstra <peterz@infradead.org>,
        Ingo Molnar <mingo@redhat.com>,
        Andrew Morton <akpm@linux-foundation.org>, will@kernel.org,
        Johannes Weiner <hannes@cmpxchg.org>,
        LKML <linux-kernel@vger.kernel.org>,
        kernel-team <kernel-team@android.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Sun, Apr 26, 2020 at 5:00 PM Suren Baghdasaryan <surenb@google.com> wrot=
e:
>
> On Fri, Apr 24, 2020 at 1:54 PM Shakeel Butt <shakeelb@google.com> wrote:
> >
> > On Fri, Apr 24, 2020 at 10:52 AM Suren Baghdasaryan <surenb@google.com>=
 wrote:
> > >
> > > On Fri, Apr 24, 2020 at 10:40 AM Peter Zijlstra <peterz@infradead.org=
> wrote:
> > > >
> > > > On Fri, Apr 24, 2020 at 09:34:42AM -0700, Suren Baghdasaryan wrote:
> > > > > Sorry to bother you again folks. Any suggestions on how to silenc=
e
> > > > > this lockdep warning which I believe to be a false positive?
> > > > >
> > > > > On Wed, Apr 15, 2020 at 4:01 PM Suren Baghdasaryan <surenb@google=
.com> wrote:
> > > > > >
> > > > > > I received a report about possible circular locking dependency =
warning
> > > > > > generated from PSI polling code. I think we are protected from =
this
> > > > > > scenario by poll_scheduled atomic but wanted to double-check an=
d I=E2=80=99m
> > > > > > looking for an advice on how to annotate this case to fix the l=
ockdep
> > > > > > warning. I copied the detailed information at the end of this e=
mail
> > > > > > but the short story is this:
> > > > > >
> > > > > > "WARNING: possible circular locking dependency detected" is gen=
erated
> > > > > > with CONFIG_PSI and CONFIG_LOCKDEP enabled. The dependency chai=
n it
> > > > > > describes is:
> > > > > >
> > > > > > #0
> > > > > > kthread_delayed_work_timer_fn()
> > > > > >  |
> > > > > > worker->lock
> > > > > >  |
> > > > > > try_to_wake_up()
> > > > > >  |
> > > > > > p->pi_lock
> > > > > >
> > > > > > #1
> > > > > > sched_fork()
> > > > > >  |
> > > > > > p->pi_lock
> > > > > >  |
> > > > > > task_fork_fair()
> > > > > >  |
> > > > > > rq->lock
> > > > > >
> > > > > > #2
> > > > > > psi_memstall_enter
> > > > > >  |
> > > > > > rq->lock
> > > > > >  |
> > > > > > kthread_queue_delayed_work()
> > > > > >  |
> > > > > > worker->lock
> > > >
> > > > Irrespective of it actually being a deadlock or not, it is fairly
> > > > fragile. Ideally we'd fix #2, we really should minimize the number =
of
> > > > locks nested under rq->lock.
> > > >
> > > > That said, here's the easy fix, which breaks #0.
> > > >
> > >
> > > Thanks for the suggestion, Peter. Let me digest this and will post a
> > > patch with your Suggested-by.
> > > Cheers!
> > >
> >
> > I tested on my simple repro and the patch fixes the lockdep splat.
> >
> > You can add
> > Tested-by: Shakeel Butt <shakeelb@google.com>
> >
>
> Thanks Shakeel! Will do.
>
> > > > ---
> > > > diff --git a/kernel/kthread.c b/kernel/kthread.c
> > > > index bfbfa481be3a..b443bba7dd21 100644
> > > > --- a/kernel/kthread.c
> > > > +++ b/kernel/kthread.c
> > > > @@ -806,14 +806,15 @@ static void kthread_insert_work_sanity_check(=
struct kthread_worker *worker,
> > > >  /* insert @work before @pos in @worker */
> > > >  static void kthread_insert_work(struct kthread_worker *worker,
> > > >                                 struct kthread_work *work,
> > > > -                               struct list_head *pos)
> > > > +                               struct list_head *pos,
> > > > +                               struct wake_q_head *wake_q)
> > > >  {
> > > >         kthread_insert_work_sanity_check(worker, work);
> > > >
> > > >         list_add_tail(&work->node, pos);
> > > >         work->worker =3D worker;
> > > >         if (!worker->current_work && likely(worker->task))
> > > > -               wake_up_process(worker->task);
> > > > +               wake_q_add(wake_q, worker->task);
> > > >  }
> > > >
> > > >  /**
> > > > @@ -831,15 +832,19 @@ static void kthread_insert_work(struct kthrea=
d_worker *worker,
> > > >  bool kthread_queue_work(struct kthread_worker *worker,
> > > >                         struct kthread_work *work)
> > > >  {
> > > > -       bool ret =3D false;
> > > > +       DEFINE_WAKE_Q(wake_q);
> > > >         unsigned long flags;
> > > > +       bool ret =3D false;
> > > >
> > > >         raw_spin_lock_irqsave(&worker->lock, flags);
> > > >         if (!queuing_blocked(worker, work)) {
> > > > -               kthread_insert_work(worker, work, &worker->work_lis=
t);
> > > > +               kthread_insert_work(worker, work, &worker->work_lis=
t, &wake_q);
> > > >                 ret =3D true;
> > > >         }
> > > >         raw_spin_unlock_irqrestore(&worker->lock, flags);
> > > > +
> > > > +       wake_up_q(&wake_q);
> > > > +
> > > >         return ret;
> > > >  }
> > > >  EXPORT_SYMBOL_GPL(kthread_queue_work);
> > > > @@ -857,6 +862,7 @@ void kthread_delayed_work_timer_fn(struct timer=
_list *t)
> > > >         struct kthread_delayed_work *dwork =3D from_timer(dwork, t,=
 timer);
> > > >         struct kthread_work *work =3D &dwork->work;
> > > >         struct kthread_worker *worker =3D work->worker;
> > > > +       DEFINE_WAKE_Q(wake_q);
> > > >         unsigned long flags;
> > > >
> > > >         /*
> > > > @@ -873,15 +879,18 @@ void kthread_delayed_work_timer_fn(struct tim=
er_list *t)
> > > >         /* Move the work from worker->delayed_work_list. */
> > > >         WARN_ON_ONCE(list_empty(&work->node));
> > > >         list_del_init(&work->node);
> > > > -       kthread_insert_work(worker, work, &worker->work_list);
> > > > +       kthread_insert_work(worker, work, &worker->work_list, &wake=
_q);
> > > >
> > > >         raw_spin_unlock_irqrestore(&worker->lock, flags);
> > > > +
> > > > +       wake_up_q(&wake_q);
> > > >  }
> > > >  EXPORT_SYMBOL(kthread_delayed_work_timer_fn);
> > > >
> > > >  static void __kthread_queue_delayed_work(struct kthread_worker *wo=
rker,
> > > >                                          struct kthread_delayed_wor=
k *dwork,
> > > > -                                        unsigned long delay)
> > > > +                                        unsigned long delay,
> > > > +                                        struct wake_q_head *wake_q=
)
> > > >  {
> > > >         struct timer_list *timer =3D &dwork->timer;
> > > >         struct kthread_work *work =3D &dwork->work;
> > > > @@ -895,7 +904,7 @@ static void __kthread_queue_delayed_work(struct=
 kthread_worker *worker,
> > > >          * on that there's no such delay when @delay is 0.
> > > >          */
> > > >         if (!delay) {
> > > > -               kthread_insert_work(worker, work, &worker->work_lis=
t);
> > > > +               kthread_insert_work(worker, work, &worker->work_lis=
t, wake_q);
> > > >                 return;
> > > >         }
> > > >
> > > > @@ -928,17 +937,21 @@ bool kthread_queue_delayed_work(struct kthrea=
d_worker *worker,
> > > >                                 unsigned long delay)
> > > >  {
> > > >         struct kthread_work *work =3D &dwork->work;
> > > > +       DEFINE_WAKE_Q(wake_q);
> > > >         unsigned long flags;
> > > >         bool ret =3D false;
> > > >
> > > >         raw_spin_lock_irqsave(&worker->lock, flags);
> > > >
> > > >         if (!queuing_blocked(worker, work)) {
> > > > -               __kthread_queue_delayed_work(worker, dwork, delay);
> > > > +               __kthread_queue_delayed_work(worker, dwork, delay, =
&wake_q);
> > > >                 ret =3D true;
> > > >         }
> > > >
> > > >         raw_spin_unlock_irqrestore(&worker->lock, flags);
> > > > +
> > > > +       wake_up_q(&wake_q);
> > > > +
> > > >         return ret;
> > > >  }
> > > >  EXPORT_SYMBOL_GPL(kthread_queue_delayed_work);
> > > > @@ -967,6 +980,7 @@ void kthread_flush_work(struct kthread_work *wo=
rk)
> > > >                 KTHREAD_WORK_INIT(fwork.work, kthread_flush_work_fn=
),
> > > >                 COMPLETION_INITIALIZER_ONSTACK(fwork.done),
> > > >         };
> > > > +       DEFINE_WAKE_Q(wake_q);
> > > >         struct kthread_worker *worker;
> > > >         bool noop =3D false;
> > > >
> > > > @@ -979,15 +993,17 @@ void kthread_flush_work(struct kthread_work *=
work)
> > > >         WARN_ON_ONCE(work->worker !=3D worker);
> > > >
> > > >         if (!list_empty(&work->node))
> > > > -               kthread_insert_work(worker, &fwork.work, work->node=
.next);
> > > > +               kthread_insert_work(worker, &fwork.work, work->node=
.next, &wake_q);
> > > >         else if (worker->current_work =3D=3D work)
> > > >                 kthread_insert_work(worker, &fwork.work,
> > > > -                                   worker->work_list.next);
> > > > +                                   worker->work_list.next, &wake_q=
);
> > > >         else
> > > >                 noop =3D true;
> > > >
> > > >         raw_spin_unlock_irq(&worker->lock);
> > > >
> > > > +       wake_up_q(&wake_q);
> > > > +
> > > >         if (!noop)
> > > >                 wait_for_completion(&fwork.done);
> > > >  }
> > > > @@ -1065,6 +1081,7 @@ bool kthread_mod_delayed_work(struct kthread_=
worker *worker,
> > > >                               unsigned long delay)
> > > >  {
> > > >         struct kthread_work *work =3D &dwork->work;
> > > > +       DEFINE_WAKE_Q(wake_q);
> > > >         unsigned long flags;
> > > >         int ret =3D false;
> > > >
> > > > @@ -1083,9 +1100,12 @@ bool kthread_mod_delayed_work(struct kthread=
_worker *worker,
> > > >
> > > >         ret =3D __kthread_cancel_work(work, true, &flags);
> > > >  fast_queue:
> > > > -       __kthread_queue_delayed_work(worker, dwork, delay);
> > > > +       __kthread_queue_delayed_work(worker, dwork, delay, &wake_q)=
;
> > > >  out:
> > > >         raw_spin_unlock_irqrestore(&worker->lock, flags);
> > > > +
> > > > +       wake_up_q(&wake_q);
> > > > +
> > > >         return ret;
> > > >  }
> > > >  EXPORT_SYMBOL_GPL(kthread_mod_delayed_work);
> > > >

Patch is posted at https://lkml.org/lkml/2020/4/27/985 . Had to
include linux/sched/wake_q.h and fix a long line but other than that
it's unchanged.
Thanks!

> > > > --
> > > > To unsubscribe from this group and stop receiving emails from it, s=
end an email to kernel-team+unsubscribe@android.com.
> > > >