membarrier fixes

arch/x86/mm/tlb.c +0 -0

index 11666ba19b62..dabe683ab076 100644

----- On Nov 30, 2020, at 12:50 PM, Andy Lutomirski [email protected] wrote:
[...]

Quoted 5 lines

arch/x86/mm/tlb.c +0 -0

index 11666ba19b62..dabe683ab076 100644

arch/x86/mm/tlb.c void switch_mm_irqs_off(struct mm_struct *prev, struct · line 480

Quoted 9 lines

arch/x86/mm/tlb.c +3 -2

@@ -474,8 +474,10 @@ void switch_mm_irqs_off(struct mm_struct *prev, struct
 mm_struct *next,
      /*
       * The membarrier system call requires a full memory barrier and
       * core serialization before returning to user-space, after
-      * storing to rq->curr. Writing to CR3 provides that full
-      * memory barrier and core serializing instruction.
+      * storing to rq->curr, when changing mm.  This is because membarrier()
+      * sends IPIs to all CPUs that are in the target mm, but another
+      * CPU switch to the target mm in the mean time.

The sentence "This is because membarrier() sends IPIs to all CPUs that are in the target mm, but another CPU switch to the target mm in the mean time." seems rather unclear. Could be clarified with e.g.:

"This is because membarrier() sends IPIs to all CPUs that are in the target mm to make them issue memory barriers. However, if another CPU switches to/from the target mm concurrently with membarrier(), it can cause that CPU not to receive the IPI when it really should issue a memory barrier."

For the rest of this patch:

Reviewed-by Mathieu Desnoyers <[email protected]>

Thanks!

Mathieu

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

It seems to be that most RSEQ membarrier users will expect any stores done before the membarrier() syscall to be visible to the target task(s). While this is extremely likely to be true in practice, nothing actually guarantees it by a strict reading of the x86 manuals. Rather than providing this guarantee by accident and potentially causing a problem down the road, just add an explicit barrier.

Signed-off-by Andy Lutomirski <[email protected]>

---
 kernel/sched/membarrier.c | 8 ++++++++
 1 file changed, 8 insertions(+)

kernel/sched/membarrier.c +0 -0

index e23e74d52db5..7d98ef5d3bcd 100644
@@ -40,6 +40,14 @@ static void ipi_mb(void *info)
 
 static void ipi_rseq(void *info)
 {

Quoted 7 lines · Andy Lutomirski

It seems to be that most RSEQ membarrier users will expect any stores done before the membarrier() syscall to be visible to the target task(s). While this is extremely likely to be true in practice, nothing actually guarantees it by a strict reading of the x86 manuals. Rather than providing this guarantee by accident and potentially causing a problem down the road, just add an explicit barrier.

A very long time ago; when Jens introduced smp_call_function(), we had this discussion. At the time Linus said that receiving an interrupt had better be ordering, and if it is not, then it's up to the architecture to handle that before it gets into the common code.

https://lkml.kernel.org/r/[email protected]

Maybe we want to revisit this now, but there might be a fair amount of code relying on all this by now.

Documenting it better might help.

Quoted 14 lines

Signed-off-by Andy Lutomirski <[email protected]>

---
 kernel/sched/membarrier.c | 8 ++++++++
 1 file changed, 8 insertions(+)

kernel/sched/membarrier.c +0 -0

index e23e74d52db5..7d98ef5d3bcd 100644
@@ -40,6 +40,14 @@ static void ipi_mb(void *info)
 
 static void ipi_rseq(void *info)
 {

kernel/sched/membarrier.c static void ipi_mb(void *info) · line 52

Quoted 10 lines

kernel/sched/membarrier.c +8 -0

+	/*
+	 * Ensure that all stores done by the calling thread are visible
+	 * to the current task before the current task resumes.  We could
+	 * probably optimize this away on most architectures, but by the
+	 * time we've already sent an IPI, the cost of the extra smp_mb()
+	 * is negligible.
+	 */
+	smp_mb();
 	rseq_preempt(current);
 }

So I think this really isn't right.

Quoted 14 lines · ----- On Dec 1, 2020, at 5:06 AM, Peter Zijlstra [email protected] wrote:

Quoted 7 lines · Andy Lutomirski

It seems to be that most RSEQ membarrier users will expect any stores done before the membarrier() syscall to be visible to the target task(s). While this is extremely likely to be true in practice, nothing actually guarantees it by a strict reading of the x86 manuals. Rather than providing this guarantee by accident and potentially causing a problem down the road, just add an explicit barrier.

A very long time ago; when Jens introduced smp_call_function(), we had this discussion. At the time Linus said that receiving an interrupt had better be ordering, and if it is not, then it's up to the architecture to handle that before it gets into the common code.

Quoted 6 lines

https://lkml.kernel.org/r/[email protected]

Maybe we want to revisit this now, but there might be a fair amount of code relying on all this by now.

Documenting it better might help.

Considering that we already have this in membarrier ipi_mb :

static void ipi_mb(void *info)

{
        smp_mb();       /* IPIs should be serializing but paranoid. */
}

I think it makes sense to add this same smp_mb() in the ipi_rseq if the expected behavior is to order memory accesses as well, and have the same level of paranoia as the ipi_mb.

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.

- 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.

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 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.

Signed-off-by Andy Lutomirski <[email protected]>

kernel/sched/membarrier.c +5 -6

@@ -228,25 +258,33 @@ static int membarrier_private_expedited(int flags, int cpu_id)
 		rcu_read_unlock();
 	}
 
