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

Groups > linux.kernel > #1330362 > 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 Mike Galbraith <umgwanakikbuti@gmail.com> - 2016-02-09 16:40 +0100
    Re: Crashes with 874bbfe600a6 in 3.18.25 Linus Torvalds <torvalds@linux-foundation.org> - 2016-02-09 17:40 +0100
      Re: Crashes with 874bbfe600a6 in 3.18.25 Tejun Heo <tj@kernel.org> - 2016-02-09 18:00 +0100
        Re: Crashes with 874bbfe600a6 in 3.18.25 Mike Galbraith <umgwanakikbuti@gmail.com> - 2016-02-09 18:10 +0100
          Re: Crashes with 874bbfe600a6 in 3.18.25 Mike Galbraith <umgwanakikbuti@gmail.com> - 2016-02-09 19:00 +0100
            Re: Crashes with 874bbfe600a6 in 3.18.25 Mike Galbraith <umgwanakikbuti@gmail.com> - 2016-02-09 19:10 +0100
              Re: Crashes with 874bbfe600a6 in 3.18.25 Tejun Heo <tj@kernel.org> - 2016-02-09 19:30 +0100
          Re: Crashes with 874bbfe600a6 in 3.18.25 Tejun Heo <tj@kernel.org> - 2016-02-09 19:00 +0100
        Re: Crashes with 874bbfe600a6 in 3.18.25 Linus Torvalds <torvalds@linux-foundation.org> - 2016-02-09 18:10 +0100
          Re: Crashes with 874bbfe600a6 in 3.18.25 Tejun Heo <tj@kernel.org> - 2016-02-09 19:00 +0100
            Re: Crashes with 874bbfe600a6 in 3.18.25 Linus Torvalds <torvalds@linux-foundation.org> - 2016-02-09 19:10 +0100

#1330362 — Re: Crashes with 874bbfe600a6 in 3.18.25

On Fri, 2016-02-05 at 16:06 -0500, 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.

A niggling question remaining is when is it gonna be killed?

1. Meanwhile, 874bbfe6 was sent to 2.6.31+, meaning that every stable
tree where it landed which did not ALSO receive 22b886dd has become
destabilized.  We have two 3.12-stability reports, one the hotplug
explosion that you provided a workaround for, one the corruption, and
one corruption report for 3.18.  Both breakage types would be sort of
fixed up by getting 22b886dd and your hotplug workaround (which does
_not_ guarantee survival) were applied everywhere, however...

2. We also have a report for the 3.18 corruption victim that adding
22b886dd did NOT restore the stable status quo, rather it replaced the
corruption that 874bbfe6 caused with a performance regression.

3. 874bbfe6 + 22b886dd also inflicts a NO_HZ_FULL regression. 
 Admittedly not a huge deal, but another regression nonetheless.

The only evidence I've seen that anything at all was the broken by the
changes that triggered the inception of 874bbfe6 in the first place was
the b0rked vmstat thing that Linus had already fixed with 176bed1d.  So
where is the breakage you mention that makes keeping 874bbfe6 the
prudent thing to do vs just reverting 874bbfe6 immediately, perhaps
22b886dd as well given it is fallout thereof, and getting that sent off
to stable?

It looks for all the world as if the sole excuse for either to exist is
to prevent any other stupid mistakes like the vmstat thing from being
exposed for what they are by actively hiding them, when in fact, that
hiding doesn't survive a hotplug event (as we saw in the crash analysis
I showed you).  Surely there's a better reason to keep that commit than
hiding bugs that can only remain hidden until they meet hotplug.  What
is it?

	-Mike

[toc] | [next] | [standalone]

#1330449

On Tue, Feb 9, 2016 at 7:31 AM, Mike Galbraith <umgwanakikbuti@gmail.com> wrote:
> On Fri, 2016-02-05 at 16:06 -0500, Tejun Heo wrote:
>> >
>> > 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.
>
> A niggling question remaining is when is it gonna be killed?

It probably should be killed sooner rather than later.

Just document that if you need something to run on a _particular_ cpu,
you need to use "schedule_delayed_work_on()" and "add_timer_on()".

The proper fix was 176bed1de5bf, and 874bbfe6 was just wrong.

                 Linus

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

#1330472

Hello,

On Tue, Feb 09, 2016 at 08:39:15AM -0800, Linus Torvalds wrote:
> > A niggling question remaining is when is it gonna be killed?
> 
> It probably should be killed sooner rather than later.
> 
> Just document that if you need something to run on a _particular_ cpu,
> you need to use "schedule_delayed_work_on()" and "add_timer_on()".

