On 09/07/2012 02:34 PM, Scott Wood wrote:
On 09/06/2012 11:41 PM, Benjamin Herrenschmidt wrote:
quoted
quoted
diff --git a/arch/powerpc/include/asm/mmu-book3e.h b/arch/powerpc/include/asm/mmu-book3e.h
index eeabcdb..3072aa0 100644
--- a/arch/powerpc/include/asm/mmu-book3e.h
+++ b/arch/powerpc/include/asm/mmu-book3e.h
@@ -264,8 +264,21 @@ extern struct mmu_psize_def mmu_psize_defs[MMU_PAGE_COUNT];
extern int mmu_linear_psize;
extern int mmu_vmemmap_psize;
+struct book3e_tlb_per_core {
+ /* For software way selection, as on Freescale TLB1 */
+ u8 esel_next, esel_max, esel_first;
+
+ /* Per-core spinlock for e6500 TLB handlers (no tlbsrx.) */
+ u8 lock;
+};
I'm no fan of the name ... tlb_core_data ?
tlb_core_data is fine with me.
quoted
Probably don't even need the book3e prefix really.
Right, it's already in a book3e file.
quoted
quoted
#if defined(CONFIG_PPC_STD_MMU_64)
/* 64-bit classic hash table MMU */
diff --git a/arch/powerpc/include/asm/paca.h b/arch/powerpc/include/asm/paca.h
index daf813f..4e18bb5 100644
--- a/arch/powerpc/include/asm/paca.h
+++ b/arch/powerpc/include/asm/paca.h
@@ -108,6 +108,12 @@ struct paca_struct {
/* Keep pgd in the same cacheline as the start of extlb */
pgd_t *pgd __attribute__((aligned(0x80))); /* Current PGD */
pgd_t *kernel_pgd; /* Kernel PGD */
+
+ struct book3e_tlb_per_core tlb_per_core;
+
+ /* Points to the tlb_per_core of the first thread on this core. */
+ struct book3e_tlb_per_core *tlb_per_core_ptr;
+
That's gross. Can't you allocate them elsewhere and then populate the
PACA pointers ?
That would be one more cache line that misses need... and the threads
share cache, so there's no ping-pong.
quoted
quoted
@@ -142,6 +173,8 @@ static void check_smt_enabled(void)
of_node_put(dn);
}
}
+
+ setup_tlb_per_core();
}
I'd rather you move that to the caller
OK.
quoted
quoted
+/*
+ * TLB miss handling for e6500 and derivatives, using hardware tablewalk.
+ *
+ * Linear mapping is bolted: no virtual page table or nested TLB misses
+ * Indirect entries in TLB1, hardware loads resulting direct entries
+ * into TLB0
+ * No HES or NV hint on TLB1, so we need to do software round-robin
+ * No tlbsrx. so we need a spinlock, and we have to deal
+ * with MAS-damage caused by tlbsx
Ouch ... so for every indirect entry you have to take a lock, backup the
MAS, do a tlbsx, restore the MAS, insert the entry and drop the lock ?
Pretty much (only a couple of the MASes need to be restored).
quoted
After all that, do you have some bullets left for the HW designers ?
They seem to not care much about making our lives easier, only how bad
the benchmarks will be without it -- and they seem to think TLB miss
performance is no longer important since we won't take them as often
with hardware tablewalk. I suspect they'll be regretting that when they
see workloads that thrash TLB1's ability to hold 2MiB indirect pages.
Then it'll probably be "why can't you use larger page tables?" :-P
quoted
quoted
+tlb_miss_common_e6500:
+ /*
+ * Search if we already have an indirect entry for that virtual
+ * address, and if we do, bail out.
+ *
+ * MAS6:IND should be already set based on MAS4
+ */
+ addi r10,r11,PERCORE_TLB_LOCK
+1: lbarx r15,0,r10
+ cmpdi r15,0
+ bne 2f
+ li r15,1
+ stbcx. r15,0,r10
No need for barriers here ?
I don't think so. We're not guarding memory accesses, just the
tlbsx+tlbwe. At least on FSL cores those instructions have enough
internal sync that isync shouldn't be needed (according to the core
manual tlbsx, tlbwe, and stbcx. all have presync and postsync, so
nothing else should be able to run at the same time). And this is
FSL-specific code. :-)
quoted
quoted
#endif /* CONFIG_PPC64 */
@@ -377,7 +382,7 @@ void tlb_flush_pgtable(struct mmu_gather *tlb, unsigned long address)
{
int tsize = mmu_psize_defs[mmu_pte_psize].enc;
- if (book3e_htw_enabled) {
+ if (book3e_htw_mode) {
Make it if (boot3e_htw_enabled != PPC_HTW_NONE)
Seems a little verbose, but OK.
Same with things like this, I guess:
book3e_htw_mode ? "enabled" : "not supported"
-Scott
That would be one more cache line that misses need... and the threads
share cache, so there's no ping-pong.
Ok, keep it that way then.
quoted
quoted
After all that, do you have some bullets left for the HW designers ?
They seem to not care much about making our lives easier, only how bad
the benchmarks will be without it -- and they seem to think TLB miss
performance is no longer important since we won't take them as often
with hardware tablewalk. I suspect they'll be regretting that when they
see workloads that thrash TLB1's ability to hold 2MiB indirect pages.
Then it'll probably be "why can't you use larger page tables?" :-P
Didn't you simulate ?
quoted
quoted
quoted
+tlb_miss_common_e6500:
+ /*
+ * Search if we already have an indirect entry for that virtual
+ * address, and if we do, bail out.
+ *
+ * MAS6:IND should be already set based on MAS4
+ */
+ addi r10,r11,PERCORE_TLB_LOCK
+1: lbarx r15,0,r10
+ cmpdi r15,0
+ bne 2f
+ li r15,1
+ stbcx. r15,0,r10
No need for barriers here ?
I don't think so. We're not guarding memory accesses, just the
tlbsx+tlbwe. At least on FSL cores those instructions have enough
internal sync that isync shouldn't be needed (according to the core
manual tlbsx, tlbwe, and stbcx. all have presync and postsync, so
nothing else should be able to run at the same time). And this is
FSL-specific code. :-)
Sadly...
quoted
quoted
quoted
#endif /* CONFIG_PPC64 */
@@ -377,7 +382,7 @@ void tlb_flush_pgtable(struct mmu_gather *tlb, unsigned long address)
{
int tsize = mmu_psize_defs[mmu_pte_psize].enc;
- if (book3e_htw_enabled) {
+ if (book3e_htw_mode) {
Make it if (boot3e_htw_enabled != PPC_HTW_NONE)
Seems a little verbose, but OK.
Same with things like this, I guess:
book3e_htw_mode ? "enabled" : "not supported"
Well, it's no longer a boolean so ...
BTW. On another note, can you pickup Ananth series for larger address
space (minus the one patch that breaks the BookE build, it shouldn't
matter) and see if there's any runtime issue on BookE 64 ? (And whether
the larger address space actually works for you too, using something
like high up mmap tests)
Cheers,
Ben.
On 08.09.2012 [09:50:04 +1000], Benjamin Herrenschmidt wrote:
<snip>
BTW. On another note, can you pickup Ananth series for larger address
I think you mean Aneesh here? Just to help Scott find the thread.
Thanks,
Nish
space (minus the one patch that breaks the BookE build, it shouldn't
matter) and see if there's any runtime issue on BookE 64 ? (And whether
the larger address space actually works for you too, using something
like high up mmap tests)
Cheers,
Ben.
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
On Fri, 2012-09-07 at 17:55 -0700, Nishanth Aravamudan wrote:
On 08.09.2012 [09:50:04 +1000], Benjamin Herrenschmidt wrote:
<snip>
quoted
BTW. On another note, can you pickup Ananth series for larger address
I think you mean Aneesh here? Just to help Scott find the thread.
Ah yes, sorry, my bad.
Cheers,
Ben.
Thanks,
Nish
quoted
space (minus the one patch that breaks the BookE build, it shouldn't
matter) and see if there's any runtime issue on BookE 64 ? (And whether
the larger address space actually works for you too, using something
like high up mmap tests)
Cheers,
Ben.
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
On 09/07/2012 06:50 PM, Benjamin Herrenschmidt wrote:
quoted
quoted
quoted
quoted
#endif /* CONFIG_PPC64 */
@@ -377,7 +382,7 @@ void tlb_flush_pgtable(struct mmu_gather *tlb, unsigned long address)
{
int tsize = mmu_psize_defs[mmu_pte_psize].enc;
- if (book3e_htw_enabled) {
+ if (book3e_htw_mode) {
Make it if (boot3e_htw_enabled != PPC_HTW_NONE)
Seems a little verbose, but OK.
Same with things like this, I guess:
book3e_htw_mode ? "enabled" : "not supported"
Well, it's no longer a boolean so ...
It's pretty common to use implicit boolean conversion when a zero value
means no/false/absent, even if there are multiple non-false
possibilities (e.g. pointers) -- but not a big deal to change it if you
prefer.
BTW. On another note, can you pickup Ananth series for larger address
space (minus the one patch that breaks the BookE build, it shouldn't
matter) and see if there's any runtime issue on BookE 64 ? (And whether
the larger address space actually works for you too, using something
like high up mmap tests)
It booted OK for me in my initial testing with a ramdisk, but when I
tried to use the network (to load a high mmap test program) I got hangs
that didn't happen before that patchset. I'll look into it more tomorrow.
-Scott