lkml.org 
[lkml]   [2017]   [Jul]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH tip/core/rcu 4/5] sys_membarrier: Add expedited option
On Wed, Jul 26, 2017 at 08:41:10AM -0700, Paul E. McKenney wrote:
> On Wed, Jul 26, 2017 at 09:41:28AM +0200, Peter Zijlstra wrote:
> > On Tue, Jul 25, 2017 at 04:59:36PM -0700, Paul E. McKenney wrote:

> > Sure, but SCHED_OTHER auto throttles in that if there's anything else to
> > run, you get to wait. So you can't generate an IPI storm with it. Also,
> > again, we can be limited to a subset of CPUs.
>
> OK, what is its auto-throttle policy? One round of IPIs per jiffy or
> some such?

No. Its called wakeup latency :-) Your SCHED_OTHER task will not get to
insta-run all the time. If there are other tasks already running, we'll
not IPI unless it should preempt.

If its idle, nobody cares..

> Does this auto-throttling also apply if the user is running a CPU-bound
> SCHED_BATCH or SCHED_IDLE task on each CPU, and periodically waking up
> one of a large group of SCHED_OTHER tasks, where the SCHED_OTHER tasks
> immediately sleep upon being awakened?

SCHED_BATCH is even more likely to suffer wakeup latency since it will
never preempt anything.

> OK, and the rq->curr assignment is in common code, correct? Does this
> allow the IPI-only-requesting-process approach to live entirely within
> common code?

That is the idea.

> The 2010 email thread ended up with sys_membarrier() acquiring the
> runqueue lock for each CPU,

Yes, that's something I'm not happy with. Machine wide banging of that
lock will be a performance no-no.

> because doing otherwise meant adding code to the scheduler fastpath.

And that's obviously another thing I'm not happy with either.

> Don't we still need to do this?
>
> https://marc.info/?l=linux-kernel&m=126341138408407&w=2
> https://marc.info/?l=linux-kernel&m=126349766324224&w=2

I don't know.. those seem focussed on mm_cpumask() and we can't use that
per Will's email.


So I think we need to think anew on this, start from the ground up.

What is missing for this:

static void ipi_mb(void *info)
{
smp_mb(); // IPIs should be serializing but paranoid
}


sys_membarrier()
{
smp_mb(); // because sysenter isn't an unconditional mb

for_each_online_cpu(cpu) {
struct task_struct *p;

rcu_read_lock();
p = task_rcu_dereference(&cpu_curr(cpu));
if (p && p->mm == current->mm)
__set_bit(cpus, cpu);
rcu_read_unlock();
}

on_cpu_cpu_mask(cpus, ipi_mb, NULL, 1); // does local smp_mb() too
}

VS

__schedule()
{
spin_lock(&rq->lock);
smp_mb__after_spinlock(); // really full mb implied

/* lots */

if (likely(prev != next)) {

rq->curr = next;

context_switch() {
switch_mm();
switch_to();
// neither need imply a barrier

spin_unlock(&rq->lock);
}
}
}




So I think we need either switch_mm() or switch_to() to imply a full
barrier for this to work, otherwise we get:

CPU0 CPU1


lock rq->lock
mb

rq->curr = A

unlock rq->lock

lock rq->lock
mb

sys_membarrier()

mb

for_each_online_cpu()
p = A
// no match no IPI

mb
rq->curr = B

unlock rq->lock


And that's bad, because now CPU0 doesn't have an MB happening _after_
sys_membarrier() if B matches.


So without audit, I only know of PPC and Alpha not having a barrier in
either switch_*().

x86 obviously has barriers all over the place, arm has a super duper
heavy barrier in switch_to().

One option might be to resurrect spin_unlock_wait(), although to use
that here is really ugly too, but it would avoid thrashing the
rq->lock.

I think it'd end up having to look like:

rq = cpu_rq(cpu);
again:
rcu_read_lock()
p = task_rcu_dereference(&rq->curr);
if (p) {
raw_spin_unlock_wait(&rq->lock);
q = task_rcu_dereference(&rq->curr);
if (q != p) {
rcu_read_unlock();
goto again;
}
}
...

which is just about as horrible as it looks.

\
 
 \ /
  Last update: 2017-07-27 10:31    [W:0.088 / U:2.728 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site