Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1325090 > unrolled thread

Re: Crashes with 874bbfe600a6 in 3.18.25

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: Crashes with 874bbfe600a6 in 3.18.25 Jiri Slaby <jslaby@suse.cz> - 2016-02-03 10:40 +0100
    Re: Crashes with 874bbfe600a6 in 3.18.25 Thomas Gleixner <tglx@linutronix.de> - 2016-02-03 11:50 +0100
    Re: Crashes with 874bbfe600a6 in 3.18.25 Michal Hocko <mhocko@kernel.org> - 2016-02-03 13:30 +0100
      Re: Crashes with 874bbfe600a6 in 3.18.25 Tejun Heo <tj@kernel.org> - 2016-02-03 17:30 +0100
        Re: Crashes with 874bbfe600a6 in 3.18.25 Michal Hocko <mhocko@kernel.org> - 2016-02-03 17:50 +0100
          Re: Crashes with 874bbfe600a6 in 3.18.25 Tejun Heo <tj@kernel.org> - 2016-02-03 18:10 +0100
            Re: Crashes with 874bbfe600a6 in 3.18.25 Michal Hocko <mhocko@kernel.org> - 2016-02-04 07:40 +0100
              Re: Crashes with 874bbfe600a6 in 3.18.25 Michal Hocko <mhocko@kernel.org> - 2016-02-04 08:50 +0100
        Re: Crashes with 874bbfe600a6 in 3.18.25 Mike Galbraith <umgwanakikbuti@gmail.com> - 2016-02-03 18:10 +0100
          Re: Crashes with 874bbfe600a6 in 3.18.25 Tejun Heo <tj@kernel.org> - 2016-02-03 18:10 +0100
            Re: Crashes with 874bbfe600a6 in 3.18.25 Tejun Heo <tj@kernel.org> - 2016-02-03 18:20 +0100
            Re: Crashes with 874bbfe600a6 in 3.18.25 Mike Galbraith <umgwanakikbuti@gmail.com> - 2016-02-03 18:20 +0100
            Re: Crashes with 874bbfe600a6 in 3.18.25 Mike Galbraith <umgwanakikbuti@gmail.com> - 2016-02-04 03:10 +0100
              Re: Crashes with 874bbfe600a6 in 3.18.25 Tejun Heo <tj@kernel.org> - 2016-02-05 17:50 +0100
                Re: Crashes with 874bbfe600a6 in 3.18.25 Mike Galbraith <umgwanakikbuti@gmail.com> - 2016-02-05 21:50 +0100
                  Re: Crashes with 874bbfe600a6 in 3.18.25 Tejun Heo <tj@kernel.org> - 2016-02-05 22:00 +0100
                    Re: Crashes with 874bbfe600a6 in 3.18.25 Tejun Heo <tj@kernel.org> - 2016-02-05 22:10 +0100
                      Re: Crashes with 874bbfe600a6 in 3.18.25 Henrique de Moraes Holschuh <hmh@hmh.eng.br> - 2016-02-06 14:10 +0100
                        Re: Crashes with 874bbfe600a6 in 3.18.25 Mike Galbraith <umgwanakikbuti@gmail.com> - 2016-02-07 06:30 +0100
                          Re: Crashes with 874bbfe600a6 in 3.18.25 Mike Galbraith <umgwanakikbuti@gmail.com> - 2016-02-07 07:10 +0100
                    Re: Crashes with 874bbfe600a6 in 3.18.25 Mike Galbraith <umgwanakikbuti@gmail.com> - 2016-02-05 22:10 +0100
            Re: Crashes with 874bbfe600a6 in 3.18.25 Mike Galbraith <umgwanakikbuti@gmail.com> - 2016-02-04 11:10 +0100
              Re: Crashes with 874bbfe600a6 in 3.18.25 Thomas Gleixner <tglx@linutronix.de> - 2016-02-04 11:50 +0100
                Re: Crashes with 874bbfe600a6 in 3.18.25 Mike Galbraith <umgwanakikbuti@gmail.com> - 2016-02-04 12:10 +0100
                Re: Crashes with 874bbfe600a6 in 3.18.25 Jan Kara <jack@suse.cz> - 2016-02-04 12:30 +0100
                  Re: Crashes with 874bbfe600a6 in 3.18.25 Daniel Bilik <daniel.bilik@neosystem.cz> - 2016-02-04 18:00 +0100
                    Re: Crashes with 874bbfe600a6 in 3.18.25 Mike Galbraith <umgwanakikbuti@gmail.com> - 2016-02-05 03:50 +0100
                      Re: Crashes with 874bbfe600a6 in 3.18.25 Daniel Bilik <daniel.bilik@neosystem.cz> - 2016-02-05 09:20 +0100
                        Re: Crashes with 874bbfe600a6 in 3.18.25 Mike Galbraith <umgwanakikbuti@gmail.com> - 2016-02-05 09:40 +0100
        Re: Crashes with 874bbfe600a6 in 3.18.25 Thomas Gleixner <tglx@linutronix.de> - 2016-02-03 19:50 +0100
          Re: Crashes with 874bbfe600a6 in 3.18.25 Thomas Gleixner <tglx@linutronix.de> - 2016-02-03 20:10 +0100
            Re: Crashes with 874bbfe600a6 in 3.18.25 Tejun Heo <tj@kernel.org> - 2016-02-03 20:20 +0100
          Re: Crashes with 874bbfe600a6 in 3.18.25 Tejun Heo <tj@kernel.org> - 2016-02-03 20:10 +0100
        Re: Crashes with 874bbfe600a6 in 3.18.25 Mike Galbraith <umgwanakikbuti@gmail.com> - 2016-02-05 06:50 +0100

