Re: [PATCH RT 6/8] sched: migrate_enable: Set state to TASK_RUNNING

[Scott Wood <swood@redhat.com>]

On Tue, 2019-09-17 at 17:31 +0200, Sebastian Andrzej Siewior wrote:
> On 2019-07-27 00:56:36 [-0500], Scott Wood wrote:
> > If migrate_enable() is called while a task is preparing to sleep
> > (state != TASK_RUNNING), that triggers a debug check in stop_one_cpu().
> > Explicitly reset state to acknowledge that we're accepting the spurious
> > wakeup.
> > 
> > Signed-off-by: Scott Wood <swood@redhat.com>
> > ---
> >  kernel/sched/core.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> > index 38a9a9df5638..eb27a9bf70d7 100644
> > --- a/kernel/sched/core.c
> > +++ b/kernel/sched/core.c
> > @@ -7396,6 +7396,14 @@ void migrate_enable(void)
> >  			unpin_current_cpu();
> >  			preempt_lazy_enable();
> >  			preempt_enable();
> > +
> > +			/*
> > +			 * Avoid sleeping with an existing non-running
> > +			 * state.  This will result in a spurious wakeup
> > +			 * for the calling context.
> > +			 */
> > +			__set_current_state(TASK_RUNNING);
> 
> Do you have an example for this?

Unfortunately I didn't save the traceback of where I originally saw it, but
I ran it again and hit the warning in prepare_to_wait_event().

>  I'm not too sure if we are not using a
> state by doing this. Actually we were losing it and get yelled at.  We do:
> > rt_spin_unlock()
> > {
> >         rt_spin_lock_fastunlock();
> >         migrate_enable();
> > }
> 
> So save the state as part of the locking process and lose it as part of
> migrate_enable() if the CPU mask was changed. I *think* we need to
> preserve that state until after the migration to the other CPU.

Preserving the state would be ideal, though in most cases occasionally
losing the state should be tolerable as long as it doesn't happen
persistently.  TASK_DEAD is an exception but that doesn't do anything before
schedule() that could end up here.  I suppose there could be some places
that use TASK_UNINTERRUPTIBLE without checking the actual condition after
schedule(), and which do something that can end up here before schedule()...

We can't use saved_state here though, because we can get here inside the
rtmutex code (e.g. from debug_rt_mutex_print_deadlock)... and besides, the
waker won't have WF_LOCK_SLEEPER and so saved_state will get cleared.

-Scott