Re: [PATCH v34 05/13] mm/damon: Implement primitives for the virtual memory address spaces

[SeongJae Park <sj38.park@gmail.com>]

From: SeongJae Park <sjpark@amazon.de>

Hello David,


On Thu, 26 Aug 2021 16:09:23 +0200 David Hildenbrand <david@redhat.com> wrote:

> > +static void damon_va_mkold(struct mm_struct *mm, unsigned long addr)
> > +{
> > +	pte_t *pte = NULL;
> > +	pmd_t *pmd = NULL;
> > +	spinlock_t *ptl;
> > +
> 
> I just stumbled over this, sorry for the dumb questions:

Appreciate for the great questions!

> 
> 
> a) What do we know about that region we are messing with?
> 
> AFAIU, just like follow_pte() and follow_pfn(), follow_invalidate_pte() 
> should only be called on VM_IO and raw VM_PFNMAP mappings in general 
> (see the doc of follow_pte()). Do you even know that it's within a 
> single VMA and that there are no concurrent modifications?

We have no idea about the region at this moment.  However, if we successfully
get the pte or pmd under the protection of the page table lock, we ensure the
page for the pte or pmd is a online LRU-page with damon_get_page(), before
updating the pte or pmd's PAGE_ACCESSED bit.  We release the page table lock
only after the update.

And concurrent VMA change doesn't matter here because we read and write only
the page table.  If the address is not mapped or not backed by LRU pages, we
simply treat it as not accessed.

> 
> b) Which locks are we holding?
> 
> I hope we're holding the mmap lock in read mode at least. Or how are you 
> making sure there are no concurrent modifications to page tables / VMA 
> layout ... ?
> 
> > +	if (follow_invalidate_pte(mm, addr, NULL, &pte, &pmd, &ptl))

All the operations are protected by the page table lock of the pte or pmd, so
no concurrent page table modification would happen.  As previously mentioned,
because we read and update only page table, we don't care about VMAs and
therefore we don't need to hold mmap lock here.

Outside of this function, DAMON reads the VMAs to know which address ranges are
not mapped, and avoid inefficiently checking access to the area with the
information.  Nevertheless, it happens only occasionally (once per 60 seconds
by default), and it holds the mmap read lock in the case.

Nonetheless, I agree the usage of follow_invalidate_pte() here could make
readers very confusing.  It would be better to implement and use DAMON's own
page table walk logic.  Of course, I might missing something important.  If you
think so, please don't hesitate at yelling to me.


Thanks,
SJ

> 
> 
> 
> -- 
> Thanks,
> 
> David / dhildenb