Page 2 of 2 — ← Prev page 1 [2]

#1328072

On Fri, 2016-02-05 at 15:54 -0500, Tejun Heo wrote:

> What are you suggesting?

That 874bbfe6 should die.

	-Mike

[toc] | [prev] | [next] | [standalone]

#1326544

On Wed, 2016-02-03 at 12:06 -0500, Tejun Heo wrote:
> On Wed, Feb 03, 2016 at 06:01:53PM +0100, Mike Galbraith wrote:
> > Hm, so it's ok to queue work to an offline CPU?  What happens if it
> > doesn't come back for an eternity or two?
> 
> Right now, it just loses affinity....

WRT affinity...

Somebody somewhere queues a delayed work, a timer is started on CPUX,
work is targeted at CPUX.  Now wash/rinse/repeat mod_delayed_work()
along with migrations.  Should __queue_delayed_work() not refrain from
altering dwork->cpu once set?

I'm also wondering why 22b886dd only applies to kernels >= 4.2.

<quote>
Regardless of the previous CPU a timer was on, add_timer_on()
currently simply sets timer->flags to the new CPU.  As the caller must
be seeing the timer as idle, this is locally fine, but the timer
leaving the old base while unlocked can lead to race conditions as
follows.

Let's say timer was on cpu 0.

  cpu 0                                 cpu 1
  -----------------------------------------------------------------------------
  del_timer(timer) succeeds
                                        del_timer(timer)
                                          lock_timer_base(timer) locks cpu_0_base
  add_timer_on(timer, 1)
    spin_lock(&cpu_1_base->lock)
    timer->flags set to cpu_1_base
    operates on @timer                    operates on @timer
</quote>

What's the difference between...
     timer->flags = (timer->flags & ~TIMER_BASEMASK) | cpu;
and...
     timer_set_base(timer, base);

...that makes that fix unneeded prior to 4.2?  We take the same locks
in < 4.2 kernels, so seemingly both will diddle concurrently above.

	-Mike

[toc] | [prev] | [next] | [standalone]

#1326652

