| Message ID | 20170724042803.25848-3-benh@kernel.crashing.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 1a92a80ad386a1a6e3b36d576d52a1a456394b70 |
| Headers | show |
On Mon, 24 Jul 2017 14:28:00 +1000 Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote: > There is no guarantee that the various isync's involved with > the context switch will order the update of the CPU mask with > the first TLB entry for the new context being loaded by the HW. > > Be safe here and add a memory barrier to order any subsequent > load/store which may bring entries into the TLB. > > The corresponding barrier on the other side already exists as > pte updates use pte_xchg() which uses __cmpxchg_u64 which has > a sync after the atomic operation. > > Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org> > --- > arch/powerpc/include/asm/mmu_context.h | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/arch/powerpc/include/asm/mmu_context.h b/arch/powerpc/include/asm/mmu_context.h > index ed9a36ee3107..ff1aeb2cd19f 100644 > --- a/arch/powerpc/include/asm/mmu_context.h > +++ b/arch/powerpc/include/asm/mmu_context.h > @@ -110,6 +110,7 @@ static inline void switch_mm_irqs_off(struct mm_struct *prev, > /* Mark this context has been used on the new CPU */ > if (!cpumask_test_cpu(smp_processor_id(), mm_cpumask(next))) { > cpumask_set_cpu(smp_processor_id(), mm_cpumask(next)); > + smp_mb(); > new_on_cpu = true; > } > I think this is the right thing to do, but it should be commented. Is hwsync the right barrier? (i.e., it will order the page table walk) Thanks, Nick
On Mon, 2017-07-24 at 21:20 +1000, Nicholas Piggin wrote: > I think this is the right thing to do, but it should be commented. > Is hwsync the right barrier? (i.e., it will order the page table walk) This is an open question, I've asked the architects and HW guys and waiting for an answer. That said, are we really trying to order the page table walk or are we trying to order any (speculative or not) load/store that may trigger a page table update ? Cheers, Ben.
On Mon, 24 Jul 2017 21:20:07 +1000 Nicholas Piggin <npiggin@gmail.com> wrote: > On Mon, 24 Jul 2017 14:28:00 +1000 > Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote: > > > There is no guarantee that the various isync's involved with > > the context switch will order the update of the CPU mask with > > the first TLB entry for the new context being loaded by the HW. > > > > Be safe here and add a memory barrier to order any subsequent > > load/store which may bring entries into the TLB. > > > > The corresponding barrier on the other side already exists as > > pte updates use pte_xchg() which uses __cmpxchg_u64 which has > > a sync after the atomic operation. > > > > Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org> > > --- > > arch/powerpc/include/asm/mmu_context.h | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/arch/powerpc/include/asm/mmu_context.h b/arch/powerpc/include/asm/mmu_context.h > > index ed9a36ee3107..ff1aeb2cd19f 100644 > > --- a/arch/powerpc/include/asm/mmu_context.h > > +++ b/arch/powerpc/include/asm/mmu_context.h > > @@ -110,6 +110,7 @@ static inline void switch_mm_irqs_off(struct mm_struct *prev, > > /* Mark this context has been used on the new CPU */ > > if (!cpumask_test_cpu(smp_processor_id(), mm_cpumask(next))) { > > cpumask_set_cpu(smp_processor_id(), mm_cpumask(next)); > > + smp_mb(); > > new_on_cpu = true; > > } > > > > I think this is the right thing to do, but it should be commented. > Is hwsync the right barrier? (i.e., it will order the page table walk) After some offline discussion, I think we have an agreement that this is the right barrier, as it orders with the subsequent load of next->context.id that the mtpid depends on (or slbmte for HPT). So we should have a comment here to that effect, and including the pte_xchg comments from your changelog. Some comment (at least refer back to here) added at pte_xchg too please. Other than that your series seems good to me if you repost it you can add Reviewed-by: Nicholas Piggin <npiggin@gmail.com> This one out of the series is the bugfix so it should go to stable as well, right? Thanks, Nick
On Fri, 2017-08-11 at 21:06 +1000, Nicholas Piggin wrote: > Other than that your series seems good to me if you repost it you > can add > > Reviewed-by: Nicholas Piggin <npiggin@gmail.com> > > This one out of the series is the bugfix so it should go to stable > as well, right? Yup. Ben.
Nicholas Piggin <npiggin@gmail.com> writes: > On Mon, 24 Jul 2017 21:20:07 +1000 > Nicholas Piggin <npiggin@gmail.com> wrote: > >> On Mon, 24 Jul 2017 14:28:00 +1000 >> Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote: >> >> > There is no guarantee that the various isync's involved with >> > the context switch will order the update of the CPU mask with >> > the first TLB entry for the new context being loaded by the HW. >> > >> > Be safe here and add a memory barrier to order any subsequent >> > load/store which may bring entries into the TLB. >> > >> > The corresponding barrier on the other side already exists as >> > pte updates use pte_xchg() which uses __cmpxchg_u64 which has >> > a sync after the atomic operation. >> > >> > Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org> >> > --- >> > arch/powerpc/include/asm/mmu_context.h | 1 + >> > 1 file changed, 1 insertion(+) >> > >> > diff --git a/arch/powerpc/include/asm/mmu_context.h b/arch/powerpc/include/asm/mmu_context.h >> > index ed9a36ee3107..ff1aeb2cd19f 100644 >> > --- a/arch/powerpc/include/asm/mmu_context.h >> > +++ b/arch/powerpc/include/asm/mmu_context.h >> > @@ -110,6 +110,7 @@ static inline void switch_mm_irqs_off(struct mm_struct *prev, >> > /* Mark this context has been used on the new CPU */ >> > if (!cpumask_test_cpu(smp_processor_id(), mm_cpumask(next))) { >> > cpumask_set_cpu(smp_processor_id(), mm_cpumask(next)); >> > + smp_mb(); >> > new_on_cpu = true; >> > } >> > >> >> I think this is the right thing to do, but it should be commented. >> Is hwsync the right barrier? (i.e., it will order the page table walk) > > After some offline discussion, I think we have an agreement that > this is the right barrier, as it orders with the subsequent load > of next->context.id that the mtpid depends on (or slbmte for HPT). > > So we should have a comment here to that effect, and including > the pte_xchg comments from your changelog. Some comment (at least > refer back to here) added at pte_xchg too please. > > Other than that your series seems good to me if you repost it you > can add > > Reviewed-by: Nicholas Piggin <npiggin@gmail.com> > > This one out of the series is the bugfix so it should go to stable > as well, right? So I'm waiting on a v2? cheers
On Mon, 2017-07-24 at 04:28:00 UTC, Benjamin Herrenschmidt wrote: > There is no guarantee that the various isync's involved with > the context switch will order the update of the CPU mask with > the first TLB entry for the new context being loaded by the HW. > > Be safe here and add a memory barrier to order any subsequent > load/store which may bring entries into the TLB. > > The corresponding barrier on the other side already exists as > pte updates use pte_xchg() which uses __cmpxchg_u64 which has > a sync after the atomic operation. > > Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org> > Reviewed-by: Nicholas Piggin <npiggin@gmail.com> Applied to powerpc fixes, thanks. https://git.kernel.org/powerpc/c/1a92a80ad386a1a6e3b36d576d52a1 cheers
diff --git a/arch/powerpc/include/asm/mmu_context.h b/arch/powerpc/include/asm/mmu_context.h index ed9a36ee3107..ff1aeb2cd19f 100644 --- a/arch/powerpc/include/asm/mmu_context.h +++ b/arch/powerpc/include/asm/mmu_context.h @@ -110,6 +110,7 @@ static inline void switch_mm_irqs_off(struct mm_struct *prev, /* Mark this context has been used on the new CPU */ if (!cpumask_test_cpu(smp_processor_id(), mm_cpumask(next))) { cpumask_set_cpu(smp_processor_id(), mm_cpumask(next)); + smp_mb(); new_on_cpu = true; }
There is no guarantee that the various isync's involved with the context switch will order the update of the CPU mask with the first TLB entry for the new context being loaded by the HW. Be safe here and add a memory barrier to order any subsequent load/store which may bring entries into the TLB. The corresponding barrier on the other side already exists as pte updates use pte_xchg() which uses __cmpxchg_u64 which has a sync after the atomic operation. Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org> --- arch/powerpc/include/asm/mmu_context.h | 1 + 1 file changed, 1 insertion(+)