Messages in this thread Patch in this message | | | Date | Wed, 4 Sep 2019 11:24:30 -0400 (EDT) | From | Mathieu Desnoyers <> | Subject | Re: [RFC PATCH 1/2] Fix: sched/membarrier: p->mm->membarrier_state racy load |
| |
----- On Sep 4, 2019, at 6:53 AM, Oleg Nesterov oleg@redhat.com wrote:
> On 09/03, Mathieu Desnoyers wrote: >> >> @@ -1130,6 +1130,10 @@ struct task_struct { >> unsigned long numa_pages_migrated; >> #endif /* CONFIG_NUMA_BALANCING */ >> >> +#ifdef CONFIG_MEMBARRIER >> + atomic_t membarrier_state; >> +#endif > > ... > >> +static inline void membarrier_prepare_task_switch(struct task_struct *t) >> +{ >> + if (!t->mm) >> + return; >> + atomic_set(&t->membarrier_state, >> + atomic_read(&t->mm->membarrier_state)); >> +} > > Why not > > rq->membarrier_state = next->mm ? t->mm->membarrier_state : 0; > > and > > if (cpu_rq(cpu)->membarrier_state & MEMBARRIER_STATE_GLOBAL_EXPEDITED) { > ... > } > > in membarrier_global_expedited() ? (I removed atomic_ to simplify) > > IOW, why this new member has to live in task_struct, not in rq?
As replied to Linus, if we copy the membarrier_state into the rq, we'd need to ensure we have full memory barriers between:
prior user-space memory accesses / setting the runqueue membarrier state
and
setting the runqueue membarrier state / following user-space memory accesses
Because membarrier does not take any runqueue lock when it iterates over runqueues.
I try to avoid putting too much memory barrier constraints on the scheduler for membarrier, but if it's really the way forward it could be done. And the basic question remains: it is acceptable performance-wise to load mm->membarrier_state from sched switch prepare ?
Thanks,
Mathieu
-- Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com
| |