On Thu, 4 Feb 2016, Mike Galbraith wrote:
> On Wed, 2016-02-03 at 12:06 -0500, Tejun Heo wrote:
> > On Wed, Feb 03, 2016 at 06:01:53PM +0100, Mike Galbraith wrote:
> > > Hm, so it's ok to queue work to an offline CPU?  What happens if it
> > > doesn't come back for an eternity or two?
> > 
> > Right now, it just loses affinity....
> 
> WRT affinity...
> 
> Somebody somewhere queues a delayed work, a timer is started on CPUX,
> work is targeted at CPUX.  Now wash/rinse/repeat mod_delayed_work()
> along with migrations.  Should __queue_delayed_work() not refrain from
> altering dwork->cpu once set?
> 
> I'm also wondering why 22b886dd only applies to kernels >= 4.2.
> 
> <quote>
> Regardless of the previous CPU a timer was on, add_timer_on()
> currently simply sets timer->flags to the new CPU.  As the caller must
> be seeing the timer as idle, this is locally fine, but the timer
> leaving the old base while unlocked can lead to race conditions as
> follows.
> 
> Let's say timer was on cpu 0.
> 
>   cpu 0                                 cpu 1
>   -----------------------------------------------------------------------------
>   del_timer(timer) succeeds
>                                         del_timer(timer)
>                                           lock_timer_base(timer) locks cpu_0_base
>   add_timer_on(timer, 1)
>     spin_lock(&cpu_1_base->lock)
>     timer->flags set to cpu_1_base
>     operates on @timer                    operates on @timer
> </quote>
> 
> What's the difference between...
>      timer->flags = (timer->flags & ~TIMER_BASEMASK) | cpu;
> and...
>      timer_set_base(timer, base);
> 
> ...that makes that fix unneeded prior to 4.2?  We take the same locks
> in < 4.2 kernels, so seemingly both will diddle concurrently above.

Indeed, you are right.

The same can happen on pre 4.2, just the fix does not apply as we changed the
internals how the base is managed in the timer itself. Backport below.

Thanks,

	tglx

8<----------------------------

