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

Groups > linux.kernel > #1172560 > unrolled thread

[RFC PATCH 0/6] stop_machine: kill stop_cpus_mutex and stop_cpus_lock

Back to article view | Back to linux.kernel

Contents

  [RFC PATCH 0/6] stop_machine: kill stop_cpus_mutex and  stop_cpus_lock Oleg Nesterov <oleg@redhat.com> - 2015-06-26 04:20 +0200
    Re: [RFC PATCH 0/6] stop_machine: kill stop_cpus_mutex and  stop_cpus_lock Oleg Nesterov <oleg@redhat.com> - 2015-06-26 04:40 +0200
    Re: [RFC PATCH 0/6] stop_machine: kill stop_cpus_mutex and  stop_cpus_lock Oleg Nesterov <oleg@redhat.com> - 2015-06-29 06:10 +0200
      Re: [RFC PATCH 0/6] stop_machine: kill stop_cpus_mutex and  stop_cpus_lock Oleg Nesterov <oleg@redhat.com> - 2015-06-30 03:10 +0200
    Re: [RFC PATCH 0/6] stop_machine: kill stop_cpus_mutex and  stop_cpus_lock Oleg Nesterov <oleg@redhat.com> - 2015-06-30 03:10 +0200

#1172560 — [RFC PATCH 0/6] stop_machine: kill stop_cpus_mutex and stop_cpus_lock

On 06/25, Peter Zijlstra wrote:
>
> On Tue, Jun 23, 2015 at 07:24:16PM +0200, Oleg Nesterov wrote:
> >
> > 	lock_stop_cpus_works(cpumask)
> > 	{
> > 		for_each_cpu(cpu, cpumask)
> > 			mutex_lock(per_cpu(cpu_stopper_task, cpu).work_mutex);
> > 	}
> >
> > 	unlock_stop_cpus_works(cpumask)
> > 	{
> > 		for_each_cpu(cpu, cpumask)
> > 			mutex_lock(...);
> > 	}
> >
> > which should be used instead of stop_cpus_mutex. After this change
> > stop_two_cpus() can just use stop_cpus().
>
> Right, lockdep annotating that will be 'interesting' though.

Sure, and this is too inefficient, this is only to explain what
I mean.

How about this series? Untested. For review.

> And
> stop_two_cpus() then has the problem of allocating a cpumask.

Yes, but we can avoid this, see the changelog in 5/6.

> Simpler to
> let it keep 'abuse' the queueing spinlock in there.

Not sure.

And note that this series kills stop_cpus_mutex, so that multiple
stop_cpus()'s / stop_machine()'s can run in parallel if cpumask's
do not overlap.

Note also the changelog in 6/6, we can simplify + optimize this code
a bit more.

What do you think?

Oleg.

 include/linux/lglock.h  |    5 -
 kernel/locking/lglock.c |   22 -----
 kernel/stop_machine.c   |  197 ++++++++++++++++++++++++++++-------------------
 3 files changed, 119 insertions(+), 105 deletions(-)

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1172566

As always, forgot to mention...

Also. We can turn these stop_work_alloc/stop_work_free into the generic
helpers which (I think) can have more users.

On 06/26, Oleg Nesterov wrote:
>
> On 06/25, Peter Zijlstra wrote:
> >
> > On Tue, Jun 23, 2015 at 07:24:16PM +0200, Oleg Nesterov wrote:
> > >
> > > 	lock_stop_cpus_works(cpumask)
> > > 	{
> > > 		for_each_cpu(cpu, cpumask)
> > > 			mutex_lock(per_cpu(cpu_stopper_task, cpu).work_mutex);
> > > 	}
> > >
> > > 	unlock_stop_cpus_works(cpumask)
> > > 	{
> > > 		for_each_cpu(cpu, cpumask)
> > > 			mutex_lock(...);
> > > 	}
> > >
> > > which should be used instead of stop_cpus_mutex. After this change
> > > stop_two_cpus() can just use stop_cpus().
> >
> > Right, lockdep annotating that will be 'interesting' though.
>
> Sure, and this is too inefficient, this is only to explain what
> I mean.
>
> How about this series? Untested. For review.
>
> > And
> > stop_two_cpus() then has the problem of allocating a cpumask.
>
> Yes, but we can avoid this, see the changelog in 5/6.
>
> > Simpler to
> > let it keep 'abuse' the queueing spinlock in there.
>
> Not sure.
>
> And note that this series kills stop_cpus_mutex, so that multiple
> stop_cpus()'s / stop_machine()'s can run in parallel if cpumask's
> do not overlap.
>
> Note also the changelog in 6/6, we can simplify + optimize this code
> a bit more.
>
> What do you think?
>
> Oleg.
>
>  include/linux/lglock.h  |    5 -
>  kernel/locking/lglock.c |   22 -----
>  kernel/stop_machine.c   |  197 ++++++++++++++++++++++++++++-------------------
>  3 files changed, 119 insertions(+), 105 deletions(-)

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1173611

