linux-kernel - Re: [PATCH] mm: workingset: clarify eviction order and distance calculation

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:   Mon, 1 Feb 2021 14:02:12 +0100
From:   Vlastimil Babka <vbabka@...e.cz>
To:     Oscar Salvador <osalvador@...e.de>, akpm@...ux-foundation.org
Cc:     hannes@...xchg.org, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] mm: workingset: clarify eviction order and distance
 calculation

On 2/1/21 7:06 AM, Oscar Salvador wrote:
> The premise of the refault distance is that it can be seen as a deficit
> of the inactive list space, so that if the inactive list would have had
> (R - E) more slots, the page would not have been evicted but promoted
> to the active list instead.
> 
> However, the way the code is ordered right now set us to be off by one,
> so the real number of slots would be (R - E) + 1.
> I stumbled upon this when trying to understand the code and it puzzled me
> that the comments did not match what the code did.
> 
> This it not an issue at all since evictions and refaults tend to happen
> in a number large enough that being off-by-one does not have any impact
> - and since the compiler and CPUs are free to rearrange the execution
> sequence anyway.
> But as Johannes says, it is better to re-arrange the code in the proper
> order since otherwise would be misleading to somebody who is actively
> reading and trying to understand the logic of the code - like it
> happened to me.
> 
> Signed-off-by: Oscar Salvador <osalvador@...e.de>
> Acked-by: Johannes Weiner <hannes@...xchg.org>

Acked-by: Vlastimil Babka <vbabka@...e.cz>

> ---
>  mm/workingset.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mm/workingset.c b/mm/workingset.c
> index 10e96de945b3..0201aa1ff320 100644
> --- a/mm/workingset.c
> +++ b/mm/workingset.c
> @@ -263,10 +263,10 @@ void *workingset_eviction(struct page *page, struct mem_cgroup *target_memcg)
>  	VM_BUG_ON_PAGE(!PageLocked(page), page);
>  
>  	lruvec = mem_cgroup_lruvec(target_memcg, pgdat);
> -	workingset_age_nonresident(lruvec, thp_nr_pages(page));
>  	/* XXX: target_memcg can be NULL, go through lruvec */
>  	memcgid = mem_cgroup_id(lruvec_memcg(lruvec));
>  	eviction = atomic_long_read(&lruvec->nonresident_age);
> +	workingset_age_nonresident(lruvec, thp_nr_pages(page));
>  	return pack_shadow(memcgid, pgdat, eviction, PageWorkingset(page));
>  }
>  
>