Messages in this thread | | | Date | Wed, 4 Sep 2019 12:11:18 -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 7:11 AM, Oleg Nesterov oleg@redhat.com wrote:
> with or without these changes... > > Why do membarrier_register_*_expedited() check get_nr_threads() == 1? > This makes no sense to me, atomic_read(mm_users) == 1 should be enough.
Indeed, if every thread within a process hold a mm_users refcount, then the get_nr_threads() == 1 check becomes redundant.
AFAIR, this check started out as "get_nr_threads() == 1", and then I changed the code to also cover the multi-process CLONE_VM use-case by adding the additional check.
> And I am not sure I understand membarrier_mm_sync_core_before_usermode(). > OK, membarrier_private_expedited() can race with user -> kernel -> user > transition, but we do not care unless both user's above have the same mm? > Shouldn't membarrier_mm_sync_core_before_usermode() do > > if (current->mm != mm) > return; > > at the start to make it more clear and avoid sync_core_before_usermode() > if possible?
Indeed, if we have taskA -> kernel -> taskB, it implies that we go through switch_mm() when scheduling taskB, which provides the required core serializing guarantees.
Moreover, if we look closely at the call to membarrier_mm_sync_core_before_usermode(), the mm it receives as parameter is the rq->prev_mm. So using the prev_mm membarrier state to decide whether we need to issue a sync_core before returning to a different next mm is not really relevant unless the next mm == rq->prev_mm.
Nothing there seem to be actively buggy, but those are indeed nice cleanups.
Thanks,
Mathieu
-- Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com
| |