Re: [PATCH] powerpc/mm/hash: Fix the reference bit update when handling hash fault

[Hugh Dickins <hughd@google.com>]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2716 bytes --]

On Tue, 31 May 2016, Benjamin Herrenschmidt wrote:
> On Mon, 2016-05-30 at 09:39 -0700, Hugh Dickins wrote:
> > I don't mean to be churlish, and subtract from your triumph in tracking
> > this down (assuming you have), but that commit log... okay, it's intended
> > for powerpc mmu experts, not me, but if it hasn't already gone into git,
> > then a rewrite could be very helpful.
> 
> Something along these lines:
> 
> The powerpc hash table has a R (Referenced) and C (Changed) bits that
> somewhat correspond to Linux _PAGE_DIRTY and _PAGE_ACCESSED. However we
> don't currently use them.
> 
> Moreover, we also require them to never be updated by HW. This is due
> to an optimization we have in the hash eviction code, which would be
> racy vs. a hardware update as the HW updates are done non-atomically.
> 
> Thus it's critical that valid hash PTEs always have R set and writeable
> ones have C set. We do this by hashing a non-dirty linux PTE as read-only and always setting _PAGE_ACCESSED (and thus R) when hashing anything else in. Any attempt by Linux at clearing those bits also removes the corresponding hash entry.
> 
> The old commit <.....> fixed an issue where we would miss setting C in
> the specific case where a Linux PTE was upgraded from read only to
> read-write (and appropriately made dirty). The hash code would realize
> the hash PTE is already present and would use a different path than the
> normal insertion path for updating a hash entry in-place. That path
> unfortunately didn't update "C".
> 
> That commit however got a bit over zealous and also forced C on any
> entry including those that aren't writeable. That was unnecessary.
> 
> In commit 89ff725051d1, when converting to C, we mangled that up:
> 
>  - We kept the useless part of <....> setting C always instead of
> only when _PAGE_DIRTY is set
> 
>  - We never set R thus letting the HW do the racy updates.
> 
> This fixes it.

Thanks, that helped me to understand a lot better - and the original
commitlog then made much more sense to me after this version.

Plus I can now see that it is inherently a very confusing situation,
made the more confusing by the series of not-quite-right commits,
so all the more difficult to explain to an outsider.

I still won't pretend to understand it fully, but don't mind that:
my main concern is that if the commitlog is confused, then that
might hint that the code is still not quite right.

But all my evidence so far is that it is now right: I'll continue
testing v4.6+fix on a couple of loads until this evening: all is
well so far.  And then switch to testing v4.5+fix on those loads
for another day and a half.

Hugh