Re: Crashes with 874bbfe600a6 in 3.18.25

[Michal Hocko]

On Wed 03-02-16 11:24:41, Tejun Heo wrote:
> On Wed, Feb 03, 2016 at 01:28:56PM +0100, Michal Hocko wrote:
> > > The CPU was 168, and that one was offlined in the meantime. So
> > > __queue_work fails at:
> > >   if (!(wq->flags & WQ_UNBOUND))
> > >     pwq = per_cpu_ptr(wq->cpu_pwqs, cpu);
> > >   else
> > >     pwq = unbound_pwq_by_node(wq, cpu_to_node(cpu));
> > >     ^^^                           ^^^^ NODE is -1
> > >       \ pwq is NULL
> > > 
> > >   if (last_pool && last_pool != pwq->pool) { <--- BOOM
> 
> So, the proper fix here is keeping cpu <-> node mapping stable across
> cpu on/offlining which has been being worked on for a long time now.
> The patchst is pending and it fixes other issues too.

What if that node was memory offlined as well? It just doesn't make any
sense to stick to the old node when the old cpu went away already. If
anything and add_timer_on also for WORK_CPU_UNBOUND is really required
then we should at least preserve WORK_CPU_UNBOUND in dwork->cpu so that
__queue_work can actually move on to the local CPU properly and handle
the offline cpu properly.

> > So I think 874bbfe600a6 is really bogus. It should be reverted. We
> > already have a proper fix for vmstat 176bed1de5bf ("vmstat: explicitly
> > schedule per-cpu work on the CPU we need it to run on"). This which
> > should be used for the stable trees as a replacement.
> 
> It's not bogus.  We can't flip a property that has been guaranteed
> without any provision for verification.  Why do you think vmstat blow
> up in the first place?

Because it wants to have a strong per-cpu guarantee while it used
to fail to tell so. My understanding was that this is exactly what
queue_delayed_work_on is for while WORK_CPU_UNBOUND tells that the
caller doesn't really insist on any particular CPU (just local CPU is
preferred).

-- 
Michal Hocko
SUSE Labs