Re: [RFC PATCH v3 1/2] membarrier: Provide register expedited private command

[Mathieu Desnoyers]

----- On Sep 22, 2017, at 4:59 AM, Boqun Feng boqun.feng@xxxxxxxxx wrote:

> On Tue, Sep 19, 2017 at 06:13:41PM -0400, Mathieu Desnoyers wrote:
> [...]
>> +static inline void membarrier_arch_sched_in(struct task_struct *prev,
>> +		struct task_struct *next)
>> +{
>> +	/*
>> +	 * Only need the full barrier when switching between processes.
>> +	 */
>> +	if (likely(!test_ti_thread_flag(task_thread_info(next),
>> +			TIF_MEMBARRIER_PRIVATE_EXPEDITED)
>> +				|| prev->mm == next->mm))
> 
> And we also don't need the smp_mb() if !prev->mm, because switching from
> kernel to user will have a smp_mb() implied by mmdrop()?

Right. And we also don't need it when switching from userspace to kernel
thread neither. Something like this:

static inline void membarrier_arch_sched_in(struct task_struct *prev,
                struct task_struct *next)
{
        /*
         * Only need the full barrier when switching between processes.
         * Barrier when switching from kernel to userspace is not
         * required here, given that it is implied by mmdrop(). Barrier
         * when switching from userspace to kernel is not needed after
         * store to rq->curr.
         */
        if (likely(!test_ti_thread_flag(task_thread_info(next),
                        TIF_MEMBARRIER_PRIVATE_EXPEDITED)
                        || !prev->mm || !next->mm || prev->mm == next->mm))
                return;

        /*
         * The membarrier system call requires a full memory barrier
         * after storing to rq->curr, before going back to user-space.
         */
        smp_mb();
}

> 
>> +		return;
>> +
>> +	/*
>> +	 * The membarrier system call requires a full memory barrier
>> +	 * after storing to rq->curr, before going back to user-space.
>> +	 */
>> +	smp_mb();
>> +}
> 
> [...]
> 
>> +static inline void membarrier_fork(struct task_struct *t,
>> +		unsigned long clone_flags)
>> +{
>> +	if (!current->mm || !t->mm)
>> +		return;
>> +	t->mm->membarrier_private_expedited =
>> +		current->mm->membarrier_private_expedited;
> 
> Have we already done the copy of ->membarrier_private_expedited in
> copy_mm()?

copy_mm() is performed without holding current->sighand->siglock, so
it appears to be racing with concurrent membarrier register cmd.
However, given that it is a single flag updated with WRITE_ONCE()
and read with READ_ONCE(), it might be OK to rely on copy_mm there.
If userspace runs registration concurrently with fork, they should
not expect the child to be specifically registered or unregistered.

So yes, I think you are right about removing this copy and relying on
copy_mm() instead. I also think we can improve membarrier_arch_fork()
on powerpc to test the current thread flag rather than using current->mm.

Which leads to those two changes:

static inline void membarrier_fork(struct task_struct *t,
                unsigned long clone_flags)
{
        /*
         * Prior copy_mm() copies the membarrier_private_expedited field
         * from current->mm to t->mm.
         */
        membarrier_arch_fork(t, clone_flags);
}

And on PowerPC:

static inline void membarrier_arch_fork(struct task_struct *t,
                unsigned long clone_flags)
{
        /*
         * Coherence of TIF_MEMBARRIER_PRIVATE_EXPEDITED against thread
         * fork is protected by siglock. membarrier_arch_fork is called
         * with siglock held.
         */
        if (test_thread_flag(TIF_MEMBARRIER_PRIVATE_EXPEDITED))
                set_ti_thread_flag(task_thread_info(t),
                                TIF_MEMBARRIER_PRIVATE_EXPEDITED);
}

Thanks,

Mathieu


> 
> Regards,
> Boqun
> 
>> +	membarrier_arch_fork(t, clone_flags);
>> +}
>> +static inline void membarrier_execve(struct task_struct *t)
>> +{
>> +	t->mm->membarrier_private_expedited = 0;
>> +	membarrier_arch_execve(t);
>> +}
>> +#else
>> +static inline void membarrier_sched_in(struct task_struct *prev,
>> +		struct task_struct *next)
>> +{
>> +}
>> +static inline void membarrier_fork(struct task_struct *t,
>> +		unsigned long clone_flags)
>> +{
>> +}
>> +static inline void membarrier_execve(struct task_struct *t)
>> +{
>> +}
>> +#endif
>> +
> [...]

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com