Messages in this thread | | | Subject | Re: [PATCH for 4.16 v7 02/11] powerpc: membarrier: Skip memory barrier in switch_mm() | From | Christophe Leroy <> | Date | Mon, 21 Jun 2021 16:11:04 +0200 |
| |
Le 19/06/2021 à 17:02, Segher Boessenkool a écrit : > On Sat, Jun 19, 2021 at 11:35:34AM +0200, Christophe Leroy wrote: >> >> >> 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: >>> [...] >>>> >>>> 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
| |