linuxppc-dev.lists.ozlabs.org archive mirror
 help / color / mirror / Atom feed
From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
To: Hugh Dickins <hughd@google.com>,
	"Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>
Cc: paulus@samba.org, mpe@ellerman.id.au, linuxppc-dev@lists.ozlabs.org
Subject: Re: [PATCH] powerpc/mm/hash: Fix the reference bit update when handling hash fault
Date: Tue, 31 May 2016 08:27:42 +1000	[thread overview]
Message-ID: <1464647262.3078.219.camel@kernel.crashing.org> (raw)
In-Reply-To: <alpine.LSU.2.11.1605300903270.1076@eggly.anvils>

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.

Ben.

  reply	other threads:[~2016-05-30 22:28 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <201605270629.u4R6OCLx014831@mx0a-001b2d01.pphosted.com>
2016-05-30 16:39 ` [PATCH] powerpc/mm/hash: Fix the reference bit update when handling hash fault Hugh Dickins
2016-05-30 22:27   ` Benjamin Herrenschmidt [this message]
2016-05-31 16:28     ` Hugh Dickins
2016-06-02 15:12       ` Hugh Dickins
2016-06-07 12:07         ` Michael Ellerman
2016-05-27  6:26 Aneesh Kumar K.V

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1464647262.3078.219.camel@kernel.crashing.org \
    --to=benh@kernel.crashing.org \
    --cc=aneesh.kumar@linux.vnet.ibm.com \
    --cc=hughd@google.com \
    --cc=linuxppc-dev@lists.ozlabs.org \
    --cc=mpe@ellerman.id.au \
    --cc=paulus@samba.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).