Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1738189
| From | Boqun Feng <boqun.feng@gmail.com> |
|---|---|
| Newsgroups | linux.kernel |
| Subject | Re: [RFC PATCH v3 1/2] membarrier: Provide register expedited private command |
| Date | 2017-09-24 15:40 +0200 |
| Message-ID | <utfiz-Ql-49@gated-at.bofh.it> (permalink) |
| References | <urz21-2JM-1@gated-at.bofh.it> <uss8b-4hz-37@gated-at.bofh.it> <usxKx-7Bl-13@gated-at.bofh.it> |
| Organization | linux.* mail to news gateway |
[Multipart message — attachments visible in raw view] - view raw
On Fri, Sep 22, 2017 at 03:10:10PM +0000, Mathieu Desnoyers wrote:
> ----- On Sep 22, 2017, at 4:59 AM, Boqun Feng boqun.feng@gmail.com 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
Yep, but this case is covered already, as I think we don't allow kernel
thread to have TIF_MEMBARRIER_PRIVATE_EXPEDITED set, right?
> 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))
, so no need to test next->mm here.
> 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.
Speak of racing, I think we currently have a problem if we do a
register_private_expedited in one thread and do a
membarrer_private_expedited in another thread(sharing the same mm), as
follow:
{t1,t2,t3 sharing the same ->mm}
CPU 0 CPU 1 CPU2
==================== =================== ============
{in thread t1}
membarrier_register_private_expedited():
...
WRITE_ONCE(->mm->membarrier_private_expedited, 1);
membarrier_arch_register_private_expedited():
...
<haven't set the TIF for t3 yet>
{in thread t2}
membarrier_private_expedited():
READ_ONCE(->mm->membarrier_private_expedited); // == 1
...
for_each_online_cpu()
...
<p is cpu_rq(CPU2)->curr>
if (p && p->mm == current->mm) // false
<so no ipi sent to CPU2>
{about to switch to t3}
rq->curr = t3;
....
context_switch():
...
finish_task_swtich():
membarrier_sched_in():
<TIF is not set>
// no smp_mb() here.
, and we will miss the smp_mb() on CPU2, right? And this could even
happen if t2 has a membarrier_register_private_expedited() preceding the
membarrier_private_expedited().
Am I missing something subtle here?
Regards,
Boqun
> 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
Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread
[RFC PATCH v3 1/2] membarrier: Provide register expedited private command Mathieu Desnoyers <mathieu.desnoyers@efficios.com> - 2017-09-20 00:20 +0200
[RFC PATCH 2/2] membarrier: selftest: Test private expedited cmd Mathieu Desnoyers <mathieu.desnoyers@efficios.com> - 2017-09-20 00:20 +0200
Re: [RFC PATCH v3 1/2] membarrier: Provide register expedited private command Boqun Feng <boqun.feng@gmail.com> - 2017-09-22 05:30 +0200
Re: [RFC PATCH v3 1/2] membarrier: Provide register expedited private command Boqun Feng <boqun.feng@gmail.com> - 2017-09-22 05:40 +0200
Re: [RFC PATCH v3 1/2] membarrier: Provide register expedited private command Mathieu Desnoyers <mathieu.desnoyers@efficios.com> - 2017-09-22 07:30 +0200
Re: [RFC PATCH v3 1/2] membarrier: Provide register expedited private command Peter Zijlstra <peterz@infradead.org> - 2017-09-22 10:30 +0200
Re: [RFC PATCH v3 1/2] membarrier: Provide register expedited private command Boqun Feng <boqun.feng@gmail.com> - 2017-09-22 11:00 +0200
Re: [RFC PATCH v3 1/2] membarrier: Provide register expedited private command Boqun Feng <boqun.feng@gmail.com> - 2017-09-22 11:10 +0200
Re: [RFC PATCH v3 1/2] membarrier: Provide register expedited private command Mathieu Desnoyers <mathieu.desnoyers@efficios.com> - 2017-09-22 17:10 +0200
Re: [RFC PATCH v3 1/2] membarrier: Provide register expedited private command Boqun Feng <boqun.feng@gmail.com> - 2017-09-24 15:40 +0200
Re: [RFC PATCH v3 1/2] membarrier: Provide register expedited private command Mathieu Desnoyers <mathieu.desnoyers@efficios.com> - 2017-09-24 16:30 +0200
Re: [RFC PATCH v3 1/2] membarrier: Provide register expedited private command Boqun Feng <boqun.feng@gmail.com> - 2017-09-25 14:20 +0200
Re: [RFC PATCH v3 1/2] membarrier: Provide register expedited private command Peter Zijlstra <peterz@infradead.org> - 2017-09-25 14:30 +0200
Re: [RFC PATCH v3 1/2] membarrier: Provide register expedited private command Mathieu Desnoyers <mathieu.desnoyers@efficios.com> - 2017-09-25 14:50 +0200
csiph-web