Re: [RFC PATCH] introduce sys_membarrier(): process-wide memory barrier (v5)

KOSAKI Motohiro
2010-01-13T03:23:51Z

Hi

Interesting patch :)

I have few comments.

> Index: linux-2.6-lttng/kernel/sched.c
> ===================================================================
> --- linux-2.6-lttng.orig/kernel/sched.c 2010-01-12 10:25:47.000000000 -0500
> +++ linux-2.6-lttng/kernel/sched.c 2010-01-12 14:33:20.000000000 -0500
> @@ -10822,6 +10822,117 @@ struct cgroup_subsys cpuacct_subsys = {
> };
> #endif /* CONFIG_CGROUP_CPUACCT */
>
> +#ifdef CONFIG_SMP
> +
> +/*
> + * Execute a memory barrier on all active threads from the current process
> + * on SMP systems. Do not rely on implicit barriers in IPI handler execution,
> + * because batched IPI lists are synchronized with spinlocks rather than full
> + * memory barriers. This is not the bulk of the overhead anyway, so let's stay
> + * on the safe side.
> + */
> +static void membarrier_ipi(void *unused)
> +{
> + smp_mb();
> +}
> +
> +/*
> + * Handle out-of-mem by sending per-cpu IPIs instead.
> + */
> +static void membarrier_retry(void)
> +{
> + struct mm_struct *mm;
> + int cpu;
> +
> + for_each_cpu(cpu, mm_cpumask(current->mm)) {
> + spin_lock_irq(&cpu_rq(cpu)->lock);
> + mm = cpu_curr(cpu)->mm;
> + spin_unlock_irq(&cpu_rq(cpu)->lock);
> + if (current->mm == mm)
> + smp_call_function_single(cpu, membarrier_ipi, NULL, 1);
> + }
> +}
> +
> +#endif /* #ifdef CONFIG_SMP */
> +
> +/*
> + * sys_membarrier - issue memory barrier on current process running threads
> + * @expedited: (0) Lowest overhead. Few milliseconds latency.
> + * (1) Few microseconds latency.

Why do we need both expedited and non-expedited mode? at least, this documentation
is bad. it suggest "you have to use non-expedited mode always!".

> + *
> + * Execute a memory barrier on all running threads of the current process.
> + * Upon completion, the caller thread is ensured that all process threads
> + * have passed through a state where memory accesses match program order.
> + * (non-running threads are de facto in such a state)
> + *
> + * mm_cpumask is used as an approximation. It is a superset of the cpumask to
> + * which we must send IPIs, mainly due to lazy TLB shootdown. Therefore,
> + * we check each runqueue to make sure our ->mm is indeed running on them. This
> + * reduces the risk of disturbing a RT task by sending unnecessary IPIs. There
> + * is still a slight chance to disturb an unrelated task, because we do not lock
> + * the runqueues while sending IPIs, but the real-time effect of this heavy
> + * locking would be worse than the comparatively small disruption of an IPI.
> + *
> + * RED PEN: before assinging a system call number for sys_membarrier() to an
> + * architecture, we must ensure that switch_mm issues full memory barriers (or a
> + * synchronizing instruction having the same effect) between:
> + * - user-space code execution and clear mm_cpumask.
> + * - set mm_cpumask and user-space code execution.
> + * In some case adding a comment to this effect will suffice, in others we will
> + * need to add smp_mb__before_clear_bit()/smp_mb__after_clear_bit() or simply
> + * smp_mb(). These barriers are required to ensure we do not _miss_ a CPU that
> + * need to receive an IPI, which would be a bug.
> + *
> + * On uniprocessor systems, this system call simply returns 0 without doing
> + * anything, so user-space knows it is implemented.
> + */
> +SYSCALL_DEFINE1(membarrier, int, expedited)
> +{
> +#ifdef CONFIG_SMP
> + cpumask_var_t tmpmask;
> + struct mm_struct *mm;
> + int cpu;
> +
> + if (unlikely(thread_group_empty(current) || (num_online_cpus() == 1)))
> + return 0;
> + if (!unlikely(expedited)) {

unlikely(!expedited)?

> + synchronize_sched();
> + return 0;
> + }
> + /*
> + * Memory barrier on the caller thread _before_ sending first
> + * IPI. Matches memory barriers around mm_cpumask modification in
> + * switch_mm().
> + */
> + smp_mb();
> + if (!alloc_cpumask_var(&tmpmask, GFP_KERNEL)) {
> + membarrier_retry();
> + goto unlock;
> + }

if CONFIG_CPUMASK_OFFSTACK=1, alloc_cpumask_var call kmalloc. FWIW,
kmalloc calling seems destory the worth of this patch.

#ifdef CONFIG_CPUMASK_OFFSTACK
membarrier_retry();
goto unlock;
#endif

is better? I'm not sure.

> + cpumask_copy(tmpmask, mm_cpumask(current->mm));
> + preempt_disable();
> + cpumask_clear_cpu(smp_processor_id(), tmpmask);
> + for_each_cpu(cpu, tmpmask) {
> + spin_lock_irq(&cpu_rq(cpu)->lock);
> + mm = cpu_curr(cpu)->mm;
> + spin_unlock_irq(&cpu_rq(cpu)->lock);
> + if (current->mm != mm)
> + cpumask_clear_cpu(cpu, tmpmask);
> + }
> + smp_call_function_many(tmpmask, membarrier_ipi, NULL, 1);
> + preempt_enable();
> + free_cpumask_var(tmpmask);
> +unlock:
> + /*
> + * Memory barrier on the caller thread _after_ we finished
> + * waiting for the last IPI. Matches memory barriers around mm_cpumask
> + * modification in switch_mm().
> + */
> + smp_mb();
> +#endif /* #ifdef CONFIG_SMP */
> + return 0;
> +}