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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 20 Jun 2018 12:57:48 +0200
From:   Vlastimil Babka <vbabka@...e.cz>
To:     Yang Shi <yang.shi@...ux.alibaba.com>, hughd@...gle.com,
        kirill.shutemov@...ux.intel.com, akpm@...ux-foundation.org,
        Andrew Morton <akpm@...ux-foundation.org>
Cc:     linux-mm@...ck.org, linux-kernel@...r.kernel.org,
        Jerome Marchand <jmarchan@...hat.com>
Subject: Re: [PATCH] thp: use mm_file_counter to determine update which rss
 counter

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.

> Signed-off-by: Yang Shi <yang.shi@...ux.alibaba.com>
> Cc: Hugh Dickins <hughd@...gle.com>
> Cc: Kirill A. Shutemov <kirill.shutemov@...ux.intel.com>

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@...e.cz>

> ---
>  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
>