lkml.org 
[lkml]   [2020]   [Dec]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH 3/3] membarrier: Propagate SYNC_CORE and RSEQ actions more carefully
On Tue, Dec 1, 2020 at 6:28 AM Mathieu Desnoyers
<mathieu.desnoyers@efficios.com> wrote:
>
> ----- On Dec 1, 2020, at 5:16 AM, Peter Zijlstra peterz@infradead.org wrote:
>
> > On Mon, Nov 30, 2020 at 09:50:35AM -0800, Andy Lutomirski wrote:
> >> membarrier() carefully propagates SYNC_CORE and RSEQ actions to all
> >> other CPUs, but there are two issues.
> >>
> >> - membarrier() does not sync_core() or rseq_preempt() the calling
> >> CPU. Aside from the logic being mind-bending, this also means
> >> that it may not be safe to modify user code through an alias,
> >> call membarrier(), and then jump to a different executable alias
> >> of the same code.
> >
> > I always understood this to be on purpose. The calling CPU can fix up
> > itself just fine. The pain point is fixing up the other CPUs, and that's
> > where membarrier() helps.
>
> Indeed, as documented in the man page:
>
> MEMBARRIER_CMD_PRIVATE_EXPEDITED_SYNC_CORE (since Linux 4.16)
> In addition to providing the memory ordering guarantees de‐
> scribed in MEMBARRIER_CMD_PRIVATE_EXPEDITED, upon return from
> system call the calling thread has a guarantee that all its run‐
> ning thread siblings have executed a core serializing instruc‐
> tion. This guarantee is provided only for threads in the same
> process as the calling thread.
>
> membarrier sync core guarantees a core serializing instruction on the siblings,
> not on the caller thread. This has been done on purpose given that the caller
> thread can always issue its core serializing instruction from user-space on
> its own.
>
> >
> > That said, I don't mind including self, these aren't fast calls by any
> > means.
>
> I don't mind including self either, but this would require documentation
> updates, including man pages, to state that starting from kernel Y this
> is the guaranteed behavior. It's then tricky for user-space to query what
> the behavior is unless we introduce a new membarrier command for it. So this
> could introduce issues if software written for the newer kernels runs on older
> kernels.

For rseq at least, if we do this now we don't have this issue -- I
don't think any released kernel has the rseq mode.

>
> >
> >> - membarrier() does not explicitly sync_core() remote CPUs either;
> >> instead, it relies on the assumption that an IPI will result in a
> >> core sync. On x86, I think this may be true in practice, but
> >> it's not architecturally reliable. In particular, the SDM and
> >> APM do not appear to guarantee that interrupt delivery is
> >> serializing.
> >
> > Right, I don't think we rely on that, we do rely on interrupt delivery
> > providing order though -- as per the previous email.
> >
> >> On a preemptible kernel, IPI return can schedule,
> >> thereby switching to another task in the same mm that was
> >> sleeping in a syscall. The new task could then SYSRET back to
> >> usermode without ever executing IRET.
> >
> > This; I think we all overlooked this scenario.
>
> Indeed, this is an issue which needs to be fixed.
>
> >
> >> This patch simplifies the code to treat the calling CPU just like
> >> all other CPUs, and explicitly sync_core() on all target CPUs. This
> >> eliminates the need for the smp_mb() at the end of the function
> >> except in the special case of a targeted remote membarrier(). This
> >> patch updates that code and the comments accordingly.
>
> I am not confident that removing the smp_mb at the end of membarrier is
> an appropriate change, nor that it simplifies the model.

Ah, but I didn't remove it. I carefully made sure that every possible
path through the function does an smp_mb() or stronger after all the
cpu_rq reads. ipi_func(), on_each_cpu(), and the explicit smp_mb()
cover the three cases.

That being said, if you prefer, I can make the change to skip the
calling CPU, in which case I'll leave the smp_mb() at the end alone.

\
 
 \ /
  Last update: 2020-12-01 19:14    [W:0.090 / U:0.108 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site