Re: [PATCH 1/2] mm: hugetlb: proc: check for hugetlb shared PMD in /proc/PID/smaps

[Michal Hocko]

On Fri 27-01-23 17:12:05, Mike Kravetz wrote:
> On 01/27/23 15:04, Andrew Morton wrote:
> > On Fri, 27 Jan 2023 17:23:39 +0100 David Hildenbrand <david@xxxxxxxxxx> wrote:
> > 
> > > On 26.01.23 23:27, Mike Kravetz wrote:
> > > > A hugetlb page will have a mapcount of 1 if mapped by multiple processes
> > > > via a shared PMD.  This is because only the first process increases the
> > > > map count, and subsequent processes just add the shared PMD page to
> > > > their page table.
> > > > 
> > > > page_mapcount is being used to decide if a hugetlb page is shared or
> > > > private in /proc/PID/smaps.  Pages referenced via a shared PMD were
> > > > incorrectly being counted as private.
> > > > 
> > > > To fix, check for a shared PMD if mapcount is 1.  If a shared PMD is
> > > > found count the hugetlb page as shared.  A new helper to check for a
> > > > shared PMD is added.
> > > > 
> > > ...
> > >
> > > > --- a/fs/proc/task_mmu.c
> > > > +++ b/fs/proc/task_mmu.c
> > > > @@ -749,8 +749,14 @@ static int smaps_hugetlb_range(pte_t *pte, unsigned long hmask,
> > > >   
> > > >   		if (mapcount >= 2)
> > > >   			mss->shared_hugetlb += huge_page_size(hstate_vma(vma));
> > > > -		else
> > > > -			mss->private_hugetlb += huge_page_size(hstate_vma(vma));
> > > > +		else {
> > > 
> > > Better:
> > > 
> > > if (mapcount >= 2 || hugetlb_pmd_shared(pte))
> > > 	mss->shared_hugetlb += huge_page_size(hstate_vma(vma));
> > > else
> > > 	mss->private_hugetlb += huge_page_size(hstate_vma(vma));
> > 
> > Yup.  And that local doesn't add any value?
> > 
> > --- a/fs/proc/task_mmu.c~mm-hugetlb-proc-check-for-hugetlb-shared-pmd-in-proc-pid-smaps-fix
> > +++ a/fs/proc/task_mmu.c
> > @@ -745,18 +745,10 @@ static int smaps_hugetlb_range(pte_t *pt
> >  			page = pfn_swap_entry_to_page(swpent);
> >  	}
> >  	if (page) {
> > -		int mapcount = page_mapcount(page);
> > -
> > -		if (mapcount >= 2)
> > +		if (page_mapcount(page) >= 2 || hugetlb_pmd_shared(pte))
> >  			mss->shared_hugetlb += huge_page_size(hstate_vma(vma));
> > -		else {
> > -			if (hugetlb_pmd_shared(pte))
> > -				mss->shared_hugetlb +=
> > -						huge_page_size(hstate_vma(vma));
> > -			else
> > -				mss->private_hugetlb +=
> > -						huge_page_size(hstate_vma(vma));
> > -		}
> > +		else
> > +			mss->private_hugetlb += huge_page_size(hstate_vma(vma));
> >  	}
> >  	return 0;
> >  }
> 
> Thank you both!  That looks much better.

Yes, this looks simple enough. My only concern would be that this
special casing might be required on other places which is hard to
evaluate. I thought PSS reported by smaps would be broken as well but it
seems pss is not really accounted for hugetlb mappings at all.

Have you tried to look into {in,de}creasing the map count of the page when
the the pte is {un}shared for it?
-- 
Michal Hocko
SUSE Labs