linux-kernel - Re: [PATCH 1/2 v3] mm: vmscan: do not pass reclaimed slab to vmpressure

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:   Thu, 2 Feb 2017 11:44:22 +0100
From:   Michal Hocko <mhocko@...nel.org>
To:     Vinayak Menon <vinmenon@...eaurora.org>
Cc:     akpm@...ux-foundation.org, hannes@...xchg.org,
        mgorman@...hsingularity.net, vbabka@...e.cz, riel@...hat.com,
        vdavydov.dev@...il.com, anton.vorontsov@...aro.org,
        minchan@...nel.org, shashim@...eaurora.org, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2 v3] mm: vmscan: do not pass reclaimed slab to
 vmpressure

On Tue 31-01-17 14:32:08, Vinayak Menon wrote:
> During global reclaim, the nr_reclaimed passed to vmpressure
> includes the pages reclaimed from slab. But the corresponding
> scanned slab pages is not passed. This can cause total reclaimed
> pages to be greater than scanned, causing an unsigned underflow
> in vmpressure resulting in a critical event being sent to root
> cgroup. So do not consider reclaimed slab pages for vmpressure
> calculation. The reclaimed pages from slab can be excluded because
> the freeing of a page by slab shrinking depends on each slab's
> object population, making the cost model (i.e. scan:free) different
> from that of LRU.

This might be true but what happens if the slab reclaim contributes
significantly to the overal reclaim? This would be quite rare but not
impossible.

I am wondering why we cannot simply make cap nr_reclaimed to nr_scanned
and be done with this all? Sure it will be imprecise but the same will
be true with this approach.

> Also, not every shrinker accounts the pages it
> reclaims. This is a regression introduced by commit 6b4f7799c6a5
> ("mm: vmscan: invoke slab shrinkers from shrink_zone()").

We usually refer to the culprit comment as
Fixes: 6b4f7799c6a5 ("mm: vmscan: invoke slab shrinkers from shrink_zone()")
 
> Signed-off-by: Vinayak Menon <vinmenon@...eaurora.org>
> ---
>  mm/vmscan.c | 17 ++++++++++++-----
>  1 file changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index 947ab6f..8969f8e 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -2594,16 +2594,23 @@ static bool shrink_node(pg_data_t *pgdat, struct scan_control *sc)
>  				    sc->nr_scanned - nr_scanned,
>  				    node_lru_pages);
>  
> +		/*
> +		 * Record the subtree's reclaim efficiency. The reclaimed
> +		 * pages from slab is excluded here because the corresponding
> +		 * scanned pages is not accounted. Moreover, freeing a page
> +		 * by slab shrinking depends on each slab's object population,
> +		 * making the cost model (i.e. scan:free) different from that
> +		 * of LRU.
> +		 */
> +		vmpressure(sc->gfp_mask, sc->target_mem_cgroup, true,
> +			   sc->nr_scanned - nr_scanned,
> +			   sc->nr_reclaimed - nr_reclaimed);
> +
>  		if (reclaim_state) {
>  			sc->nr_reclaimed += reclaim_state->reclaimed_slab;
>  			reclaim_state->reclaimed_slab = 0;
>  		}
>  
> -		/* Record the subtree's reclaim efficiency */
> -		vmpressure(sc->gfp_mask, sc->target_mem_cgroup, true,
> -			   sc->nr_scanned - nr_scanned,
> -			   sc->nr_reclaimed - nr_reclaimed);
> -
>  		if (sc->nr_reclaimed - nr_reclaimed)
>  			reclaimable = true;
>  
> -- 
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a
> member of the Code Aurora Forum, hosted by The Linux Foundation
> 

-- 
Michal Hocko
SUSE Labs