linux-kernel - Re: [RFC][PATCH 12/13] stop

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20150623180411.GF3644@twins.programming.kicks-ass.net>
Date:	Tue, 23 Jun 2015 20:04:11 +0200
From:	Peter Zijlstra <peterz@...radead.org>
To:	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
Cc:	Oleg Nesterov <oleg@...hat.com>, tj@...nel.org, mingo@...hat.com,
	linux-kernel@...r.kernel.org, der.herr@...r.at, dave@...olabs.net,
	riel@...hat.com, viro@...IV.linux.org.uk,
	torvalds@...ux-foundation.org
Subject: Re: [RFC][PATCH 12/13] stop_machine: Remove lglock

On Tue, Jun 23, 2015 at 10:30:38AM -0700, Paul E. McKenney wrote:
> Good, you don't need this because you can check for dynticks later.
> You will need to check for offline CPUs.

get_online_cpus()
for_each_online_cpus() {
 ...
}

is what the new code does.

> > -	/*
> > -	 * Each pass through the following loop attempts to force a
> > -	 * context switch on each CPU.
> > -	 */
> > -	while (try_stop_cpus(cma ? cm : cpu_online_mask,
> > -			     synchronize_sched_expedited_cpu_stop,
> > -			     NULL) == -EAGAIN) {
> > -		put_online_cpus();
> > -		atomic_long_inc(&rsp->expedited_tryfail);
> > -
> > -		/* Check to see if someone else did our work for us. */
> > -		s = atomic_long_read(&rsp->expedited_done);
> > -		if (ULONG_CMP_GE((ulong)s, (ulong)firstsnap)) {
> > -			/* ensure test happens before caller kfree */
> > -			smp_mb__before_atomic(); /* ^^^ */
> > -			atomic_long_inc(&rsp->expedited_workdone1);
> > -			free_cpumask_var(cm);
> > -			return;
> 
> Here you lose batching.  Yeah, I know that synchronize_sched_expedited()
> is -supposed- to be used sparingly, but it is not cool for the kernel
> to melt down just because some creative user found a way to heat up a
> code path.  Need a mutex_trylock() with a counter and checking for
> others having already done the needed work.

I really think you're making that expedited nonsense far too accessible.

But it was exactly that trylock I was trying to get rid of.

> And we still need to be able to drop back to synchronize_sched()
> (AKA wait_rcu_gp(call_rcu_sched) in this case) in case we have both a
> creative user and a long-running RCU-sched read-side critical section.

No, a long-running RCU-sched read-side is a bug and we should fix that,
its called a preemption-latency, we don't like those.

> > +	for_each_online_cpu(cpu) {
> > +		struct rcu_dynticks *rdtp = &per_cpu(rcu_dynticks, cpu);
> > 
> > -		/* Recheck to see if someone else did our work for us. */
> > -		s = atomic_long_read(&rsp->expedited_done);
> > -		if (ULONG_CMP_GE((ulong)s, (ulong)firstsnap)) {
> > -			/* ensure test happens before caller kfree */
> > -			smp_mb__before_atomic(); /* ^^^ */
> > -			atomic_long_inc(&rsp->expedited_workdone2);
> > -			free_cpumask_var(cm);
> > -			return;
> > -		}
> > +		/* Offline CPUs, idle CPUs, and any CPU we run on are quiescent. */
> > +		if (!(atomic_add_return(0, &rdtp->dynticks) & 0x1))
> > +			continue;
> 
> Let's see...  This does work for idle CPUs and for nohz_full CPUs running
> in userspace.
> 
> It does not work for the current CPU, so the check needs an additional
> check against raw_smp_processor_id(), which is easy enough to add.

Right, realized after I send it out, but it _should_ work for the
current cpu too. Just pointless doing it.

> There always has been a race window involving CPU hotplug.

There is no hotplug race, the entire thing has get_online_cpus() held
across it.

> > +		stop_one_cpu(cpu, synchronize_sched_expedited_cpu_stop, NULL);
> 
> My thought was to use smp_call_function_single(), and to have the function
> called recheck dyntick-idle state, avoiding doing a set_tsk_need_resched()
> if so.

set_tsk_need_resched() is buggy and should not be used.

> This would result in a single pass through schedule() instead
> of stop_one_cpu()'s double context switch.  It would likely also require
> some rework of rcu_note_context_switch(), which stop_one_cpu() avoids
> the need for.

_IF_ you're going to touch rcu_note_context_switch(), you might as well
use a completion, set it for the number of CPUs that need a resched,
spray resched-IPI and have rcu_note_context_switch() do a complete().

But I would really like to avoid adding code to
rcu_note_context_switch(), because we run that on _every_ single context
switch.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/