--- a/kernel/time/timer.c
+++ b/kernel/time/timer.c
@@ -956,13 +956,26 @@ EXPORT_SYMBOL(add_timer);
  */
 void add_timer_on(struct timer_list *timer, int cpu)
 {
-	struct tvec_base *base = per_cpu(tvec_bases, cpu);
+	struct tvec_base *new_base = per_cpu(tvec_bases, cpu);
+	struct tvec_base *base;
 	unsigned long flags;
 
 	timer_stats_timer_set_start_info(timer);
 	BUG_ON(timer_pending(timer) || !timer->function);
-	spin_lock_irqsave(&base->lock, flags);
-	timer_set_base(timer, base);
+
+	/*
+	 * If @timer was on a different CPU, it must be migrated with the
+	 * old base locked to prevent other operations proceeding with the
+	 * wrong base locked.  See lock_timer_base().
+	 */
+	base = lock_timer_base(timer, &flags);
+	if (base != new_base) {
+		timer_set_base(timer, NULL);
+		spin_unlock(&base->lock);
+		base = new_base;
+		spin_lock(&base->lock);
+		timer_set_base(timer, base);
+	}
 	debug_activate(timer, timer->expires);
 	internal_add_timer(base, timer);
 	spin_unlock_irqrestore(&base->lock, flags);

[toc] | [prev] | [next] | [standalone]

#1326668

On Thu, 2016-02-04 at 11:46 +0100, Thomas Gleixner wrote:
> On Thu, 4 Feb 2016, Mike Galbraith wrote:

> > I'm also wondering why 22b886dd only applies to kernels >= 4.2.
> > 
> > 
> > Regardless of the previous CPU a timer was on, add_timer_on()
> > currently simply sets timer->flags to the new CPU.  As the caller must
> > be seeing the timer as idle, this is locally fine, but the timer
> > leaving the old base while unlocked can lead to race conditions as
> > follows.
> > 
> > Let's say timer was on cpu 0.
> > 
> >   cpu 0                                 cpu 1
> >   -----------------------------------------------------------------------------
> >   del_timer(timer) succeeds
> >                                         del_timer(timer)
> >                                           lock_timer_base(timer) locks cpu_0_base
> >   add_timer_on(timer, 1)
> >     spin_lock(&cpu_1_base->lock)
> >     timer->flags set to cpu_1_base
> >     operates on @timer                    operates on @timer
> > 
> > 
> > What's the difference between...
> >      timer->flags = (timer->flags & ~TIMER_BASEMASK) | cpu;
> > and...
> >      timer_set_base(timer, base);
> > 
> > ...that makes that fix unneeded prior to 4.2?  We take the same locks
> > in < 4.2 kernels, so seemingly both will diddle concurrently above.
> 
> Indeed, you are right.

Whew, thanks for confirming, looking for what the hell I was missing
wasn't going well at all, ate most of my day.

> The same can happen on pre 4.2, just the fix does not apply as we changed the
> internals how the base is managed in the timer itself. Backport below.

Exactly what I did locally.

	-Mike

[toc] | [prev] | [next] | [standalone]

#1326682

On Thu 04-02-16 11:46:47, Thomas Gleixner wrote:
> On Thu, 4 Feb 2016, Mike Galbraith wrote:
> > On Wed, 2016-02-03 at 12:06 -0500, Tejun Heo wrote:
> > > On Wed, Feb 03, 2016 at 06:01:53PM +0100, Mike Galbraith wrote:
> > > > Hm, so it's ok to queue work to an offline CPU?  What happens if it
> > > > doesn't come back for an eternity or two?
> > > 
> > > Right now, it just loses affinity....
> > 
> > WRT affinity...
> > 
> > Somebody somewhere queues a delayed work, a timer is started on CPUX,
> > work is targeted at CPUX.  Now wash/rinse/repeat mod_delayed_work()
> > along with migrations.  Should __queue_delayed_work() not refrain from
> > altering dwork->cpu once set?
> > 
> > I'm also wondering why 22b886dd only applies to kernels >= 4.2.
> > 
> > <quote>
> > Regardless of the previous CPU a timer was on, add_timer_on()
> > currently simply sets timer->flags to the new CPU.  As the caller must
> > be seeing the timer as idle, this is locally fine, but the timer
> > leaving the old base while unlocked can lead to race conditions as
> > follows.
> > 
> > Let's say timer was on cpu 0.
> > 
> >   cpu 0                                 cpu 1
> >   -----------------------------------------------------------------------------
> >   del_timer(timer) succeeds
> >                                         del_timer(timer)
> >                                           lock_timer_base(timer) locks cpu_0_base
> >   add_timer_on(timer, 1)
> >     spin_lock(&cpu_1_base->lock)
> >     timer->flags set to cpu_1_base
> >     operates on @timer                    operates on @timer
> > </quote>
> > 
> > What's the difference between...
> >      timer->flags = (timer->flags & ~TIMER_BASEMASK) | cpu;
> > and...
> >      timer_set_base(timer, base);
> > 
> > ...that makes that fix unneeded prior to 4.2?  We take the same locks
> > in < 4.2 kernels, so seemingly both will diddle concurrently above.
> 
> Indeed, you are right.
> 
> The same can happen on pre 4.2, just the fix does not apply as we changed the
> internals how the base is managed in the timer itself. Backport below.

Thanks for backport Thomas and to Mike for persistence :). I've asked my
friend seeing crashes with 3.18.25 to try whether this patch fixes the
issues. It may take some time so stay tuned...

								Honza