I'll queue a patch to put unbound work items on foreign cpus (maybe
every Nth to reduce perf impact).  Wanted to align it to rc1 and then
let it get tested during the devel cycle but missed this window.  It's
a bit late in devel cycle but we can still do it in this cycle.

> The proper fix was 176bed1de5bf, and 874bbfe6 was just wrong.

idk, not doing so is likely to cause subtle bugs which are difficult
to track down.  The problem with -stable is 874bbfe6 being backported
without the matching timer fix.  The right thing to do now probably is
reverting 874bbfe6 for -stable kernels which don't get the timer fix.

Thanks.

-- 
tejun

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

#1330486

On Tue, 2016-02-09 at 11:50 -0500, Tejun Heo wrote:
> Hello,
> 
> On Tue, Feb 09, 2016 at 08:39:15AM -0800, Linus Torvalds wrote:
> > > A niggling question remaining is when is it gonna be killed?
> > 
> > It probably should be killed sooner rather than later.
> > 
> > Just document that if you need something to run on a _particular_
> > cpu,
> > you need to use "schedule_delayed_work_on()" and "add_timer_on()".
> 
> I'll queue a patch to put unbound work items on foreign cpus (maybe
> every Nth to reduce perf impact).  Wanted to align it to rc1 and then
> let it get tested during the devel cycle but missed this window.  It's
> a bit late in devel cycle but we can still do it in this cycle.

Or do something like the below, and get guinea pigs for free. 

workqueue: schedule WORK_CPU_UNBOUND work on wq_unbound_cpumask CPUs

WORK_CPU_UNBOUND work items queued to a bound workqueue always run
locally.  This is a good thing normally, but not when the user has
asked us to keep unbound work away from certain CPUs.  Round robin
these to wq_unbound_cpumask CPUs instead, as perturbation avoidance
trumps performance.

Signed-off-by: Mike Galbraith <umgwanakikbuti@gmail.com>
---
 kernel/workqueue.c |   27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -303,6 +303,9 @@ static bool workqueue_freezing;		/* PL:
 
 static cpumask_var_t wq_unbound_cpumask; /* PL: low level cpumask for all unbound wqs */
 
+/* CPU where WORK_CPU_UNBOUND work was last round robin scheduled from this CPU */
+static DEFINE_PER_CPU(unsigned int, wq_unbound_rr_cpu_last);
+
 /* the per-cpu worker pools */
 static DEFINE_PER_CPU_SHARED_ALIGNED(struct worker_pool [NR_STD_WORKER_POOLS],
 				     cpu_worker_pools);
@@ -1298,6 +1301,28 @@ static bool is_chained_work(struct workq
 	return worker && worker->current_pwq->wq == wq;
 }
 