On 06/26, Oleg Nesterov wrote:
>
> 2 cmpxchg()'s vs 2 spin_lock()'s. Plus wake_up(), but we can check
> waitqueue_active().
>
> Do you think thi will be noticeably slower?
>
> Of course, if it races with another stop_two_cpus/stop_cpus it will
> sleep, but in this case we need to wait anyway.
>
>
> And I don't think that percpu-rwsem instead of stop_cpu_mutex makes
> sense. at least I don't understand how can it help. OK, stop_two_cpus()
> can use percpu_down_read() to avoid the deadlock with stop_cpus(), but
> you still need double-lock... So I don't think this will make it faster,
> this will just penalize stop_cpus(). Or I misunderstood.
>
> So I am still not convinced... But probably I am too biased ;)

Yes... I'll probably try to make v2, this version is overcomplicated
and buggy.


> Btw. I can't understand the cpu_active() checks in stop_two_cpus().
> Do we really need them?

Ah, please ignore.

Yes, we can't rely on stopper->enabled check in cpu_stop_queue_work(),
cpu_stop_signal_done() does not update multi_stop_data->num_threads /
->thread_ack. So we need to ensure that cpu_online() == T for both CPUS
or multi_cpu_stop() can hang.

But we can't use cpu_online() instead, take_cpu_down() can be already
queued.

So this relies on the fact that CPU_DOWN_PREPARE (which removes CPU
from cpu_active_mask) is called before stop_machine(take_cpu_down) and
we do not care that cpu_active() is not stable; if we see cpu_active()
cpu_online() can't change unders us because take_cpu_down() was not
queued.

If we change stop_two_cpus() to use stop_work_alloc_one() it can use
cpu_online(),

	int stop_two_cpus(unsigned int cpu1, unsigned int cpu2, cpu_stop_fn_t fn, void *arg)
	{
		struct cpu_stop_work *work1, *work2;
		struct cpu_stop_done done;
		struct multi_stop_data msdata = {
			.fn = fn,
			.data = arg,
			.num_threads = 2,
			.active_cpus = cpumask_of(cpu1),
		};

		set_state(&msdata, MULTI_STOP_PREPARE);
		cpu_stop_init_done(&done, 2);

		if (cpu1 > cpu2)
			swap(cpu1, cpu2);

		work1 = stop_work_alloc_one(cpu1, true);
		work2 = stop_work_alloc_one(cpu2, true);
		/* stop_machine() is blocked, cpu can't go away */
		if (cpu_online(cpu1) && cpu_online(cpu2)) {
			work1->fn   = work2->fn   = multi_cpu_stop;
			work1->arg  = work2->arg  = &msdata;
			work1->done = work2->done = &done;

			preempt_disable();
			cpu_stop_queue_work(cpu1, work1);
			cpu_stop_queue_work(cpu2, work2);
			preempt_enable();
			wait_for_completion(&done.completion);
		}

		stop_work_free_one(cpu1);
		stop_work_free_one(cpu2);
		stop_work_wake_up();

		return done.executed ? done.ret : -ENOENT;
	}

Oleg.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1174166

On 06/29, Peter Zijlstra wrote:
>
> On Mon, Jun 29, 2015 at 06:02:51AM +0200, Oleg Nesterov wrote:
>
> > If we change stop_two_cpus() to use stop_work_alloc_one() it can use
> > cpu_online(),
>
> So the one user of this actually needs cpu_active(); we do not want to
> go move tasks to an inactive cpu.
>
> So if you change this to cpu_online() we need to audit the user is doing
> the stricter test.

Hmm. But the user (migrate_swap_stop) should check cpu_active() anyway?

The cpu_active() checks in stop_two_cpus() can only help to ensure that
multi_cpu_stop() won't hang. CPU_DOWN_PREPARE can deactivate either CPU
right after the check?

Or stop_two_cpus() needs get_online_cpus(). Or I missed something.

Oleg.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1174168

On 06/29, Peter Zijlstra wrote:
>
> On Fri, Jun 26, 2015 at 10:46:12PM +0200, Oleg Nesterov wrote:
>
> > So I am still not convinced... But probably I am too biased ;)
>
> I'm just a tad worried, I don't want to make the relatively cheap
> operation of stop_two_cpus() more expensive to the benefit of
> stop_cpus().

OK. And I have another (simple) idea...

Never say tomorrow, but I will try to re-check and make the patch
tomorrow ;)

But let me send some cleanups first. Plus I believe I found another
stop_machine bug, see the last patch. So I hope these changes make
sense in any case.

Oleg.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web