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

[Vlastimil Babka <vbabka@suse.cz>]

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@suse.de>
> Acked-by: Johannes Weiner <hannes@cmpxchg.org>

Acked-by: Vlastimil Babka <vbabka@suse.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));
>  }
>  
>