----- On Sep 21, 2017, at 11:30 PM, Boqun Feng [email protected] wrote: > On Fri, Sep 22, 2017 at 11:22:06AM +0800, Boqun Feng wrote: >> Hi Mathieu, >> >> On Tue, Sep 19, 2017 at 06:13:41PM -0400, Mathieu Desnoyers wrote: >> > Provide a new command allowing processes to register their intent to use >> > the private expedited command. >> > >> > This allows PowerPC to skip the full memory barrier in switch_mm(), and >> > only issue the barrier when scheduling into a task belonging to a >> > process that has registered to use expedited private. >> > >> > Processes are now required to register before using >> > MEMBARRIER_CMD_PRIVATE_EXPEDITED, otherwise that command returns EPERM. >> > >> >> Sorry I'm late for the party, but I couldn't stop thinking whether we >> could avoid the register thing at all, because the registering makes >> sys_membarrier() more complex(both for the interface and the >> implementation). So how about we trade-off a little bit by taking >> some(not all) the rq->locks? >> >> The idea is in membarrier_private_expedited(), we go through all ->curr >> on each CPU and >> >> 1) If it's a userspace task and its ->mm is matched, we send an ipi >> >> 2) If it's a kernel task, we skip >> >> (Because there will be a smp_mb() implied by mmdrop(), when it >> switchs to userspace task). >> >> 3) If it's a userspace task and its ->mm is not matched, we take >> the corresponding rq->lock and check rq->curr again, if its ->mm >> matched, we send an ipi, otherwise we do nothing. >> >> (Because if we observe rq->curr is not matched with rq->lock >> held, when a task having matched ->mm schedules in, the rq->lock >> pairing along with the smp_mb__after_spinlock() will guarantee >> it observes all memory ops before sys_membarrir()). >> >> membarrier_private_expedited() will look like this if we choose this >> way: >> >> void membarrier_private_expedited() >> { >> int cpu; >> bool fallback = false; >> cpumask_var_t tmpmask; >> struct rq_flags rf; >> >> >> if (num_online_cpus() == 1) >> return; >> >> smp_mb(); >> >> if (!zalloc_cpumask_var(&tmpmask, GFP_NOWAIT)) { >> /* Fallback for OOM. */ >> fallback = true; >> } >> >> cpus_read_lock(); >> for_each_online_cpu(cpu) { >> struct task_struct *p; >> >> if (cpu == raw_smp_processor_id()) >> continue; >> >> rcu_read_lock(); >> p = task_rcu_dereference(&cpu_rq(cpu)->curr); >> >> if (!p) { >> rcu_read_unlock(); >> continue; >> } >> >> if (p->mm == current->mm) { >> if (!fallback) >> __cpumask_set_cpu(cpu, tmpmask); >> else >> smp_call_function_single(cpu, ipi_mb, NULL, 1); >> } >> >> if (p->mm == current->mm || !p->mm) { >> rcu_read_unlock(); >> continue; >> } >> >> rcu_read_unlock(); >> >> /* >> * This should be a arch-specific code, as we don't >> * need it at else place other than some archs without >> * a smp_mb() in switch_mm() (i.e. powerpc) >> */ >> rq_lock_irq(cpu_rq(cpu), &rf); >> if (p->mm == current->mm) { > > Oops, this one should be > > if (cpu_curr(cpu)->mm == current->mm) > >> if (!fallback) >> __cpumask_set_cpu(cpu, tmpmask); >> else >> smp_call_function_single(cpu, ipi_mb, NULL, 1); > > , and this better be moved out of the lock rq->lock critical section. > > Regards, > Boqun > >> } >> rq_unlock_irq(cpu_rq(cpu), &rf); >> } >> if (!fallback) { >> smp_call_function_many(tmpmask, ipi_mb, NULL, 1); >> free_cpumask_var(tmpmask); >> } >> cpus_read_unlock(); >> >> smp_mb(); >> } >> >> Thoughts?
Hi Boqun, The main concern Peter has with the runqueue locking approach is interference with the scheduler by hitting all CPU's runqueue locks repeatedly if someone performs membarrier system calls in a short loop. Just reading the rq->curr pointer does not generate as much overhead as grabbing each rq lock. Thanks, Mathieu >> >> Regards, >> Boqun >> > [...] -- Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com
