Re: [RFC PATCH for 4.15 5/6] membarrier: x86: Provide core serializing command

----- On Nov 9, 2017, at 2:07 PM, Andy Lutomirski luto-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org wrote:

On Wed, Nov 8, 2017 at 10:35 AM, Mathieu Desnoyers
[off-list ref] wrote:

quoted

+/*
+ * x86-64 implements return to user-space through sysret, which is not a
+ * core-serializing instruction. Therefore, we need an explicit core
+ * serializing instruction after going from kernel thread back to
+ * user-space thread (active_mm moved back to current mm).
+ */
+static inline void membarrier_arch_mm_sync_core(struct mm_struct *mm)
+{
+       if (likely(!(atomic_read(&mm->membarrier_state) &
+                       MEMBARRIER_STATE_SYNC_CORE)))
+               return;
+       sync_core();
+}

IMO there should be an extremely clear specification somewhere for
what this function is supposed to do.

If I remember correctly, it's supposed to promise that the icache is
synced before the next time we return to usermode for the current mm
on this CPU.  If that's correct, then let's document it very
explicitly and let's also drop the "membarrier" from the name -- it's
a primitive we'll need anyway given the existing migration bug.

I understand that on x86 (specifically), synchronizing the icache and
doing a core serializing instruction may mean the same thing.

However, on architectures like ARM, icache sync differs from core
serialization. Those architectures typically have either a user-space
accessible instruction or a system call to perform the icache flush.
The missing part for JIT is core serialization (also called context
synchronization). icache flush is already handled by pre-existing
means.

So the promise here given by membarrier_arch_mm_sync_core() is that
a core serializing instruction is issued before the next time we return
to usermode on the current thread. However, we only need that guarantee
if the current thread's mm is a registered MEMBARRIER_{...}_SYNC_CORE user.

Regarding the existing migration bug, what I think you want is a kind
of weaker "sync_core()", which ensures that a core serializing
instruction is issued before the next time the current thread returns
to usermode.

It could be e.g.: set_tsk_need_core_sync() which would set a
TIF_NEED_CORE_SYNC thread flag on the current thread.

Clearly, when this kind of thread flag is introduced as an
optimization over sync_core(), I would like to use that. However,
I don't think it replaces the membarrier_arch_mm_sync_core() entirely,
given that it would not check for the mm membarrier "SYNC_CORE"
registration state. It appears to me to be merely an optimization over
directly invoking sync_core.

What I suggest is that I update the comment above
membarrier_arch_mm_sync_core to spell out more clearly that all we
need is to have a core serializing instruction issued before the next
time the current thread returns to user-space. I can still use
sync_core for now, and we can then improve the implementation
whenever a new thread flag is introduced. The new comment would look
like:

/*
 * x86-64 implements return to user-space through sysret, which is not a
 * core-serializing instruction. Therefore, we need an explicit core
 * serializing instruction after going from kernel thread back to
 * user-space thread (active_mm moved back to current mm).
 *
 * This function ensures that a core serializing instruction is issued
 * before the current thread returns to user-space.
 */

Thoughts ?

Thanks,

Mathieu


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