Allow PowerPC to skip the full memory barrier in switch_mm(), and
only issue the barrier when scheduling into a task belonging to a
process that has registered to use expedited private.
Threads targeting the same VM but which belong to different thread
groups is a tricky case. It has a few consequences:
It turns out that we cannot rely on get_nr_threads(p) to count the
number of threads using a VM. We can use
(atomic_read(&mm->mm_users) == 1 && get_nr_threads(p) == 1)
instead to skip the synchronize_sched() for cases where the VM only has
a single user, and that user only has a single thread.
It also turns out that we cannot use for_each_thread() to set
thread flags in all threads using a VM, as it only iterates on the
thread group.
Therefore, test the membarrier state variable directly rather than
relying on thread flags. This means
membarrier_register_private_expedited() needs to set the
MEMBARRIER_STATE_PRIVATE_EXPEDITED flag, issue synchronize_sched(), and
only then set MEMBARRIER_STATE_PRIVATE_EXPEDITED_READY which allows
private expedited membarrier commands to succeed.
membarrier_arch_switch_mm() now tests for the
MEMBARRIER_STATE_PRIVATE_EXPEDITED flag.
Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
CC: Paul E. McKenney <redacted>
CC: Boqun Feng <redacted>
CC: Andrew Hunter <redacted>
CC: Maged Michael <redacted>
CC: Avi Kivity <redacted>
CC: Benjamin Herrenschmidt <benh@kernel.crashing.org>
CC: Paul Mackerras <redacted>
CC: Michael Ellerman <mpe@ellerman.id.au>
CC: Dave Watson <redacted>
CC: Alan Stern <stern@rowland.harvard.edu>
CC: Will Deacon <redacted>
CC: Andy Lutomirski <luto@kernel.org>
CC: Ingo Molnar <mingo@redhat.com>
CC: Alexander Viro <viro@zeniv.linux.org.uk>
CC: Nicholas Piggin <npiggin@gmail.com>
CC: linuxppc-dev@lists.ozlabs.org
CC: linux-arch@vger.kernel.org
---
Changes since v1:
- Use test_ti_thread_flag(next, ...) instead of test_thread_flag() in
powerpc membarrier_arch_sched_in(), given that we want to specifically
check the next thread state.
- Add missing ARCH_HAS_MEMBARRIER_HOOKS in Kconfig.
- Use task_thread_info() to pass thread_info from task to
*_ti_thread_flag().
Changes since v2:
- Move membarrier_arch_sched_in() call to finish_task_switch().
- Check for NULL t->mm in membarrier_arch_fork().
- Use membarrier_sched_in() in generic code, which invokes the
arch-specific membarrier_arch_sched_in(). This fixes allnoconfig
build on PowerPC.
- Move asm/membarrier.h include under CONFIG_MEMBARRIER, fixing
allnoconfig build on PowerPC.
- Build and runtime tested on PowerPC.
Changes since v3:
- Simply rely on copy_mm() to copy the membarrier_private_expedited mm
field on fork.
- powerpc: test thread flag instead of reading
membarrier_private_expedited in membarrier_arch_fork().
- powerpc: skip memory barrier in membarrier_arch_sched_in() if coming
from kernel thread, since mmdrop() implies a full barrier.
- Set membarrier_private_expedited to 1 only after arch registration
code, thus eliminating a race where concurrent commands could succeed
when they should fail if issued concurrently with process
registration.
- Use READ_ONCE() for membarrier_private_expedited field access in
membarrier_private_expedited. Matches WRITE_ONCE() performed in
process registration.
Changes since v4:
- Move powerpc hook from sched_in() to switch_mm(), based on feedback
from Nicholas Piggin.
Changes since v5:
- Rebase on v4.14-rc6.
- Fold "Fix: membarrier: Handle CLONE_VM + !CLONE_THREAD correctly on
powerpc (v2)"
Changes since v6:
- Rename MEMBARRIER_STATE_SWITCH_MM to MEMBARRIER_STATE_PRIVATE_EXPEDITED.
---
MAINTAINERS | 1 +
arch/powerpc/Kconfig | 1 +
arch/powerpc/include/asm/membarrier.h | 26 ++++++++++++++++++++++++++
arch/powerpc/mm/mmu_context.c | 7 +++++++
include/linux/sched/mm.h | 13 ++++++++++++-
init/Kconfig | 3 +++
kernel/sched/core.c | 10 ----------
kernel/sched/membarrier.c | 8 ++++++++
8 files changed, 58 insertions(+), 11 deletions(-)
create mode 100644 arch/powerpc/include/asm/membarrier.h
Yeah, so I have renamed this to ARCH_HAS_MEMBARRIER_CALLBACKS, and propagated it
through the rest of the patches. "Callback" is the canonical name, and I also
cringe every time I see 'hook'.
Please let me know if there are any big objections against this minor cleanup.
Thanks,
Ingo
----- On Feb 5, 2018, at 3:22 PM, Ingo Molnar mingo@kernel.org wrote:
* Mathieu Desnoyers [off-list ref] wrote:
quoted
+config ARCH_HAS_MEMBARRIER_HOOKS
+ bool
Yeah, so I have renamed this to ARCH_HAS_MEMBARRIER_CALLBACKS, and propagated it
through the rest of the patches. "Callback" is the canonical name, and I also
cringe every time I see 'hook'.
Please let me know if there are any big objections against this minor cleanup.
Le 29/01/2018 à 21:20, Mathieu Desnoyers a écrit :
Allow PowerPC to skip the full memory barrier in switch_mm(), and
only issue the barrier when scheduling into a task belonging to a
process that has registered to use expedited private.
Threads targeting the same VM but which belong to different thread
groups is a tricky case. It has a few consequences:
It turns out that we cannot rely on get_nr_threads(p) to count the
number of threads using a VM. We can use
(atomic_read(&mm->mm_users) == 1 && get_nr_threads(p) == 1)
instead to skip the synchronize_sched() for cases where the VM only has
a single user, and that user only has a single thread.
It also turns out that we cannot use for_each_thread() to set
thread flags in all threads using a VM, as it only iterates on the
thread group.
Therefore, test the membarrier state variable directly rather than
relying on thread flags. This means
membarrier_register_private_expedited() needs to set the
MEMBARRIER_STATE_PRIVATE_EXPEDITED flag, issue synchronize_sched(), and
only then set MEMBARRIER_STATE_PRIVATE_EXPEDITED_READY which allows
private expedited membarrier commands to succeed.
membarrier_arch_switch_mm() now tests for the
MEMBARRIER_STATE_PRIVATE_EXPEDITED flag.
Looking at switch_mm_irqs_off(), I found it more complex than expected and found that this patch is
the reason for that complexity.
Before the patch (ie in kernel 4.14), we have:
00000000 <switch_mm_irqs_off>:
0: 81 24 01 c8 lwz r9,456(r4)
4: 71 29 00 01 andi. r9,r9,1
8: 40 82 00 1c bne 24 <switch_mm_irqs_off+0x24>
c: 39 24 01 c8 addi r9,r4,456
10: 39 40 00 01 li r10,1
14: 7d 00 48 28 lwarx r8,0,r9
18: 7d 08 53 78 or r8,r8,r10
1c: 7d 00 49 2d stwcx. r8,0,r9
20: 40 c2 ff f4 bne- 14 <switch_mm_irqs_off+0x14>
24: 7c 04 18 40 cmplw r4,r3
28: 81 24 00 24 lwz r9,36(r4)
2c: 91 25 04 4c stw r9,1100(r5)
30: 4d 82 00 20 beqlr
34: 48 00 00 00 b 34 <switch_mm_irqs_off+0x34>
34: R_PPC_REL24 switch_mmu_context
After the patch (ie in 5.13-rc6), that now is:
00000000 <switch_mm_irqs_off>:
0: 81 24 02 18 lwz r9,536(r4)
4: 71 29 00 01 andi. r9,r9,1
8: 41 82 00 24 beq 2c <switch_mm_irqs_off+0x2c>
c: 7c 04 18 40 cmplw r4,r3
10: 81 24 00 24 lwz r9,36(r4)
14: 91 25 04 d0 stw r9,1232(r5)
18: 4d 82 00 20 beqlr
1c: 81 24 00 28 lwz r9,40(r4)
20: 71 29 00 0a andi. r9,r9,10
24: 40 82 00 34 bne 58 <switch_mm_irqs_off+0x58>
28: 48 00 00 00 b 28 <switch_mm_irqs_off+0x28>
28: R_PPC_REL24 switch_mmu_context
2c: 39 24 02 18 addi r9,r4,536
30: 39 40 00 01 li r10,1
34: 7d 00 48 28 lwarx r8,0,r9
38: 7d 08 53 78 or r8,r8,r10
3c: 7d 00 49 2d stwcx. r8,0,r9
40: 40 a2 ff f4 bne 34 <switch_mm_irqs_off+0x34>
44: 7c 04 18 40 cmplw r4,r3
48: 81 24 00 24 lwz r9,36(r4)
4c: 91 25 04 d0 stw r9,1232(r5)
50: 4d 82 00 20 beqlr
54: 48 00 00 00 b 54 <switch_mm_irqs_off+0x54>
54: R_PPC_REL24 switch_mmu_context
58: 2c 03 00 00 cmpwi r3,0
5c: 41 82 ff cc beq 28 <switch_mm_irqs_off+0x28>
60: 48 00 00 00 b 60 <switch_mm_irqs_off+0x60>
60: R_PPC_REL24 switch_mmu_context
Especially, the comparison of 'prev' to 0 is pointless as both cases end up with just branching to
'switch_mmu_context'
I don't understand all that complexity to just replace a simple 'smp_mb__after_unlock_lock()'.
#define smp_mb__after_unlock_lock() smp_mb()
#define smp_mb() barrier()
# define barrier() __asm__ __volatile__("": : :"memory")
Am I missing some subtility ?
Thanks
Christophe
----- On Jun 18, 2021, at 1:13 PM, Christophe Leroy christophe.leroy@csgroup.eu wrote:
[...]
I don't understand all that complexity to just replace a simple
'smp_mb__after_unlock_lock()'.
#define smp_mb__after_unlock_lock() smp_mb()
#define smp_mb() barrier()
# define barrier() __asm__ __volatile__("": : :"memory")
Am I missing some subtility ?
On powerpc CONFIG_SMP, smp_mb() is actually defined as:
#define smp_mb() __smp_mb()
#define __smp_mb() mb()
#define mb() __asm__ __volatile__ ("sync" : : : "memory")
So the original motivation here was to skip a "sync" instruction whenever
switching between threads which are part of the same process. But based on
recent discussions, I suspect my implementation may be inaccurately doing
so though.
Thanks,
Mathieu
Le 18/06/2021 à 19:26, Mathieu Desnoyers a écrit :
----- On Jun 18, 2021, at 1:13 PM, Christophe Leroy christophe.leroy@csgroup.eu wrote:
[...]
quoted
I don't understand all that complexity to just replace a simple
'smp_mb__after_unlock_lock()'.
#define smp_mb__after_unlock_lock() smp_mb()
#define smp_mb() barrier()
# define barrier() __asm__ __volatile__("": : :"memory")
Am I missing some subtility ?
On powerpc CONFIG_SMP, smp_mb() is actually defined as:
#define smp_mb() __smp_mb()
#define __smp_mb() mb()
#define mb() __asm__ __volatile__ ("sync" : : : "memory")
So the original motivation here was to skip a "sync" instruction whenever
switching between threads which are part of the same process. But based on
recent discussions, I suspect my implementation may be inaccurately doing
so though.
I see.
Then, if you think a 'sync' is a concern, shouldn't we try and remove the forest of 'sync' in the
I/O accessors ?
I can't really understand why we need all those 'sync' and 'isync' and 'twi' around the accesses
whereas I/O memory is usually mapped as 'Guarded' so memory access ordering is already garantied.
I'm sure we'll save a lot with that.
Christophe
On Sat, Jun 19, 2021 at 11:35:34AM +0200, Christophe Leroy wrote:
Le 18/06/2021 à 19:26, Mathieu Desnoyers a écrit :
quoted
----- On Jun 18, 2021, at 1:13 PM, Christophe Leroy
christophe.leroy@csgroup.eu wrote:
[...]
quoted
I don't understand all that complexity to just replace a simple
'smp_mb__after_unlock_lock()'.
#define smp_mb__after_unlock_lock() smp_mb()
#define smp_mb() barrier()
# define barrier() __asm__ __volatile__("": : :"memory")
Am I missing some subtility ?
On powerpc CONFIG_SMP, smp_mb() is actually defined as:
#define smp_mb() __smp_mb()
#define __smp_mb() mb()
#define mb() __asm__ __volatile__ ("sync" : : : "memory")
So the original motivation here was to skip a "sync" instruction whenever
switching between threads which are part of the same process. But based on
recent discussions, I suspect my implementation may be inaccurately doing
so though.
I see.
Then, if you think a 'sync' is a concern, shouldn't we try and remove the
forest of 'sync' in the I/O accessors ?
I can't really understand why we need all those 'sync' and 'isync' and
'twi' around the accesses whereas I/O memory is usually mapped as 'Guarded'
so memory access ordering is already garantied.
I'm sure we'll save a lot with that.
The point of the twi in the I/O accessors was to make things easier to
debug if the accesses fail: for the twi insn to complete the load will
have to have completed as well. On a correctly working system you never
should need this (until something fails ;-) )
Without the twi you might need to enforce ordering in some cases still.
The twi is a very heavy hammer, but some of that that gives us is no
doubt actually needed.
Segher
Le 19/06/2021 à 17:02, Segher Boessenkool a écrit :
On Sat, Jun 19, 2021 at 11:35:34AM +0200, Christophe Leroy wrote:
quoted
Le 18/06/2021 à 19:26, Mathieu Desnoyers a écrit :
quoted
----- On Jun 18, 2021, at 1:13 PM, Christophe Leroy
christophe.leroy@csgroup.eu wrote:
[...]
quoted
I don't understand all that complexity to just replace a simple
'smp_mb__after_unlock_lock()'.
#define smp_mb__after_unlock_lock() smp_mb()
#define smp_mb() barrier()
# define barrier() __asm__ __volatile__("": : :"memory")
Am I missing some subtility ?
On powerpc CONFIG_SMP, smp_mb() is actually defined as:
#define smp_mb() __smp_mb()
#define __smp_mb() mb()
#define mb() __asm__ __volatile__ ("sync" : : : "memory")
So the original motivation here was to skip a "sync" instruction whenever
switching between threads which are part of the same process. But based on
recent discussions, I suspect my implementation may be inaccurately doing
so though.
I see.
Then, if you think a 'sync' is a concern, shouldn't we try and remove the
forest of 'sync' in the I/O accessors ?
I can't really understand why we need all those 'sync' and 'isync' and
'twi' around the accesses whereas I/O memory is usually mapped as 'Guarded'
so memory access ordering is already garantied.
I'm sure we'll save a lot with that.
The point of the twi in the I/O accessors was to make things easier to
debug if the accesses fail: for the twi insn to complete the load will
have to have completed as well. On a correctly working system you never
should need this (until something fails ;-) )
Without the twi you might need to enforce ordering in some cases still.
The twi is a very heavy hammer, but some of that that gives us is no
doubt actually needed.
Well, I've always been quite perplex about that. According to the documentation of the 8xx, if a bus
error or something happens on an I/O access, the exception will be accounted on the instruction
which does the access. But based on the following function, I understand that some version of
powerpc do generate the trap on the instruction which was being executed at the time the I/O access
failed, not the instruction that does the access itself ?
/*
* I/O accesses can cause machine checks on powermacs.
* Check if the NIP corresponds to the address of a sync
* instruction for which there is an entry in the exception
* table.
* -- paulus.
*/
static inline int check_io_access(struct pt_regs *regs)
{
#ifdef CONFIG_PPC32
unsigned long msr = regs->msr;
const struct exception_table_entry *entry;
unsigned int *nip = (unsigned int *)regs->nip;
if (((msr & 0xffff0000) == 0 || (msr & (0x80000 | 0x40000)))
&& (entry = search_exception_tables(regs->nip)) != NULL) {
/*
* Check that it's a sync instruction, or somewhere
* in the twi; isync; nop sequence that inb/inw/inl uses.
* As the address is in the exception table
* we should be able to read the instr there.
* For the debug message, we look at the preceding
* load or store.
*/
if (*nip == PPC_INST_NOP)
nip -= 2;
else if (*nip == PPC_INST_ISYNC)
--nip;
if (*nip == PPC_INST_SYNC || (*nip >> 26) == OP_TRAP) {
unsigned int rb;
--nip;
rb = (*nip >> 11) & 0x1f;
printk(KERN_DEBUG "%s bad port %lx at %p\n",
(*nip & 0x100)? "OUT to": "IN from",
regs->gpr[rb] - _IO_BASE, nip);
regs->msr |= MSR_RI;
regs->nip = extable_fixup(entry);
return 1;
}
}
#endif /* CONFIG_PPC32 */
return 0;
}
Am I right ?
It is not only the twi which bother's me in the I/O accessors but also the sync/isync and stuff.
A write typically is
sync
stw
A read is
sync
lwz
twi
isync
Taking into account that HW ordering is garanteed by the fact that __iomem is guarded, isn't the
'memory' clobber enough as a barrier ?
Thanks
Christophe
Hi!
On Mon, Jun 21, 2021 at 04:11:04PM +0200, Christophe Leroy wrote:
Le 19/06/2021 à 17:02, Segher Boessenkool a écrit :
quoted
The point of the twi in the I/O accessors was to make things easier to
debug if the accesses fail: for the twi insn to complete the load will
have to have completed as well. On a correctly working system you never
should need this (until something fails ;-) )
Without the twi you might need to enforce ordering in some cases still.
The twi is a very heavy hammer, but some of that that gives us is no
doubt actually needed.
Well, I've always been quite perplex about that. According to the
documentation of the 8xx, if a bus error or something happens on an I/O
access, the exception will be accounted on the instruction which does the
access. But based on the following function, I understand that some version
of powerpc do generate the trap on the instruction which was being executed
at the time the I/O access failed, not the instruction that does the access
itself ?
Trap instructions are never speculated (this may not be architectural,
but it is true on all existing implementations). So the instructions
after the twi;isync will not execute until the twi itself has finished,
and that cannot happen before the preceding load has (because it uses
the loaded register).
Now, some I/O accesses can cause machine checks. Machine checks are
asynchronous and can be hard to correlate to specific load insns, and
worse, you may not even have the address loaded from in architected
registers anymore. Since I/O accesses often take *long*, tens or even
hundreds of cycles is not unusual, this can be a challenge.
To recover from machine checks you typically need special debug hardware
and/or software. For the Apple machines those are not so easy to come
by. This "twi after loads" thing made it pretty easy to figure out
where your code was going wrong.
And it isn't as slow as it may sound: typically you really need to have
the result of the load before you can go on do useful work anyway, and
loads from I/O are slow non-posted things.
/*
* I/O accesses can cause machine checks on powermacs.
* Check if the NIP corresponds to the address of a sync
* instruction for which there is an entry in the exception
* table.
* -- paulus.
*/
I suspect this is from before the twi thing was added?
It is not only the twi which bother's me in the I/O accessors but also the
sync/isync and stuff.
A write typically is
sync
stw
A read is
sync
lwz
twi
isync
Taking into account that HW ordering is garanteed by the fact that __iomem
is guarded,
Yes. But machine checks are asynchronous :-)
isn't the 'memory' clobber enough as a barrier ?
A "memory" clobber isn't a barrier of any kind. "Compiler barriers" do
not exist.
The only thing such a clobber does is it tells the compiler that this
inline asm can access some memory, and we do not say at what address.
So the compiler cannot reorder this asm with other memory accesses. It
has no other effects, no magical effects, and it is not comparable to
actual barrier instructions (that actually tell the hardware that some
certain ordering is required).
"Compiler barrier" is a harmful misnomer: language shapes thoughts,
using misleading names causes misguided thoughts.
Anyway :-)
The isync is simply to make sure the code after it does not start before
the code before it has completed. The sync before I am not sure.
Segher