Re: [PATCH] thp: use mm_file_counter to determine update which rss counter

[Yang Shi]

On 6/20/18 3:57 AM, Vlastimil Babka wrote:
> On 06/19/2018 11:08 PM, Yang Shi wrote:
> > Since commit eca56ff906bdd0239485e8b47154a6e73dd9a2f3 ("mm, shmem: add
> > internal shmem resident memory accounting"), MM_SHMEMPAGES is added to
> > separate the shmem accounting from regular files. So, all shmem pages
> > should be accounted to MM_SHMEMPAGES instead of MM_FILEPAGES.
> >
> > And, normal 4K shmem pages have been accounted to MM_SHMEMPAGES, so
> > shmem thp pages should be not treated differently. Accouting them to
> > MM_SHMEMPAGES via mm_counter_file() since shmem pages are swap backed
> > to keep consistent with normal 4K shmem pages.
> >
> > This will not change the rss counter of processes since shmem pages are
> > still a part of it.
> <Andrew>So what are the user-visible effects of the patch then?</Andrew>
>
> Let's add this?:
>
> The /proc/pid/status and /proc/pid/statm counters will however be more
> accurate wrt shmem usage, as originally intended.

Thanks. Yes, other than this as commit 
eca56ff906bdd0239485e8b47154a6e73dd9a2f3 mentioned, oom also could 
report more accurate "shmem-rss"

> > Signed-off-by: Yang Shi <yang.shi@xxxxxxxxxxxxxxxxx>
> > Cc: Hugh Dickins <hughd@xxxxxxxxxx>
> > Cc: Kirill A. Shutemov <kirill.shutemov@xxxxxxxxxxxxxxx>
> Good catch, thanks. I guess the discrepancy happened due to the
> thp-tmpfs patchset existing externally for a long time and number of
> iterations, while commit eca56ff906bdd was merged, and nobody noticed
> that the thp case added more MM_FILEPAGES users.
>
> Acked-by: Vlastimil Babka <vbabka@xxxxxxx>

Thanks.

Yang

> > ---
> >   mm/huge_memory.c | 4 ++--
> >   mm/memory.c      | 2 +-
> >   2 files changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> > index 1cd7c1a..2687f7c 100644
> > --- a/mm/huge_memory.c
> > +++ b/mm/huge_memory.c
> > @@ -1740,7 +1740,7 @@ int zap_huge_pmd(struct mmu_gather *tlb, struct vm_area_struct *vma,
> >   		} else {
> >   			if (arch_needs_pgtable_deposit())
> >   				zap_deposited_table(tlb->mm, pmd);
> > -			add_mm_counter(tlb->mm, MM_FILEPAGES, -HPAGE_PMD_NR);
> > +			add_mm_counter(tlb->mm, mm_counter_file(page), -HPAGE_PMD_NR);
> >   		}
> >   
> >   		spin_unlock(ptl);
> > @@ -2088,7 +2088,7 @@ static void __split_huge_pmd_locked(struct vm_area_struct *vma, pmd_t *pmd,
> >   			SetPageReferenced(page);
> >   		page_remove_rmap(page, true);
> >   		put_page(page);
> > -		add_mm_counter(mm, MM_FILEPAGES, -HPAGE_PMD_NR);
> > +		add_mm_counter(mm, mm_counter_file(page), -HPAGE_PMD_NR);
> >   		return;
> >   	} else if (is_huge_zero_pmd(*pmd)) {
> >   		/*
> > diff --git a/mm/memory.c b/mm/memory.c
> > index 7206a63..4a2f2a8 100644
> > --- a/mm/memory.c
> > +++ b/mm/memory.c
> > @@ -3372,7 +3372,7 @@ static int do_set_pmd(struct vm_fault *vmf, struct page *page)
> >   	if (write)
> >   		entry = maybe_pmd_mkwrite(pmd_mkdirty(entry), vma);
> >   
> > -	add_mm_counter(vma->vm_mm, MM_FILEPAGES, HPAGE_PMD_NR);
> > +	add_mm_counter(vma->vm_mm, mm_counter_file(page), HPAGE_PMD_NR);
> >   	page_add_file_rmap(page, true);
> >   	/*
> >   	 * deposit and withdraw with pmd lock held