-	preempt_disable();
-	if (cpu_id >= 0)
-		smp_call_function_single(cpu_id, ipi_func, NULL, 1);
-	else
-		smp_call_function_many(tmpmask, ipi_func, NULL, 1);
-	preempt_enable();
+	if (cpu_id >= 0) {
+		int cpu = get_cpu();
+
+		if (cpu_id == cpu) {
+			ipi_func(NULL);

kernel/sched/membarrier.c +6 -7

+		}
+
+		put_cpu();
+	} else {
+		on_each_cpu_mask(tmpmask, ipi_func, NULL, true);
+	}
 
 out:
 	if (cpu_id < 0)
 		free_cpumask_var(tmpmask);
 	cpus_read_unlock();
 
-	/*
-	 * Memory barrier on the caller thread _after_ we finished
-	 * waiting for the last IPI. Matches memory barriers around
-	 * rq->curr modification in scheduler.
-	 */
-	smp_mb();	/* exit from system call is not a mb */
-
 	return 0;
 }
 

Signature · 1 line

2.28.0

Quoted 8 lines · Andy Lutomirski

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.

That said, I don't mind including self, these aren't fast calls by any means.

Quoted 7 lines

- 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.

Quoted 4 lines

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.

Quoted 6 lines

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.

Quoted 10 lines · ----- On Dec 1, 2020, at 5:16 AM, Peter Zijlstra [email protected] wrote:

Quoted 8 lines · Andy Lutomirski

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.

Quoted 3 lines

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.

Quoted 3 lines

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.

Quoted 12 lines

Quoted 8 lines

- 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.

Quoted 6 lines

Quoted 4 lines

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.

Quoted 1 line

Quoted 5 lines

Quoted 5 lines

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.

Quoted 36 lines · Mathieu Desnoyers

Quoted 10 lines · ----- On Dec 1, 2020, at 5:16 AM, Peter Zijlstra [email protected] wrote:

Quoted 8 lines · Andy Lutomirski

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.

Quoted 3 lines

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.

Quoted 3 lines

That said, I don't mind including self, these aren't fast calls by any means.

Quoted 6 lines

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.

Quoted 23 lines

Quoted 13 lines

Quoted 9 lines

- 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.

Quoted 6 lines

Quoted 4 lines

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.

Quoted 1 line

Quoted 8 lines

Quoted 5 lines

Quoted 5 lines

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.

Quoted 45 lines · ----- On Dec 1, 2020, at 1:12 PM, Andy Lutomirski [email protected] wrote:

Quoted 36 lines · Mathieu Desnoyers

Quoted 10 lines · ----- On Dec 1, 2020, at 5:16 AM, Peter Zijlstra [email protected] wrote:

Quoted 8 lines · Andy Lutomirski

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.

Quoted 3 lines

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.

Quoted 3 lines

That said, I don't mind including self, these aren't fast calls by any means.

Quoted 6 lines

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.

Quoted 2 lines

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.

But for rseq, there is no core-sync. And considering that it is invalid to issue a system call within an rseq critical section (including membarrier), I don't see what we gain by doing a rseq barrier on self ?

The only case where it really changes the semantic is for core-sync I think. And in this case, it would be adding an additional core-sync on self. I am OK with doing that considering that it will simplify use of the system call. I'm just wondering how we should document this change in the man page.

Quoted 37 lines

Quoted 23 lines

Quoted 13 lines

Quoted 9 lines

- 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.

Quoted 6 lines

Quoted 4 lines

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.

Quoted 1 line

Quoted 8 lines

Quoted 5 lines

Quoted 5 lines

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.

Quoted 2 lines

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.

For the memory barrier commands, I prefer skipping self and leaving the smp_mb at the very beginning/end of the system call. Those are the key before/after points we are synchronizing against, and those are simple to document.

Quoted 97 lines · Mathieu Desnoyers

