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 1 of 2  [1] 2  Next page →

#1325090 — Re: Crashes with 874bbfe600a6 in 3.18.25

On 01/26/2016, 02:09 PM, Thomas Gleixner wrote:
> On Tue, 26 Jan 2016, Petr Mladek wrote:
>> On Tue 2016-01-26 10:34:00, Jan Kara wrote:
>>> On Sat 23-01-16 17:11:54, Thomas Gleixner wrote:
>>>> On Sat, 23 Jan 2016, Ben Hutchings wrote:
>>>>> On Fri, 2016-01-22 at 11:09 -0500, Tejun Heo wrote:
>>>>>>> Looks like it requires more than trivial backport (I think). Tejun?
>>>>>>
>>>>>> The timer migration has changed quite a bit.  Given that we've never
>>>>>> seen vmstat work crashing in 3.18 era, I wonder whether the right
>>>>>> thing to do here is reverting 874bbfe600a6 from 3.18 stable?
>>>>>
>>>>> It's not just 3.18 that has this; 874bbfe600a6 was backported to all
>>>>> stable branches from 3.10 onward.  Only the 4.2-ckt branch has
>>>>> 22b886dd10180939.
>>>>
>>>> 22b886dd10180939 fixes a bug which was introduced with the timer wheel
>>>> overhaul in 4.2. So only 4.2/3 should have it backported.
>>>
>>> Thanks for explanation. So do I understand right that timers are always run
>>> on the calling CPU in kernels prior to 4.2 and thus commit 874bbfe600a6 (to
>>> run timer for delayed work on the calling CPU) doesn't make sense there? If
>>> that is true than reverting the commit from older stable kernels is
>>> probably the easiest way to resolve the crashes.
>>
>> The commit 874bbfe600a6 ("workqueue: make sure delayed work run in
>> local cpu") forces the timer to run on the local CPU. It might be correct
>> for vmstat. But I wonder if it might break some other delayed work
>> user that depends on running on different CPU.
> 
> The default of add_timer() is to run on the current cpu. It only moves the
> timer to a different cpu when the power saving code says so. So 874bbfe600a6
> enforces that the timer runs on the cpu on which queue_delayed_work() is
> called, but before that commit it was likely that the timer was queued on the
> calling cpu. So there is nothing which can depend on running on a different
> CPU, except callers of queue_delayed_work_on() which provide the target cpu
> explicitely. 874bbfe600a6 does not affect those callers at all.
> 
> Now, what's different is:
> 
> +       if (cpu == WORK_CPU_UNBOUND)
> +               cpu = raw_smp_processor_id();
>         dwork->cpu = cpu;
> 
> So before that change dwork->cpu was set to WORK_CPU_UNBOUND. Now it's set to
> the current cpu, but I can't see how that matters.

What happens in later kernels, when the cpu is offlined before the
delayed_work timer ticks? In stable 3.12, with the patch, this  scenario
results in an oops:
 #5 [ffff8c03fdd63d80] page_fault at ffffffff81523a88
    [exception RIP: __queue_work+121]
    RIP: ffffffff81071989  RSP: ffff8c03fdd63e30  RFLAGS: 00010086
    RAX: ffff88048b96bc00  RBX: ffff8c03e9bcc800  RCX: ffff880473820478
    RDX: 0000000000000400  RSI: 0000000000000004  RDI: ffff880473820458
    RBP: 0000000000000000   R8: ffff8c03fdd71f40   R9: ffff8c03ea4c4002
    R10: 0000000000000000  R11: 0000000000000005  R12: ffff880473820458
    R13: 00000000000000a8  R14: 000000000000e328  R15: 00000000000000a8
    ORIG_RAX: ffffffffffffffff  CS: 0010  SS: 0018
 #6 [ffff8c03fdd63e68] call_timer_fn at ffffffff81065611
 #7 [ffff8c03fdd63e98] run_timer_softirq at ffffffff810663b7
 #8 [ffff8c03fdd63f00] __do_softirq at ffffffff8105e2c5
 #9 [ffff8c03fdd63f68] call_softirq at ffffffff8152cf9c
#10 [ffff8c03fdd63f80] do_softirq at ffffffff81004665
#11 [ffff8c03fdd63fa0] smp_apic_timer_interrupt at ffffffff8152d835
#12 [ffff8c03fdd63fb0] apic_timer_interrupt at ffffffff8152c2dd

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

Any ideas?

thanks,
-- 
js
suse labs

[toc] | [next] | [standalone]

#1325182

On Wed, 3 Feb 2016, Jiri Slaby wrote:
> On 01/26/2016, 02:09 PM, Thomas Gleixner wrote:
> What happens in later kernels, when the cpu is offlined before the
> delayed_work timer ticks? In stable 3.12, with the patch, this  scenario
> results in an oops:
>  #5 [ffff8c03fdd63d80] page_fault at ffffffff81523a88
>     [exception RIP: __queue_work+121]
>     RIP: ffffffff81071989  RSP: ffff8c03fdd63e30  RFLAGS: 00010086
>     RAX: ffff88048b96bc00  RBX: ffff8c03e9bcc800  RCX: ffff880473820478
>     RDX: 0000000000000400  RSI: 0000000000000004  RDI: ffff880473820458
>     RBP: 0000000000000000   R8: ffff8c03fdd71f40   R9: ffff8c03ea4c4002
>     R10: 0000000000000000  R11: 0000000000000005  R12: ffff880473820458
>     R13: 00000000000000a8  R14: 000000000000e328  R15: 00000000000000a8
>     ORIG_RAX: ffffffffffffffff  CS: 0010  SS: 0018
>  #6 [ffff8c03fdd63e68] call_timer_fn at ffffffff81065611
>  #7 [ffff8c03fdd63e98] run_timer_softirq at ffffffff810663b7
>  #8 [ffff8c03fdd63f00] __do_softirq at ffffffff8105e2c5
>  #9 [ffff8c03fdd63f68] call_softirq at ffffffff8152cf9c
> #10 [ffff8c03fdd63f80] do_softirq at ffffffff81004665
> #11 [ffff8c03fdd63fa0] smp_apic_timer_interrupt at ffffffff8152d835
> #12 [ffff8c03fdd63fb0] apic_timer_interrupt at ffffffff8152c2dd
> 
> 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

I don't see how that works on later kernels. If cpu_to_node() returns -1 we
access outside of the array bounds....

Thanks,

	tglx

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

#1325330

[I wasn't aware of this email thread before so I am jumping in late]

On Wed 03-02-16 10:35:32, Jiri Slaby wrote:
> On 01/26/2016, 02:09 PM, Thomas Gleixner wrote:
> > On Tue, 26 Jan 2016, Petr Mladek wrote:
[...]
> >> The commit 874bbfe600a6 ("workqueue: make sure delayed work run in
> >> local cpu") forces the timer to run on the local CPU. It might be correct
> >> for vmstat. But I wonder if it might break some other delayed work
> >> user that depends on running on different CPU.
> > 
> > The default of add_timer() is to run on the current cpu. It only moves the
> > timer to a different cpu when the power saving code says so. So 874bbfe600a6
> > enforces that the timer runs on the cpu on which queue_delayed_work() is
> > called, but before that commit it was likely that the timer was queued on the
> > calling cpu. So there is nothing which can depend on running on a different
> > CPU, except callers of queue_delayed_work_on() which provide the target cpu
> > explicitely. 874bbfe600a6 does not affect those callers at all.
> > 
> > Now, what's different is:
> > 
> > +       if (cpu == WORK_CPU_UNBOUND)
> > +               cpu = raw_smp_processor_id();
> >         dwork->cpu = cpu;
> > 
> > So before that change dwork->cpu was set to WORK_CPU_UNBOUND. Now it's set to
> > the current cpu, but I can't see how that matters.

It matters because if somebody did queue_delayed_work() and the
current cpu gets offlined then even though the associated timer gets
migrated the __queue_work wouldn't recognize the associated cpu as
WORK_CPU_UNBOUND anymore and won't reset the following path will go
kaboom...

> 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 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.

-- 
Michal Hocko
SUSE Labs

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

#1325615

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.

Thanks.

-- 
tejun

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

#1325659

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

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

#1325690

On Wed, Feb 03, 2016 at 05:48:52PM +0100, Michal Hocko wrote:
> > 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

Whether a memory node is offlined or not doesn't affect how cpus map
to the node.  The mapping is something which is fixed at physical and
firmware level throughout while the system is running.  If the node
becomes memory-less what changes is the memory allocation strategy for
the node, not how cpus map to nodes.  The only problem here is that we
currently lose how we mapped logical IDs to physical ones across
off/online cycles.

> 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.

delayed_work->cpu is determined on queueing time.  Dealing with
offlined cpus at execution is completley fine.  There's no need to
"preserve" anything.

> > 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).

What you said just doesn't fit the reality.  Again, think about why
vmstat crashed.  Why is this difficult to understand?

-- 
tejun

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

#1326439

On Wed 03-02-16 11:59:01, Tejun Heo wrote:
> On Wed, Feb 03, 2016 at 05:48:52PM +0100, Michal Hocko wrote:
[...]
> > 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.
> 
> delayed_work->cpu is determined on queueing time.  Dealing with
> offlined cpus at execution is completley fine.  There's no need to
> "preserve" anything.

I've seen you have posted a fix in the mean time but just for my
understading. Why the following is not an appropriate fix?

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index c579dbab2e36..52bb11cf20d1 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -1459,9 +1459,9 @@ static void __queue_delayed_work(int cpu, struct workqueue_struct *wq,
 
 	dwork->wq = wq;
 	/* timer isn't guaranteed to run in this cpu, record earlier */
+	dwork->cpu = cpu;
 	if (cpu == WORK_CPU_UNBOUND)
 		cpu = raw_smp_processor_id();
-	dwork->cpu = cpu;
 	timer->expires = jiffies + delay;
 
 	add_timer_on(timer, cpu);

Thanks!
-- 
Michal Hocko
SUSE Labs

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

#1326461

On Thu 04-02-16 07:37:23, Michal Hocko wrote:
> On Wed 03-02-16 11:59:01, Tejun Heo wrote:
> > On Wed, Feb 03, 2016 at 05:48:52PM +0100, Michal Hocko wrote:
> [...]
> > > 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.
> > 
> > delayed_work->cpu is determined on queueing time.  Dealing with
> > offlined cpus at execution is completley fine.  There's no need to
> > "preserve" anything.
> 
> I've seen you have posted a fix in the mean time but just for my
> understading. Why the following is not an appropriate fix?
> 
> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> index c579dbab2e36..52bb11cf20d1 100644
> --- a/kernel/workqueue.c
> +++ b/kernel/workqueue.c
> @@ -1459,9 +1459,9 @@ static void __queue_delayed_work(int cpu, struct workqueue_struct *wq,
>  
>  	dwork->wq = wq;
>  	/* timer isn't guaranteed to run in this cpu, record earlier */
> +	dwork->cpu = cpu;
>  	if (cpu == WORK_CPU_UNBOUND)
>  		cpu = raw_smp_processor_id();
> -	dwork->cpu = cpu;
>  	timer->expires = jiffies + delay;
>  
>  	add_timer_on(timer, cpu);

Ok, so after some more thinking about that, this won't really help for
memory less CPU which would still have NUMA_NO_NODE associated with it
AFAIU. So this is definitely better to be handled at unbound_pwq_by_node
level.
-- 
Michal Hocko
SUSE Labs

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

#1325699

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.

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?

	-Mike

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

#1325701

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.  A more interesting case is a cpu
going offline whlie work items bound to the cpu are still running and
the root problem is that we've never distinguished between affinity
for correctness and optimization and thus can't flush or warn on the
stagglers.  The plan is to ensure that all correctness users specify
the CPU explicitly.  Once we're there, we can warn on illegal usages.

Thanks.

-- 
tejun

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

#1325712

On Wed, Feb 03, 2016 at 06:13:15PM +0100, Mike Galbraith wrote:
> Ah, and the rest (the vast majority) can then be safely deflected away
> from nohz_full cpus.

Yeap, it should be possible to bounce majority of work items across
CPUs all we want.

-- 
tejun

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

#1325713

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.  A more interesting case is a cpu
> going offline whlie work items bound to the cpu are still running and
> the root problem is that we've never distinguished between affinity
> for correctness and optimization and thus can't flush or warn on the
> stagglers.  The plan is to ensure that all correctness users specify
> the CPU explicitly.  Once we're there, we can warn on illegal usages.

Ah, and the rest (the vast majority) can then be safely deflected away
from nohz_full cpus.

	-Mike

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

#1326346

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.  A more interesting case is a cpu
> going offline whlie work items bound to the cpu are still running and
> the root problem is that we've never distinguished between affinity
> for correctness and optimization and thus can't flush or warn on the
> stagglers.  The plan is to ensure that all correctness users specify
> the CPU explicitly.  Once we're there, we can warn on illegal usages.

Isn't it the case that, currently at least, each and every spot that
requires execution on a specific CPU yet does not take active measures
to deal with hotplug events is in fact buggy?  The timer code clearly
states that the user is responsible, and so do both workqueue.[ch].

I was surprised me to hear that some think they have an iron clad
guarantee, given the null and void clause is prominently displayed.

	-Mike

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

#1327934

Hello, Mike.

On Thu, Feb 04, 2016 at 03:00:17AM +0100, Mike Galbraith wrote:
> Isn't it the case that, currently at least, each and every spot that
> requires execution on a specific CPU yet does not take active measures
> to deal with hotplug events is in fact buggy?  The timer code clearly
> states that the user is responsible, and so do both workqueue.[ch].

Yeah, the usages which require affinity for correctness must flush the
work items from a cpu down callback.

> I was surprised me to hear that some think they have an iron clad
> guarantee, given the null and void clause is prominently displayed.

Nobody is (or at least should be) expecting workqueue to handle
affinity across CPU offlining events.  That is not the problem.  The
problem is that currently queue_work(work) and
queue_work_on(smp_processor_id(), work) are identical and there likely
are affinity-for-correctness users which are doing the former.

Thanks.

-- 
tejun

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

#1328061

On Fri, 2016-02-05 at 11:49 -0500, Tejun Heo wrote:
> Hello, Mike.
> 
> On Thu, Feb 04, 2016 at 03:00:17AM +0100, Mike Galbraith wrote:
> > Isn't it the case that, currently at least, each and every spot that
> > requires execution on a specific CPU yet does not take active measures
> > to deal with hotplug events is in fact buggy?  The timer code clearly
> > states that the user is responsible, and so do both workqueue.[ch].
> 
> Yeah, the usages which require affinity for correctness must flush the
> work items from a cpu down callback.

Good, we agree.  Now bear with me a moment..

That very point is what makes it wrong for the workqueue code to ever
target a work item.  The instant it does target selection, correctness
may be at stake, it doesn't know, thus it must assume the full onus,
which it has neither the knowledge not the time to do.  That's how we
exploded on node = -1, trying to help out the user by doing his job,
but then not doing the whole job.  IMHO, a better plan is to let the
user screw it up all by himself.

	-Mike

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

#1328067

Hello, Mike.

On Fri, Feb 05, 2016 at 09:47:11PM +0100, Mike Galbraith wrote:
> That very point is what makes it wrong for the workqueue code to ever
> target a work item.  The instant it does target selection, correctness
> may be at stake, it doesn't know, thus it must assume the full onus,
> which it has neither the knowledge not the time to do.  That's how we
> exploded on node = -1, trying to help out the user by doing his job,

I have a hard time seeing the NUMA_NO_NODE bug as something that
indicative of anything.  It is a dumb bug from mm side which puts
everyone using cpu_to_node() at risk.

> but then not doing the whole job.  IMHO, a better plan is to let the
> user screw it up all by himself.

What are you suggesting?

Thanks.

-- 
tejun

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

#1328070

On Fri, Feb 05, 2016 at 09:59:49PM +0100, Mike Galbraith wrote:
> On Fri, 2016-02-05 at 15:54 -0500, Tejun Heo wrote:
> 
> > What are you suggesting?
> 
> That 874bbfe6 should die.

Yeah, it's gonna be killed.  The commit is there because the behavior
change broke things.  We don't want to guarantee it but have been and
can't change it right away just because we don't like it when things
may break from it.  The plan is to implement a debug option to force
workqueue to always execute these work items on a foreign cpu to weed
out breakages.

Thanks.

-- 
tejun

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

#1328298

On Fri, 05 Feb 2016, Tejun Heo wrote:
> On Fri, Feb 05, 2016 at 09:59:49PM +0100, Mike Galbraith wrote:
> > On Fri, 2016-02-05 at 15:54 -0500, Tejun Heo wrote:
> > 
> > > What are you suggesting?
> > 
> > That 874bbfe6 should die.
> 
> Yeah, it's gonna be killed.  The commit is there because the behavior
> change broke things.  We don't want to guarantee it but have been and
> can't change it right away just because we don't like it when things
> may break from it.  The plan is to implement a debug option to force
> workqueue to always execute these work items on a foreign cpu to weed
> out breakages.

Is there a path to filter down sane behavior (whichever one it might be) to
the affected stable/LTS kernels?

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

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

#1328413

On Sat, 2016-02-06 at 11:07 -0200, Henrique de Moraes Holschuh wrote:
> On Fri, 05 Feb 2016, Tejun Heo wrote:
> > On Fri, Feb 05, 2016 at 09:59:49PM +0100, Mike Galbraith wrote:
> > > On Fri, 2016-02-05 at 15:54 -0500, Tejun Heo wrote:
> > > 
> > > > What are you suggesting?
> > > 
> > > That 874bbfe6 should die.
> > 
> > Yeah, it's gonna be killed.  The commit is there because the behavior
> > change broke things.  We don't want to guarantee it but have been and
> > can't change it right away just because we don't like it when things
> > may break from it.  The plan is to implement a debug option to force
> > workqueue to always execute these work items on a foreign cpu to weed
> > out breakages.
> 
> Is there a path to filter down sane behavior (whichever one it might be) to
> the affected stable/LTS kernels?

What Michal said, replace 874bbfe6 with 176bed1d.  Without 22b886dd,
874bbfe6 is a landmine, uses add_timer_on() as if it were mod_timer(),
which it is not, or rather was not until 22b886dd came along, and still
does not look like the mod_timer() alias that add_timer() is.

	-Mike

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

#1328417

On Sun, 2016-02-07 at 06:19 +0100, Mike Galbraith wrote:
> On Sat, 2016-02-06 at 11:07 -0200, Henrique de Moraes Holschuh wrote:
> > On Fri, 05 Feb 2016, Tejun Heo wrote:
> > > On Fri, Feb 05, 2016 at 09:59:49PM +0100, Mike Galbraith wrote:
> > > > On Fri, 2016-02-05 at 15:54 -0500, Tejun Heo wrote:
> > > > 
> > > > > What are you suggesting?
> > > > 
> > > > That 874bbfe6 should die.
> > > 
> > > Yeah, it's gonna be killed.  The commit is there because the behavior
> > > change broke things.  We don't want to guarantee it but have been and
> > > can't change it right away just because we don't like it when things
> > > may break from it.  The plan is to implement a debug option to force
> > > workqueue to always execute these work items on a foreign cpu to weed
> > > out breakages.
> > 
> > Is there a path to filter down sane behavior (whichever one it might be) to
> > the affected stable/LTS kernels?
> 
> What Michal said, replace 874bbfe6 with 176bed1d.  Without 22b886dd,
> 874bbfe6 is a landmine, uses add_timer_on() as if it were mod_timer(),
> which it is not, or rather was not until 22b886dd came along, and still
> does not look like the mod_timer() alias that add_timer() is.

BTW, with the 874bbfe6 22b886dd pair, mundane workqueue timers are no
longer deflected to housekeeper CPUs, so NO_HZ_FULL regresses.

	-Mike

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

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web