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-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
                    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 3 — ← Prev page 1 [2] 3  Next page →

#1330362

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] | [prev] | [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] | [next] | [standalone]

#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]

Page 2 of 3 — ← Prev page 1 [2] 3  Next page →

Back to top | Article view | linux.kernel

csiph-web