Quoted 45 lines · ----- On Dec 1, 2020, at 1:12 PM, Andy Lutomirski [email protected] wrote:

Quoted 36 lines · Mathieu Desnoyers

Quoted 10 lines · ----- On Dec 1, 2020, at 5:16 AM, Peter Zijlstra [email protected] wrote:

Quoted 8 lines · Andy Lutomirski

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.

Quoted 3 lines

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.

Quoted 3 lines

That said, I don't mind including self, these aren't fast calls by any means.

Quoted 6 lines

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.

Quoted 2 lines

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.

But for rseq, there is no core-sync. And considering that it is invalid to issue a system call within an rseq critical section (including membarrier), I don't see what we gain by doing a rseq barrier on self ?

The only case where it really changes the semantic is for core-sync I think. And in this case, it would be adding an additional core-sync on self. I am OK with doing that considering that it will simplify use of the system call. I'm just wondering how we should document this change in the man page.

Quoted 37 lines

Quoted 23 lines

Quoted 13 lines

Quoted 9 lines

- 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.

Quoted 6 lines

Quoted 4 lines

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.

Quoted 1 line

Quoted 8 lines

Quoted 5 lines

Quoted 5 lines

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.

Quoted 8 lines

Quoted 2 lines

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.

For the memory barrier commands, I prefer skipping self and leaving the smp_mb at the very beginning/end of the system call. Those are the key before/after points we are synchronizing against, and those are simple to document.

Is there a reason that doing the barrier at the very end could make an observable difference? The two models are:

membarrier() {
smp_mb();
read a bunch of cpu_rq memory and make decisions;
execute smp_mb() on relevant cpus including self;

}

versus

membarrier() {
smp_mb();
read a bunch of cpu_rq memory and make decisions; execute smp_mb() on relevant non-self cpus;
wait for that to finish (acquire-style on the local cpu); smp_mb();
}

Is the idea that, on a sufficiently weakly ordered architecture, some remote CPU could do a store before the IPI and a local load after the membarrier() syscall might not observe the load unless the local smp_mb() is after the remote smp_mb()? If so, I'm not entirely convinced that this is observably different from the store simply occurring after the IPI, but maybe there are some gnarly situations in which this could happen.

If your concern is something along these lines, I could try to write up an appropriate comment, and I'll rework the patch.

Quoted 8 lines · Andy Lutomirski

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.

That said, I don't mind including self, these aren't fast calls by any means.

Quoted 7 lines

- 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.

Quoted 5 lines

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.

Quoted 28 lines

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.

Signed-off-by Andy Lutomirski <[email protected]>

kernel/sched/membarrier.c +10 -6

@@ -228,25 +258,33 @@ static int membarrier_private_expedited(int flags, int cpu_id)
              rcu_read_unlock();
      }
 
-     preempt_disable();
-     if (cpu_id >= 0)
-             smp_call_function_single(cpu_id, ipi_func, NULL, 1);
-     else
-             smp_call_function_many(tmpmask, ipi_func, NULL, 1);
-     preempt_enable();
+     if (cpu_id >= 0) {
+             int cpu = get_cpu();
+
+             if (cpu_id == cpu) {
+                     ipi_func(NULL);
+             } else {
+                     smp_call_function_single(cpu_id, ipi_func, NULL, 1);
+                     /*
+                      * This is analogous to the smp_mb() at the beginning
+                      * of the function -- exit from a system call is not a

kernel/sched/membarrier.c static int membarrier_private_expedited(int flags, int cpu_id) · line 275

Quoted 5 lines

kernel/sched/membarrier.c +5 -0

+                      * barrier.  We only need this if we're targeting a
+                      * specific remote CPU, though -- otherwise ipi_func()
+                      * would serves the same purpose.
+                      */
+                     smp_mb();

smp_call_function_single(.wait=1) already orders against completion of the IPI. Do we really need more?

Quoted 9 lines · Peter Zijlstra

Quoted 6 lines

- 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.

I looked for a bit, and I couldn't find anything in the SDM or APM to support this, and I would be rather surprised if other architectures synchronize their instruction streams on interrupt delivery. On architectures without hardware I$ coherency and with actual fast interrupts, I would be surprised if interrupts ensured I$ coherency with prior writes from other cores.

Quoted 2 lines

smp_call_function_single(.wait=1) already orders against completion of the IPI. Do we really need more?

What kind of order does it provide? A quick skim of the code suggests that it's an acquire barrier, but I think we need a full sequential consistency barrier, at least on sufficiently weakly ordered architectures. On x86, loads are ordered and this is probably irrelevant. Also, this barrier was already there (it's the one I deleted below), and I think that removing it should be its own patch if we want to go that route.