From: "Paul E. McKenney" <paulmck@kernel.org>
To: peterz@infradead.org
Cc: syzbot <syzbot+cb3b69ae80afd6535b0e@syzkaller.appspotmail.com>,
fweisbec@gmail.com, linux-kernel@vger.kernel.org,
mingo@kernel.org, syzkaller-bugs@googlegroups.com,
tglx@linutronix.de
Subject: Re: INFO: rcu detected stall in smp_call_function
Date: Wed, 26 Aug 2020 14:16:15 -0700 [thread overview]
Message-ID: <20200826211615.GA22279@paulmck-ThinkPad-P72> (raw)
In-Reply-To: <20200826140733.GM2855@paulmck-ThinkPad-P72>
On Wed, Aug 26, 2020 at 07:07:33AM -0700, Paul E. McKenney wrote:
> On Wed, Aug 26, 2020 at 11:51:44AM +0200, peterz@infradead.org wrote:
> > On Tue, Aug 25, 2020 at 08:48:41AM -0700, Paul E. McKenney wrote:
> >
> > > > Paul, I wanted to use this function, but found it has very weird
> > > > semantics.
> > > >
> > > > Why do you need it to (remotely) call @func when p is current? The user
> > > > in rcu_print_task_stall() explicitly bails in this case, and the other
> > > > in rcu_wait_for_one_reader() will attempt an IPI.
> > >
> > > Good question. Let me look at the invocations:
> > >
> > > o trc_wait_for_one_reader() bails on current before
> > > invoking try_invoke_on_locked_down_task():
> > >
> > > if (t == current) {
> > > t->trc_reader_checked = true;
> > > trc_del_holdout(t);
> > > WARN_ON_ONCE(t->trc_reader_nesting);
> > > return;
> > > }
> > >
> > > o rcu_print_task_stall() might well invoke on the current task,
> > > low though the probability of this happening might be. (The task
> > > has to be preempted within an RCU read-side critical section
> > > and resume in time for the scheduling-clock irq that will report
> > > the RCU CPU stall to interrupt it.)
> > >
> > > And you are right, no point in an IPI in this case.
> > >
> > > > Would it be possible to change this function to:
> > > >
> > > > - blocked task: call @func with p->pi_lock held
> > > > - queued, !running task: call @func with rq->lock held
> > > > - running task: fail.
> > > >
> > > > ?
> > >
> > > Why not a direct call in the current-task case, perhaps as follows,
> > > including your change above? This would allow the RCU CPU stall
> > > case to work naturally and without the IPI.
> > >
> > > Would that work for your use case?
> >
> > It would in fact, but at this point I'd almost be inclined to stick the
> > IPI in as well. But small steps I suppose. So yes.
>
> If interrupts are disabled, won't a self-IPI deadlock?
>
> But good point that the current-task case could be the only case invoking
> the function with interrupts enabled, which now that you mention it does
> sound like an accident waiting to happen. So how about the following
> instead?
>
> Thanx, Paul
>
> ------------------------------------------------------------------------
>
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 8471a0f..f8ed04c 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -2997,7 +2997,7 @@ try_to_wake_up(struct task_struct *p, unsigned int state, int wake_flags)
> * state while invoking @func(@arg). This function can use ->on_rq and
> * task_curr() to work out what the state is, if required. Given that
> * @func can be invoked with a runqueue lock held, it had better be quite
> - * lightweight.
> + * lightweight. Note that the current task is implicitly locked down.
> *
> * Returns:
> * @false if the task slipped out from under the locks.
> @@ -3006,12 +3006,17 @@ try_to_wake_up(struct task_struct *p, unsigned int state, int wake_flags)
> */
> bool try_invoke_on_locked_down_task(struct task_struct *p, bool (*func)(struct task_struct *t, void *arg), void *arg)
> {
> - bool ret = false;
> struct rq_flags rf;
> + bool ret = false;
> struct rq *rq;
>
> - lockdep_assert_irqs_enabled();
> - raw_spin_lock_irq(&p->pi_lock);
> + if (p == current) {
> + local_irq_save(rf.flags);
> + ret = func(p, arg);
> + local_irq_restore(rf.flags);
> + return ret;
> + }
Is this "if" statement anything more than an optimization, and a dubious
one at that? It looks to me like the current task would otherwise end
up acquiring its own ->pi_lock, then acquiring the rq lock which cannot
change due to interrupts being disabled. And then invoking the same
function with interrupts disabled, returning the same result.
Am I missing something subtle here?
Thanx, Paul
> + raw_spin_lock_irqsave(&p->pi_lock, rf.flags);
> if (p->on_rq) {
> rq = __task_rq_lock(p, &rf);
> if (task_rq(p) == rq)
> @@ -3028,7 +3033,7 @@ bool try_invoke_on_locked_down_task(struct task_struct *p, bool (*func)(struct t
> ret = func(p, arg);
> }
> }
> - raw_spin_unlock_irq(&p->pi_lock);
> + raw_spin_unlock_irqrestore(&p->pi_lock, rf.flags);
> return ret;
> }
>
next prev parent reply other threads:[~2020-08-26 21:16 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-07-29 8:44 INFO: rcu detected stall in smp_call_function syzbot
2020-07-29 12:58 ` peterz
2020-08-25 13:24 ` peterz
2020-08-25 15:48 ` Paul E. McKenney
2020-08-26 9:51 ` peterz
2020-08-26 14:07 ` Paul E. McKenney
2020-08-26 21:16 ` Paul E. McKenney [this message]
2020-09-06 18:40 ` syzbot
[not found] <20220322074002.3294-1-hdanton@sina.com>
2022-03-22 7:40 ` syzbot
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200826211615.GA22279@paulmck-ThinkPad-P72 \
--to=paulmck@kernel.org \
--cc=fweisbec@gmail.com \
--cc=linux-kernel@vger.kernel.org \
--cc=mingo@kernel.org \
--cc=peterz@infradead.org \
--cc=syzbot+cb3b69ae80afd6535b0e@syzkaller.appspotmail.com \
--cc=syzkaller-bugs@googlegroups.com \
--cc=tglx@linutronix.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).