> 8<----------------------------
> 
> --- a/kernel/time/timer.c
> +++ b/kernel/time/timer.c
> @@ -956,13 +956,26 @@ EXPORT_SYMBOL(add_timer);
>   */
>  void add_timer_on(struct timer_list *timer, int cpu)
>  {
> -	struct tvec_base *base = per_cpu(tvec_bases, cpu);
> +	struct tvec_base *new_base = per_cpu(tvec_bases, cpu);
> +	struct tvec_base *base;
>  	unsigned long flags;
>  
>  	timer_stats_timer_set_start_info(timer);
>  	BUG_ON(timer_pending(timer) || !timer->function);
> -	spin_lock_irqsave(&base->lock, flags);
> -	timer_set_base(timer, base);
> +
> +	/*
> +	 * If @timer was on a different CPU, it must be migrated with the
> +	 * old base locked to prevent other operations proceeding with the
> +	 * wrong base locked.  See lock_timer_base().
> +	 */
> +	base = lock_timer_base(timer, &flags);
> +	if (base != new_base) {
> +		timer_set_base(timer, NULL);
> +		spin_unlock(&base->lock);
> +		base = new_base;
> +		spin_lock(&base->lock);
> +		timer_set_base(timer, base);
> +	}
>  	debug_activate(timer, timer->expires);
>  	internal_add_timer(base, timer);
>  	spin_unlock_irqrestore(&base->lock, flags);
> 
> 
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[toc] | [prev] | [next] | [standalone]

#1327011

On Thu, 4 Feb 2016 12:20:44 +0100
Jan Kara <jack@suse.cz> wrote:

> Thanks for backport Thomas and to Mike for persistence :). I've asked my
> friend seeing crashes with 3.18.25 to try whether this patch fixes the
> issues. It may take some time so stay tuned...

Patch tested and it really fixes the crash we were experiencing on 3.18.25
with commit 874bbfe+. But it seem to introduce (rather scary) regression.
Tested host shows abnormal cpu usage in both kernel and userland under the
same load and traffic pattern. One picture is worth a thousand words, so
I've taken snapshots of our graphs, see here:
http://neosystem.cz/test/linux-3.18.25/
The host was running 3.18.25 with commit 874bbfe+ (1e7af29+ on
3.18-stable) reverted. With this commit included, it crashed within
minutes. Around 13:30 we booted 3.18.25 with commit 874bbfe+ included and
with the patch from Thomas. And around 15:40 we've booted the host with
previous kernel, just to ensure this abnormal behaviour was really caused
by the test kernel.
Also interesting, in addition to high cpu usage, there is abnormally high
number of zombie processes reported by the system.

HTH.

--
						Daniel Bilik
						neosystem.cz

[toc] | [prev] | [next] | [standalone]

#1327421

On Thu, 2016-02-04 at 17:39 +0100, Daniel Bilik wrote:
> On Thu, 4 Feb 2016 12:20:44 +0100
> Jan Kara <jack@suse.cz> wrote:
> 
> > Thanks for backport Thomas and to Mike for persistence :). I've asked my
> > friend seeing crashes with 3.18.25 to try whether this patch fixes the
> > issues. It may take some time so stay tuned...
> 
> Patch tested and it really fixes the crash we were experiencing on 3.18.25
> with commit 874bbfe+. But it seem to introduce (rather scary) regression.
> Tested host shows abnormal cpu usage in both kernel and userland under the
> same load and traffic pattern. One picture is worth a thousand words, so
> I've taken snapshots of our graphs, see here:
> http://neosystem.cz/test/linux-3.18.25/
> The host was running 3.18.25 with commit 874bbfe+ (1e7af29+ on
> 3.18-stable) reverted. With this commit included, it crashed within
> minutes. Around 13:30 we booted 3.18.25 with commit 874bbfe+ included and
> with the patch from Thomas. And around 15:40 we've booted the host with
> previous kernel, just to ensure this abnormal behaviour was really caused
> by the test kernel.
> Also interesting, in addition to high cpu usage, there is abnormally high
> number of zombie processes reported by the system.

IMHO you should restore the CC list and re-post.  (If I were the
maintainer of either the workqueue code or 3.18-stable, I'd be highly
interested in this finding).

	-Mike

[toc] | [prev] | [next] | [standalone]

#1327529

On Fri, 05 Feb 2016 03:40:46 +0100
Mike Galbraith <umgwanakikbuti@gmail.com> wrote:

> On Thu, 2016-02-04 at 17:39 +0100, Daniel Bilik wrote:
> > On Thu, 4 Feb 2016 12:20:44 +0100
> > Jan Kara <jack@suse.cz> wrote:
> > 
> > > Thanks for backport Thomas and to Mike for persistence :). I've
> > > asked my friend seeing crashes with 3.18.25 to try whether this
> > > patch fixes the issues. It may take some time so stay tuned...
> > 
> > Patch tested and it really fixes the crash we were experiencing on
> > 3.18.25 with commit 874bbfe+. But it seem to introduce (rather scary)
> > regression. Tested host shows abnormal cpu usage in both kernel and
> > userland under the same load and traffic pattern. One picture is worth
> > a thousand words, so I've taken snapshots of our graphs, see here:
> > http://neosystem.cz/test/linux-3.18.25/
> > The host was running 3.18.25 with commit 874bbfe+ (1e7af29+ on
> > 3.18-stable) reverted. With this commit included, it crashed within
> > minutes. Around 13:30 we booted 3.18.25 with commit 874bbfe+ included
> > and with the patch from Thomas. And around 15:40 we've booted the host
> > with previous kernel, just to ensure this abnormal behaviour was
> > really caused by the test kernel.
> > Also interesting, in addition to high cpu usage, there is abnormally
> > high number of zombie processes reported by the system.
> 
> IMHO you should restore the CC list and re-post.  (If I were the
> maintainer of either the workqueue code or 3.18-stable, I'd be highly
> interested in this finding).

Sorry, I haven't realized tha patch proposed by Thomas is already on its
way to stable. CC restored and re-posting.

--
						Daniel Bilik
						neosystem.cz

[toc] | [prev] | [next] | [standalone]

#1327533

On Fri, 2016-02-05 at 09:11 +0100, Daniel Bilik wrote:
> On Fri, 05 Feb 2016 03:40:46 +0100
> Mike Galbraith <umgwanakikbuti@gmail.com> wrote:

> > IMHO you should restore the CC list and re-post.  (If I were the
> > maintainer of either the workqueue code or 3.18-stable, I'd be highly
> > interested in this finding).
> 
> Sorry, I haven't realized tha patch proposed by Thomas is already on its
> way to stable. CC restored and re-posting.

I don't know where it's at, but where things stand is that it is
needed, but when combined with the patch which at least uncovered the
fact that it's needed, the two aren't playing well together according
to your test result.  Given both patches are already in kernels
upstream, and presumably Thomas's patch will eventually wander to
stable to fix them up, there might be some maintainer interest.

	-Mike

[toc] | [prev] | [next] | [standalone]

#1325831

On Wed, 3 Feb 2016, 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.
> 
> > 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?  vmstat would be the canary case as it runs
> frequently on all systems.  It's exactly the sign that we can't break
> this guarantee willy-nilly.

You're in complete failure denial mode once again.

Fact is:

  That patch breaks stuff because there is no stable cpu -> node mapping
  accross cpu on/offlining. As a result this selects unbound_pwq_by_node() on
  node -1.

The reason why you need to do that work->cpu assignment might be legitimate,
but that does not justify that you expose systems to a lurking out of bounds
access which results in a NULL pointer dereference.

As long as cpu_to_node(cpu) can return -1, we need a sanity check there. And
we need that now and not at some point in the future when the patches
establishing a stable cpu -> node mapping are finished.

Stop arguing around a bug which really exists and was exposed by this patch.

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1325872

On Wed, 3 Feb 2016, Tejun Heo wrote:
> On Wed, Feb 03, 2016 at 07:46:11PM +0100, Thomas Gleixner wrote:
> > > > 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?  vmstat would be the canary case as it runs
> > > frequently on all systems.  It's exactly the sign that we can't break
> > > this guarantee willy-nilly.
> > 
> > You're in complete failure denial mode once again.
> 
> Well, you're in an unnecessary escalation mode as usual.  Was the
> attitude really necessary?  Chill out and read the thread again.
> Michal is saying the dwork->cpu assignment was bogus and I was
> refuting that.

Right, but at the same time you could have admitted, that the current state is
buggy and needs a sanity check in unbound_pwq_by_node().
 
