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

[Ram Pai <linuxram@us.ibm.com>]

On Mon, Oct 23, 2017 at 02:17:39PM +0530, Aneesh Kumar K.V wrote:
> Michael Ellerman <mpe@ellerman.id.au> writes:
> 
> > Ram Pai <linuxram@us.ibm.com> writes:
> >
> >> 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:
> >   
> >> -		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 ...".
> >
> >>  		 * 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.
> > 	 */
> >
> >> +	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.
> 
> That would be imply, we loose one slot in primary group, which means we
> will do extra work in some case because our primary now has only 7
> slots. And in case of pseries, the hypervisor will always return the
> least available slot, which implie we will do extra hcalls in case of an
> hpte insert to an empty group?


No. that is not the idea.  The idea is that slot 'F' in the seconday
will continue to be a invalid slot, but will be represented as
offset-by-one in the PTE.  In other words, 0 will be repesented as 1,
1 as 2....   and  n as (n+1)%32

The idea seems feasible.  It has the advantage -- where 0 in the PTE
means invalid slot. But it can be confusing to the casual code-
reader. Will need to put in a big-huge comment to explain that.

RP