lkml.org 
[lkml]   [2019]   [Jun]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] time/tick-broadcast: Fix tick_broadcast_offline() lockdep complaint
On Thu, Jun 20, 2019 at 09:01:18AM -0700, Paul E. McKenney wrote:

> > > +#define TICK_SCHED_REMOTE_OFFLINE 0
> > > +#define TICK_SCHED_REMOTE_RUNNING 1
> > > +#define TICK_SCHED_REMOTE_OFFLINING 2
> >
> > That seems a daft set of values; consider { RUNNING, OFFLINING, OFFLINE }
> > and see below.
>
> As in make it an enum? I could do that.

Enum or define, I don't much care, but the 'natural' ordering of the
states is either: running -> offlining -> offline, or the other way
around, the given order in the patch just didn't make sense.

The one with running=0 just seems to work out nicer.

> > > +
> > > +// State diagram for ->state:
> > > +//
> > > +//
> > > +// +----->OFFLINE--------------------------+
> > > +// | |
> > > +// | |
> > > +// | | sched_tick_start()
> > > +// | sched_tick_remote() |
> > > +// | |
> > > +// | V
> > > +// | +---------->RUNNING
> > > +// | | |
> > > +// | | |
> > > +// | | |
> > > +// | sched_tick_start() | | sched_tick_stop()
> > > +// | | |
> > > +// | | |
> > > +// | | |
> > > +// +--------------------OFFLINING<---------+
> > > +//
> > > +//
> > > +// Other transitions get WARN_ON_ONCE(), except that sched_tick_remote()
> > > +// and sched_tick_start() are happy to leave the state in RUNNING.

> > Also, I find it harder to read that needed, maybe a little something
> > like so:
> >
> > /*
> > * OFFLINE
> > * | ^
> > * | | tick_remote()
> > * | |
> > * +--OFFLINING
> > * | ^
> > * tick_start() | | tick_stop()
> > * v |
> > * RUNNING
> > */
>
> As in remove the leading "sched_" from the function names? (The names
> were already there, so I left them be.)

That was just me being lazy, the main part being getting the states in a
linear order, instead of spread around a 2d grid.

> > While not wrong, it seems overly complicated; can't we do something
> > like:
> >
> > tick:
>
> As in sched_tick_remote(), right?
>
> > state = atomic_read(->state);
> > if (state) {
>
> You sure you don't want "if (state != RUNNING)"? But I guess you need
> to understand that RUNNING==0 to understand the atomic_inc_not_zero().

Right..

>
> > WARN_ON_ONCE(state != OFFLINING);
> > if (atomic_inc_not_zero(->state))
>
> This assumes that there cannot be concurrent calls to sched_tick_remote(),
> otherwise, you can end up with ->state==3. Which is a situation that
> my version does a WARN_ON_ONCE() for, so I guess the only difference is
> that mine would be guaranteed to complain and yours would complain with
> high probability. So fair enough!

I was assuming there was only a single work per CPU and there'd not be
concurrency on this path.

> > return;
> > }
> > queue_delayed_work();
> >
> >
> > stop:
> > /*
> > * This is hotplug; even without stop-machine, there cannot be
> > * concurrency on offlining specific CPUs.
> > */
> > state = atomic_read(->state);
>
> There cannot be a sched_tick_stop() or sched_tick_stop(), but there really
> can be a sched_tick_remote() right here in the absence of stop-machine,
> can't there? Or am I missing something other than stop-machine that
> prevents this?

There can be a remote tick, indeed.

> Now, you could argue that concurrency is safe: Either sched_tick_remote()
> sees RUNNING and doesn't touch the value, or it sees offlining and
> sched_tick_stop() makes no further changes,

That was indeed the thinking.

> but I am not sure that this qualifies as simpler...

There is that I suppose. I think my initial version was a little more
complicated, but after a few passes this happened :-)

> > WARN_ON_ONCE(state != RUNNING);
> > atomic_set(->state, OFFLINING);
>
> Another option would be to use atomic_xchg() as below instead of the
> atomic_read()/atomic_set() pair. Would that work for you?

Yes, that works I suppose. Is more expensive, but I don't think we
particularly care about that here.

> > start:
> > state = atomic_xchg(->state, RUNNING);
> > WARN_ON_ONCE(state == RUNNING);
> > if (state == OFFLINE) {
> > // ...
> > queue_delayed_work();
> > }
>
> This one looks to be an improvement on mine regardless of the other two.


\
 
 \ /
  Last update: 2019-06-20 23:11    [W:0.179 / U:4.144 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site