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

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

[Nicholas Piggin]

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.

Thanks,
Nick

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

[Huang Pei]

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