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

["Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>]

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?

-aneesh