Re: [PATCHv5 01/28] mm, proc: adjust PSS calculation

[Vlastimil Babka]

On 04/23/2015 11:03 PM, Kirill A. Shutemov wrote:
> With new refcounting all subpages of the compound page are not nessessary
> have the same mapcount. We need to take into account mapcount of every
> sub-page.
>
> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@xxxxxxxxxxxxxxx>
> Tested-by: Sasha Levin <sasha.levin@xxxxxxxxxx>

Acked-by: Vlastimil Babka <vbabka@xxxxxxx>

(some nitpicks below)

> ---
>   fs/proc/task_mmu.c | 43 ++++++++++++++++++++++---------------------
>   1 file changed, 22 insertions(+), 21 deletions(-)
>
> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> index 956b75d61809..95bc384ee3f7 100644
> --- a/fs/proc/task_mmu.c
> +++ b/fs/proc/task_mmu.c
> @@ -449,9 +449,10 @@ struct mem_size_stats {
>   };
>
>   static void smaps_account(struct mem_size_stats *mss, struct page *page,
> -		unsigned long size, bool young, bool dirty)
> +		bool compound, bool young, bool dirty)
>   {
> -	int mapcount;
> +	int i, nr = compound ? hpage_nr_pages(page) : 1;

Why not just HPAGE_PMD_NR instead of hpage_nr_pages(page)? We already 
came here through a pmd mapping. Even if the page stopped being a 
hugepage meanwhile (I'm not sure if any locking prevents that or not?), 
it would be more accurate to continue assuming it's a hugepage, 
otherwise we account only the base page (formerly head) and skip the 511 
formerly tail pages?

Also, is there some shortcut way to tell us that we are the only one 
mapping the whole compound page, and nobody has any base pages, so we 
don't need to loop on each tail page? I guess not under the new design, 
right...

> +	unsigned long size = nr * PAGE_SIZE;
>
>   	if (PageAnon(page))
>   		mss->anonymous += size;
> @@ -460,23 +461,23 @@ static void smaps_account(struct mem_size_stats *mss, struct page *page,
>   	/* Accumulate the size in pages that have been accessed. */
>   	if (young || PageReferenced(page))
>   		mss->referenced += size;
> -	mapcount = page_mapcount(page);
> -	if (mapcount >= 2) {
> -		u64 pss_delta;
>
> -		if (dirty || PageDirty(page))
> -			mss->shared_dirty += size;
> -		else
> -			mss->shared_clean += size;
> -		pss_delta = (u64)size << PSS_SHIFT;
> -		do_div(pss_delta, mapcount);
> -		mss->pss += pss_delta;
> -	} else {
> -		if (dirty || PageDirty(page))
> -			mss->private_dirty += size;
> -		else
> -			mss->private_clean += size;
> -		mss->pss += (u64)size << PSS_SHIFT;
> +	for (i = 0; i < nr; i++) {
> +		int mapcount = page_mapcount(page + i);
> +
> +		if (mapcount >= 2) {
> +			if (dirty || PageDirty(page + i))
> +				mss->shared_dirty += PAGE_SIZE;
> +			else
> +				mss->shared_clean += PAGE_SIZE;
> +			mss->pss += (PAGE_SIZE << PSS_SHIFT) / mapcount;
> +		} else {
> +			if (dirty || PageDirty(page + i))
> +				mss->private_dirty += PAGE_SIZE;
> +			else
> +				mss->private_clean += PAGE_SIZE;
> +			mss->pss += PAGE_SIZE << PSS_SHIFT;
> +		}

That's 3 instances of "page + i", why not just use page and do a page++ 
in the for loop?

>   	}
>   }
>
> @@ -500,7 +501,8 @@ static void smaps_pte_entry(pte_t *pte, unsigned long addr,
>
>   	if (!page)
>   		return;
> -	smaps_account(mss, page, PAGE_SIZE, pte_young(*pte), pte_dirty(*pte));
> +
> +	smaps_account(mss, page, false, pte_young(*pte), pte_dirty(*pte));
>   }
>
>   #ifdef CONFIG_TRANSPARENT_HUGEPAGE
> @@ -516,8 +518,7 @@ static void smaps_pmd_entry(pmd_t *pmd, unsigned long addr,
>   	if (IS_ERR_OR_NULL(page))
>   		return;
>   	mss->anonymous_thp += HPAGE_PMD_SIZE;
> -	smaps_account(mss, page, HPAGE_PMD_SIZE,
> -			pmd_young(*pmd), pmd_dirty(*pmd));
> +	smaps_account(mss, page, true, pmd_young(*pmd), pmd_dirty(*pmd));
>   }
>   #else
>   static void smaps_pmd_entry(pmd_t *pmd, unsigned long addr,

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/