Re: [PATCH 3/7] powerpc: Free up four 64K PTE bits in 4K backed HPTE pages

On Thu, Oct 19, 2017 at 02:25:47PM +1100, Michael Ellerman wrote:

Ram Pai [off-list ref] writes:

quoted

diff --git a/arch/powerpc/mm/hash64_64k.c b/arch/powerpc/mm/hash64_64k.c
index 1a68cb1..c6c5559 100644
--- a/arch/powerpc/mm/hash64_64k.c
+++ b/arch/powerpc/mm/hash64_64k.c

@@ -126,18 +113,13 @@ int __hash_page_4K(unsigned long ea, unsigned long access, unsigned long vsid,
 	if (__rpte_sub_valid(rpte, subpg_index)) {
 		int ret;
 
-		hash = hpt_hash(vpn, shift, ssize);
-		hidx = __rpte_to_hidx(rpte, subpg_index);
-		if (hidx & _PTEIDX_SECONDARY)
-			hash = ~hash;
-		slot = (hash & htab_hash_mask) * HPTES_PER_GROUP;
-		slot += hidx & _PTEIDX_GROUP_IX;
+		gslot = pte_get_hash_gslot(vpn, shift, ssize, rpte,
+					subpg_index);
+		ret = mmu_hash_ops.hpte_updatepp(gslot, rflags, vpn,
+			MMU_PAGE_4K, MMU_PAGE_4K, ssize, flags);

This was formatted correctly before:
  

quoted

-		ret = mmu_hash_ops.hpte_updatepp(slot, rflags, vpn,
-						 MMU_PAGE_4K, MMU_PAGE_4K,
-						 ssize, flags);
 		/*
-		 *if we failed because typically the HPTE wasn't really here
+		 * if we failed because typically the HPTE wasn't really here

If you're fixing it up please make it "If ...".

quoted

 		 * we try an insertion.
 		 */
 		if (ret == -1)

@@ -148,6 +130,15 @@ int __hash_page_4K(unsigned long ea, unsigned long access, unsigned long vsid,
 	}
 
 htab_insert_hpte:
+
+	/*
+	 * initialize all hidx entries to invalid value,
+	 * the first time the PTE is about to allocate
+	 * a 4K hpte
+	 */

Should be:
	/*
	 * Initialize all hidx entries to invalid value, the first time
         * the PTE is about to allocate a 4K HPTE.
	 */

quoted

+	if (!(old_pte & H_PAGE_COMBO))
+		rpte.hidx = ~0x0UL;
+

Paul had the idea that if we biased the slot number by 1, we could make
the "invalid" value be == 0.

That would avoid needing to that above, and also mean the value is
correctly invalid from the get-go, which would be good IMO.

I think now that you've added the slot accessors it would be pretty easy
to do.

I did attempt to do so, and was not getting it right. The machine went
unstable. So left it with an accessor, to be revisited at a
later point in time. That time has come... I suppose.  Shall I make it a
separate patch instead of baking it into this patch?

RP