+/*
+ * When queueing WORK_CPU_UNBOUND work to a !WQ_UNBOUND queue, round
+ * robin among wq_unbound_cpumask to avoid perturbing sensitive tasks.
+ */
+static unsigned int select_round_robin_cpu(unsigned int cpu)
+{
+	int new_cpu;
+
+	if (cpumask_test_cpu(cpu, wq_unbound_cpumask))
+		return cpu;
+	if (cpumask_empty(wq_unbound_cpumask))
+		return cpu;
+	new_cpu = __this_cpu_read(wq_unbound_rr_cpu_last);
+	new_cpu = cpumask_next_and(new_cpu, wq_unbound_cpumask, cpu_online_mask);
+	if (unlikely(new_cpu >= nr_cpu_ids))
+		new_cpu = cpumask_first_and(wq_unbound_cpumask, cpu_online_mask);
+	if (unlikely(WARN_ON_ONCE(new_cpu >= nr_cpu_ids)))
+		return cpu;
+	__this_cpu_write(wq_unbound_rr_cpu_last, new_cpu);
+	return new_cpu;
+}
+
 static void __queue_work(int cpu, struct workqueue_struct *wq,
 			 struct work_struct *work)
 {
@@ -1323,7 +1348,7 @@ static void __queue_work(int cpu, struct
 		return;
 retry:
 	if (req_cpu == WORK_CPU_UNBOUND)
-		cpu = raw_smp_processor_id();
+		cpu = select_round_robin_cpu(raw_smp_processor_id());
 
 	/* pwq which will be used unless @work is executing elsewhere */
 	if (!(wq->flags & WQ_UNBOUND))

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

#1330542

On Tue, 2016-02-09 at 12:54 -0500, Tejun Heo wrote:
> Hello, Mike.
> 
> On Tue, Feb 09, 2016 at 06:04:04PM +0100, Mike Galbraith wrote:
> > workqueue: schedule WORK_CPU_UNBOUND work on wq_unbound_cpumask
> > CPUs
> > 
> > WORK_CPU_UNBOUND work items queued to a bound workqueue always run
> > locally.  This is a good thing normally, but not when the user has
> > asked us to keep unbound work away from certain CPUs.  Round robin
> > these to wq_unbound_cpumask CPUs instead, as perturbation avoidance
> > trumps performance.
> 
> I don't think doing this by default for everyone is a good idea.  A
> lot of workqueue usages tend to touch whatever the scheduler was
> touching after all.  Doing things per-cpu is generally a pretty good
> thing.

It doesn't do anything unless the user twiddles the mask to exclude
certain (think no_hz_full) CPUs, so there are no clueless victims.

	-Mike

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

#1330557

On Tue, 2016-02-09 at 18:56 +0100, Mike Galbraith wrote:
> On Tue, 2016-02-09 at 12:54 -0500, Tejun Heo wrote:
> > Hello, Mike.
> > 
> > On Tue, Feb 09, 2016 at 06:04:04PM +0100, Mike Galbraith wrote:
> > > workqueue: schedule WORK_CPU_UNBOUND work on wq_unbound_cpumask
> > > CPUs
> > > 
> > > WORK_CPU_UNBOUND work items queued to a bound workqueue always
> > > run
> > > locally.  This is a good thing normally, but not when the user
> > > has
> > > asked us to keep unbound work away from certain CPUs.  Round
> > > robin
> > > these to wq_unbound_cpumask CPUs instead, as perturbation
> > > avoidance
> > > trumps performance.
> > 
> > I don't think doing this by default for everyone is a good idea.  A
> > lot of workqueue usages tend to touch whatever the scheduler was
> > touching after all.  Doing things per-cpu is generally a pretty
> > good
> > thing.
> 
> It doesn't do anything unless the user twiddles the mask to exclude
> certain (think no_hz_full) CPUs, so there are no clueless victims.

(a plus: testers/robots can twiddle mask to help find bugs, _and_
nohz_full people can use it if they so choose)

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

#1330575

Hello, Mike.

On Tue, Feb 09, 2016 at 07:02:35PM +0100, Mike Galbraith wrote:
> > It doesn't do anything unless the user twiddles the mask to exclude
> > certain (think no_hz_full) CPUs, so there are no clueless victims.
> 
> (a plus: testers/robots can twiddle mask to help find bugs, _and_
> nohz_full people can use it if they so choose)

Ah, yeah, it makes sense then.  I'm gonna play with a bit and add a
debug option to always force the behavior.

Thanks.

-- 
tejun

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

#1330545

Hello, Mike.

On Tue, Feb 09, 2016 at 06:04:04PM +0100, Mike Galbraith wrote:
> workqueue: schedule WORK_CPU_UNBOUND work on wq_unbound_cpumask CPUs
> 
> WORK_CPU_UNBOUND work items queued to a bound workqueue always run
> locally.  This is a good thing normally, but not when the user has
> asked us to keep unbound work away from certain CPUs.  Round robin
> these to wq_unbound_cpumask CPUs instead, as perturbation avoidance
> trumps performance.

I don't think doing this by default for everyone is a good idea.  A
lot of workqueue usages tend to touch whatever the scheduler was
touching after all.  Doing things per-cpu is generally a pretty good
thing.

Thanks.

-- 
tejun

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

#1330493

On Tue, Feb 9, 2016 at 8:50 AM, Tejun Heo <tj@kernel.org> wrote:
>
> idk, not doing so is likely to cause subtle bugs which are difficult
> to track down.  The problem with -stable is 874bbfe6 being backported
> without the matching timer fix.

Well, according to this thread, even witht he timer fix the end result
then shows odd problems, _and_ has a NO_HZ_FULL regression.

I do agree about subtle bugs, but we haven't actually seen any other
ones than the vmstat breakage so far.

Also, I suspect that to flush out any bugs, we might want to

 (a) actually dequeue timers and work queues that are bound to a
particular CPU when a CPU goes down.

     Sure, we *could* make it a rule that everybody who binds a timer
to a particular CPU should just register the cpu-down thing, but why
make a rule that you have to make extra work? People who do per-cpu
work should have a setup function for when a new CPU comes _up_, but
why make people do pointless extra crap for the cpu-down case when the
generic code could just do ti for them.

 (b) maybe one of the test-bots could be encouraged to do a lot of cpu
offlining/onlining as a stress test>

That (a) part is important in that it avoids the subtle bug where some
timer or workqueue entry ends up being run on the wrong CPU after all,
just because the target CPU went down.

And the (b) part would hopefully flush out things that didn't start
things properly when a new cpu comes online.

Hmm? The above is obviously a longer-term thing and a bigger change,
but I think we should be able to just revert 874bbfe6 without anything
else going on, since I don't think we ever found anything else than
vmstat that had issues.

                    Linus

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

#1330547

Hello,

On Tue, Feb 09, 2016 at 09:04:18AM -0800, Linus Torvalds wrote:
> On Tue, Feb 9, 2016 at 8:50 AM, Tejun Heo <tj@kernel.org> wrote:
> > idk, not doing so is likely to cause subtle bugs which are difficult
> > to track down.  The problem with -stable is 874bbfe6 being backported
> > without the matching timer fix.
> 
> Well, according to this thread, even witht he timer fix the end result
> then shows odd problems, _and_ has a NO_HZ_FULL regression.

I don't know what that odd problem is indicating but it's likely we're
seeing another issue exposed by these changes or a bug during
backport, but yeah it's problematic.

> I do agree about subtle bugs, but we haven't actually seen any other
> ones than the vmstat breakage so far.

The thing with vmstat is that it's a work item which is most likely to
expose the issue as it runs constantly on all systems and we started
seeing it triggering soon after timer migration becomes more common.
I'd be surprised if we don't discover a lot more subtler ones down the
road.  Maybe it's that most of them won't trigger often enough to
matter much but it's a bit scary.

> Also, I suspect that to flush out any bugs, we might want to
> 
>  (a) actually dequeue timers and work queues that are bound to a
> particular CPU when a CPU goes down.
> 
>      Sure, we *could* make it a rule that everybody who binds a timer
> to a particular CPU should just register the cpu-down thing, but why
> make a rule that you have to make extra work? People who do per-cpu
> work should have a setup function for when a new CPU comes _up_, but
> why make people do pointless extra crap for the cpu-down case when the
> generic code could just do ti for them.

This goes the same for work items and timers.  If we want to do
explicit dequeueing or flushing of cpu-bound stuff on cpu down, we'll
have to either dedicate *_on() interfaces for correctness or introduce
a separate set of interfaces to use for optimization and correctness.
The current situation is that work itmes which are explicitly shut
down on cpu-down are correctness usages while the ones which are not
are optimization usages.  I'll try to scan through the usages and see
what the actual proportions are like.  Maybe we can get away with
declaring that _on() usages are absolute.

>  (b) maybe one of the test-bots could be encouraged to do a lot of cpu
> offlining/onlining as a stress test>
> 
> That (a) part is important in that it avoids the subtle bug where some
> timer or workqueue entry ends up being run on the wrong CPU after all,
> just because the target CPU went down.
> 
> And the (b) part would hopefully flush out things that didn't start
> things properly when a new cpu comes online.
> 
> Hmm? The above is obviously a longer-term thing and a bigger change,
> but I think we should be able to just revert 874bbfe6 without anything
> else going on, since I don't think we ever found anything else than
> vmstat that had issues.

So, how about reverting 874bbfe6 and performing random foreign
queueing during -rc's for a couple cycles so that we can at least find
out the broken ones quickly in devel branch and backport fixes as
they're found?

Thanks.

-- 
tejun

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

#1330553

On Tue, Feb 9, 2016 at 9:51 AM, Tejun Heo <tj@kernel.org> wrote:
>>
>>  (a) actually dequeue timers and work queues that are bound to a
>> particular CPU when a CPU goes down.
>>
> This goes the same for work items and timers.  If we want to do
> explicit dequeueing or flushing of cpu-bound stuff on cpu down, we'll
> have to either dedicate *_on() interfaces for correctness or introduce
> a separate set of interfaces to use for optimization and correctness.

We already do that. "add_timer_on()" for timers, and cpu !=
WORK_CPU_UNBOUND for work items.

>    Maybe we can get away with
> declaring that _on() usages are absolute.

I really think that anything else would be odd as hell. If you asked
for a timer (or work) on a particular CPU, and you get it on another
one, that's a bug.

It's much better to just dequeue those entries and say "sorry, your
CPU went away".

Of course, we could play around with just run them early at CPU-down
time (and anybody trying to requeue would get an error because the CPU
is in the process of going down), but that sounds like more work for
any users, and like a much more fundamental difference. The "just
silently dequeue" makes more sense, and pairs well with anything that
sets things up on CPU-up time (which a percpu entity will have to do
anyway).

> So, how about reverting 874bbfe6 and performing random foreign
> queueing during -rc's for a couple cycles so that we can at least find
> out the broken ones quickly in devel branch and backport fixes as
> they're found?

Yeah, that sounds good to me. Having some "cpu work/timer debug"
config option that ends up spreading out non-cpu-specific timers and
work in order to find bugs sounds like a good idea. And I don't think
it should be limited to rc releases, I think lots of people might be
willing to run that (the same way we had people - and even
distributions - that did PAGEALLOC_DEBUG which is a lot bigger
hammer).

             Linus

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web