On Wed, Oct 25, 2017 at 11:18:49AM +0200, Michael Ellerman wrote:

Ram Pai [off-list ref] writes:

quoted

On Mon, Oct 23, 2017 at 02:17:39PM +0530, Aneesh Kumar K.V wrote:

quoted

Michael Ellerman [off-list ref] writes:

quoted

Ram Pai [off-list ref] writes:

...

quoted

quoted

quoted

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.

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

Right.

quoted

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.

This code is already confusing to *any* reader, so I don't think it's a
worry. :)

I just got it coded and working.  But I see no advantage implementing
the shifted-value. The hidx in the secondary-part of the pte, still
needs to be initialzed to all-zeros. Because it could contain the value of
the hidx corresponding the 64k-backed hpte, which needs to be cleared.

I will send the patch anyway.  But we should not apply it
for I see no apparent gain.

RP

cheers

-- 
Ram Pai