Messages in this thread | | | From | Linus Torvalds <> | Date | Sun, 8 Sep 2019 09:51:08 -0700 | Subject | Re: [RFC PATCH 4/4] Fix: sched/membarrier: p->mm->membarrier_state racy load (v2) |
| |
On Sun, Sep 8, 2019 at 6:49 AM Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote: > > +static void sync_runqueues_membarrier_state(struct mm_struct *mm) > +{ > + int membarrier_state = atomic_read(&mm->membarrier_state); > + bool fallback = false; > + cpumask_var_t tmpmask; > + > + if (!zalloc_cpumask_var(&tmpmask, GFP_NOWAIT)) { > + /* Fallback for OOM. */ > + fallback = true; > + } > + > + /* > + * For each cpu runqueue, if the task's mm match @mm, ensure that all > + * @mm's membarrier state set bits are also set in in the runqueue's > + * membarrier state. This ensures that a runqueue scheduling > + * between threads which are users of @mm has its membarrier state > + * updated. > + */ > + cpus_read_lock(); > + rcu_read_lock(); > + for_each_online_cpu(cpu) { > + struct rq *rq = cpu_rq(cpu); > + struct task_struct *p; > + > + p = task_rcu_dereference(&rq->curr); > + if (p && p->mm == mm) { > + if (!fallback) > + __cpumask_set_cpu(cpu, tmpmask); > + else > + smp_call_function_single(cpu, ipi_sync_rq_state, > + mm, 1); > + } > + }
I really absolutely detest this whole "fallback" code.
It will never get any real testing, and the code is just broken.
Why don't you just use the mm_cpumask(mm) unconditionally? Yes, it will possibly call too many CPU's, but this fallback code is just completely disgusting.
Do a simple and clean implementation. Then, if you can show real performance issues (which I doubt), maybe do something else, but even then you should never do something that will effectively create cases that have absolutely zero test-coverage.
Linus
| |