Re: [PATCH v3 5/7] mm: Remember young/dirty bit for page migrations

[Peter Xu]

On Wed, Aug 10, 2022 at 02:30:33PM +0800, Huang, Ying wrote:
> Peter Xu <peterx@xxxxxxxxxx> writes:
> 
> > When page migration happens, we always ignore the young/dirty bit settings
> > in the old pgtable, and marking the page as old in the new page table using
> > either pte_mkold() or pmd_mkold(), and keeping the pte clean.
> >
> > That's fine from functional-wise, but that's not friendly to page reclaim
> > because the moving page can be actively accessed within the procedure.  Not
> > to mention hardware setting the young bit can bring quite some overhead on
> > some systems, e.g. x86_64 needs a few hundreds nanoseconds to set the bit.
> > The same slowdown problem to dirty bits when the memory is first written
> > after page migration happened.
> >
> > Actually we can easily remember the A/D bit configuration and recover the
> > information after the page is migrated.  To achieve it, define a new set of
> > bits in the migration swap offset field to cache the A/D bits for old pte.
> > Then when removing/recovering the migration entry, we can recover the A/D
> > bits even if the page changed.
> >
> > One thing to mention is that here we used max_swapfile_size() to detect how
> > many swp offset bits we have, and we'll only enable this feature if we know
> > the swp offset can be big enough to store both the PFN value and the young
>                                                                        ~~~~~
> Nitpick: A/D

Fixed.

> 
> > bit.  Otherwise the A/D bits are dropped like before.
> >
> > Signed-off-by: Peter Xu <peterx@xxxxxxxxxx>
> > ---
> >  include/linux/swapops.h | 99 +++++++++++++++++++++++++++++++++++++++++
> >  mm/huge_memory.c        | 18 +++++++-
> >  mm/migrate.c            |  6 ++-
> >  mm/migrate_device.c     |  4 ++
> >  mm/rmap.c               |  5 ++-
> >  5 files changed, 128 insertions(+), 4 deletions(-)
> >
> > diff --git a/include/linux/swapops.h b/include/linux/swapops.h
> > index e1accbcd1136..0e9579b90659 100644
> > --- a/include/linux/swapops.h
> > +++ b/include/linux/swapops.h
> > @@ -8,6 +8,10 @@
> >  
> >  #ifdef CONFIG_MMU
> >  
> > +#ifdef CONFIG_SWAP
> > +#include <linux/swapfile.h>
> > +#endif	/* CONFIG_SWAP */
> 
> I don't think we need the comment here.  The #ifdef is too near.  But
> this isn't a big deal.

I'd slightly prefer keeping it (especially Nadav used to complain on
missing comments on ifdefs in previous versions..) since any ifdef can grow
by adding code into it. Then it'll be hard to justify how to define "near"
or not, so hard to define who should be adding that if I'm not the one.

Thanks,

-- 
Peter Xu