linux-mm.kvack.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] mm: workingset: clarify eviction order and distance calculation
@ 2021-02-01  6:06 Oscar Salvador
  2021-02-01 13:02 ` Vlastimil Babka
  2021-02-11 21:26 ` Oscar Salvador
  0 siblings, 2 replies; 4+ messages in thread
From: Oscar Salvador @ 2021-02-01  6:06 UTC (permalink / raw)
  To: akpm; +Cc: hannes, linux-mm, linux-kernel, Oscar Salvador

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>
---
 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));
 }
 
-- 
2.26.2



^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] mm: workingset: clarify eviction order and distance calculation
  2021-02-01  6:06 [PATCH] mm: workingset: clarify eviction order and distance calculation Oscar Salvador
@ 2021-02-01 13:02 ` Vlastimil Babka
  2021-02-11 21:26 ` Oscar Salvador
  1 sibling, 0 replies; 4+ messages in thread
From: Vlastimil Babka @ 2021-02-01 13:02 UTC (permalink / raw)
  To: Oscar Salvador, akpm; +Cc: hannes, linux-mm, linux-kernel

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));
>  }
>  
> 



^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] mm: workingset: clarify eviction order and distance calculation
  2021-02-01  6:06 [PATCH] mm: workingset: clarify eviction order and distance calculation Oscar Salvador
  2021-02-01 13:02 ` Vlastimil Babka
@ 2021-02-11 21:26 ` Oscar Salvador
  2021-02-11 21:29   ` Oscar Salvador
  1 sibling, 1 reply; 4+ messages in thread
From: Oscar Salvador @ 2021-02-11 21:26 UTC (permalink / raw)
  To: akpm; +Cc: hannes, linux-mm, linux-kernel

On Mon, Feb 01, 2021 at 07:06:51AM +0100, 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>

Hi Andrew,

is this on your radar?

Thanks!

-- 
Oscar Salvador
SUSE L3


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] mm: workingset: clarify eviction order and distance calculation
  2021-02-11 21:26 ` Oscar Salvador
@ 2021-02-11 21:29   ` Oscar Salvador
  0 siblings, 0 replies; 4+ messages in thread
From: Oscar Salvador @ 2021-02-11 21:29 UTC (permalink / raw)
  To: akpm; +Cc: hannes, linux-mm, linux-kernel

On Thu, Feb 11, 2021 at 10:26:45PM +0100, Oscar Salvador wrote:
> Hi Andrew,
> 
> is this on your radar?

Please, disregard this, I was obviously blind as I did not spot it
in mmotm.

> 
> Thanks!
> 
> -- 
> Oscar Salvador
> SUSE L3
> 

-- 
Oscar Salvador
SUSE L3


^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2021-02-11 21:29 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-01  6:06 [PATCH] mm: workingset: clarify eviction order and distance calculation Oscar Salvador
2021-02-01 13:02 ` Vlastimil Babka
2021-02-11 21:26 ` Oscar Salvador
2021-02-11 21:29   ` Oscar Salvador

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).