> Michal brought it up here but there's a different thread where Mike
> reported NUMA_NO_NODE issue and I already posted the fix.
> 
>  http://lkml.kernel.org/g/20160203185425.GK14091@mtj.duckdns.org

5 minute ago w/o cc'ing the people who participated in that discussion.

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1325881

On Wed, Feb 03, 2016 at 08:05:57PM +0100, Thomas Gleixner wrote:
> > Well, you're in an unnecessary escalation mode as usual.  Was the
> > attitude really necessary?  Chill out and read the thread again.
> > Michal is saying the dwork->cpu assignment was bogus and I was
> > refuting that.
>
> Right, but at the same time you could have admitted, that the current state is
> buggy and needs a sanity check in unbound_pwq_by_node().

It's crashing.  Of course it's buggy.  The main discussion on the bug
was on the other thread and I was trying to put out the confusions
posted on this thread.

> > Michal brought it up here but there's a different thread where Mike
> > reported NUMA_NO_NODE issue and I already posted the fix.
> > 
> >  http://lkml.kernel.org/g/20160203185425.GK14091@mtj.duckdns.org
> 
> 5 minute ago w/o cc'ing the people who participated in that discussion.

Yeah, I got to the thread this morning and got your email right after
sending out the patch.  I don't know.  The handling of this wasn't out
of the norm.  I asked this multiple times but let me try again.  Can
we please try to stay civil and technical?  There are times where
escalation is necessary but this one was gratuitous.

Thanks.

-- 
tejun

[toc] | [prev] | [next] | [standalone]

#1325873

Hello, Thomas.

On Wed, Feb 03, 2016 at 07:46:11PM +0100, Thomas Gleixner wrote:
> > > 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?  vmstat would be the canary case as it runs
> > frequently on all systems.  It's exactly the sign that we can't break
> > this guarantee willy-nilly.
> 
> You're in complete failure denial mode once again.

Well, you're in an unnecessary escalation mode as usual.  Was the
attitude really necessary?  Chill out and read the thread again.
Michal is saying the dwork->cpu assignment was bogus and I was
refuting that.

> Fact is:
> 
>   That patch breaks stuff because there is no stable cpu -> node mapping
>   accross cpu on/offlining. As a result this selects unbound_pwq_by_node() on
>   node -1.
> 
> The reason why you need to do that work->cpu assignment might be legitimate,
> but that does not justify that you expose systems to a lurking out of bounds
> access which results in a NULL pointer dereference.
> 
> As long as cpu_to_node(cpu) can return -1, we need a sanity check there. And
> we need that now and not at some point in the future when the patches
> establishing a stable cpu -> node mapping are finished.
> 
> Stop arguing around a bug which really exists and was exposed by this patch.

Michal brought it up here but there's a different thread where Mike
reported NUMA_NO_NODE issue and I already posted the fix.

 http://lkml.kernel.org/g/20160203185425.GK14091@mtj.duckdns.org

Thanks.

-- 
tejun

[toc] | [prev] | [next] | [standalone]

#1327499

On Wed, 2016-02-03 at 11:24 -0500, 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.
> 
> > 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?  vmstat would be the canary case as it runs
> frequently on all systems.  It's exactly the sign that we can't break
> this guarantee willy-nilly.

If the intent of the below is to fulfill a guarantee...

+       /* timer isn't guaranteed to run in this cpu, record earlier */
+       if (cpu == WORK_CPU_UNBOUND)
+               cpu = raw_smp_processor_id();
        dwork->cpu = cpu;
        timer->expires = jiffies + delay;
  
-       if (unlikely(cpu != WORK_CPU_UNBOUND))
-               add_timer_on(timer, cpu);
-       else
-               add_timer(timer);
+       add_timer_on(timer, cpu);

...it appears to be incomplete.  Hotplug aside, when adding a timer
with the expectation that it stay put, should it not also be pinned?

	-Mike

[toc] | [prev] | [standalone]

Page 2 of 2 — ← Prev page 1 [2]

Back to top | Article view | linux.kernel

csiph-web