On Mon, Oct 23, 2017 at 02:17:39PM +0530, Aneesh Kumar K.V wrote:
> Michael Ellerman <m...@ellerman.id.au> writes:
>
> > Ram Pai <linux...@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
