Re: [PATCH v3 3/3] mm: optimise pte dirty/accessed bit setting by demand based pte insertion

From: Huang Pei <huangpei@loongson.cn>
To: Nicholas Piggin <npiggin@gmail.com>
Cc: Hugh Dickins <hughd@google.com>,
	linux-mm@kvack.org, Bibo Mao <maobibo@loongson.cn>,
	Linus Torvalds <torvalds@linux-foundation.org>,
	linux-arch@vger.kernel.org, linux-mips@vger.kernel.org
Subject: Re: [PATCH v3 3/3] mm: optimise pte dirty/accessed bit setting by demand based pte insertion
Date: Wed, 23 Dec 2020 08:56:52 +0800	[thread overview]
Message-ID: <20201223005651.bviywoihdzzlm3z6@ambrosehua-HP-xw6600-Workstation> (raw)
In-Reply-To: <1608606460.clzumasfvm.astroid@bobo.none>

Hi, 
On Tue, Dec 22, 2020 at 01:24:53PM +1000, Nicholas Piggin wrote:
> Excerpts from Hugh Dickins's message of December 22, 2020 4:21 am:
> > Hi Nick,
> > 
> > On Sun, 20 Dec 2020, Nicholas Piggin wrote:
> > 
> >> Similarly to the previous patch, this tries to optimise dirty/accessed
> >> bits in ptes to avoid access costs of hardware setting them.
> >> 
> >> This tidies up a few last cases where dirty/accessed faults can be seen,
> >> and subsumes the pte_sw_mkyoung helper -- it's not just architectures
> >> with explicit software dirty/accessed bits that take expensive faults to
> >> modify ptes.
> >> 
> >> The vast majority of the remaining dirty/accessed faults on kbuild
> >> workloads after this patch are from NUMA migration, due to
> >> remove_migration_pte inserting old/clean ptes.
> > 
> > Are you sure about this patch? It looks wrong to me: because isn't
> > _PAGE_ACCESSED (young) already included in the vm_page_prot __S001 etc?
> > 
> > I haven't checked all instances below, but in general, that's why the
> > existing code tends not to bother to mkyoung, but does sometimes mkold
> > (admittedly confusing).
> 
> There might have been one or two cases where it didn't come directly
> from vm_page_prot, but it was a few rebases and updates ago. I did see
> one or two places where powerpc was taking faults. Good point though I 
> can test again and see, and I might split the patch.
> 
> > 
> > A quick check on x86 and powerpc looks like they do have _PAGE_ACCESSED
> > in there; and IIRC that's true, or ought to be true, of all architectures.
> > 
> > Maybe not mips, which I see you have singled out: I remember going round
> > this loop a few months ago with mips, where strange changes were being
> > proposed to compensate for not having that bit in their vm_page_prot.
> > I didn't follow through to see how that ended up, but I did suggest
> > mips needed to follow the same convention as the other architectures.
> 
> Yeah the main thing is to try get all architectures doing the same thing
> and get rid of that sw_mkyoung too. Given the (Intel) x86 result of the
> heavy micro-fault, I don't think anybody is special and we should 
> require them to follow the same behaviour unless it's proven that one
> needs something different.
> 
> If we can get all arch to set accessed in vm_page_prot and get rid of 
> most of these mkyoung()s then all the better. And it actually looks like
> MIPS may be changing direction:
> 
> https://lore.kernel.org/linux-arch/20200919074731.22372-1-huangpei@loongson.cn/
> 
> What's the chances of that going upstream for the next merge window? It
> seems like the right thing to do.

Any comment on V4:

https://lore.kernel.org/linux-arch/20201019081257.32127-1-huangpei@loongson.cn/

> 
> Thanks,
> Nick

Thanks,
Huang Pei