[PATCH linux-next v2] mm: workingset: update description of the source file

[PATCH linux-next v2] mm: workingset: update description of the source file

[yang.yang29]

From: Yang Yang <yang.yang29@zte.com.cn>

The calculation of workingset size is the core logic of handling refault,
it had been updated several times[1][2] after workingset.c was created[3].
But the description hadn't been updated accordingly, this mismatch may
confuse the readers.
So we update the description to make it consistent to the code. 

[1] commit 34e58cac6d8f ("mm: workingset: let cache workingset challenge anon")
[2] commit aae466b0052e ("mm/swap: implement workingset detection for anonymous LRU")
[3] commit a528910e12ec ("mm: thrash detection-based file cache sizing")

Signed-off-by: Yang Yang <yang.yang29@zte.com.cn>
---
change for v2 - Update commit of workingset_refault() suggested Johannes Weiner.
See https://lore.kernel.org/all/20230316143007.GC116016@cmpxchg.org/
---
 mm/workingset.c | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/mm/workingset.c b/mm/workingset.c
index a304e8571d54..b864eec49ddd 100644
--- a/mm/workingset.c
+++ b/mm/workingset.c
@@ -111,9 +111,20 @@
  *
  *   NR_inactive + (R - E) <= NR_inactive + NR_active
  *
- * which can be further simplified to
+ * If we have swap we should consider about NR_inactive_anon and
+ * NR_active_anon, so for page cache and anonymous respectively:
  *
- *   (R - E) <= NR_active
+ *   NR_inactive_file + (R - E) <= NR_inactive_file + NR_active_file
+ *   + NR_inactive_anon + NR_active_anon
+ *
+ *   NR_inactive_anon + (R - E) <= NR_inactive_anon + NR_active_anon
+ *   + NR_inactive_file + NR_active_file
+ *
+ * Which can be further simplified to:
+ *
+ *   (R - E) <= NR_active_file + NR_inactive_anon + NR_active_anon
+ *
+ *   (R - E) <= NR_active_anon + NR_inactive_file + NR_active_file
  *
  * Put into words, the refault distance (out-of-cache) can be seen as
  * a deficit in inactive list space (in-cache).  If the inactive list
@@ -130,14 +141,14 @@
  * are no longer in active use.
  *
  * So when a refault distance of (R - E) is observed and there are at
- * least (R - E) active pages, the refaulting page is activated
- * optimistically in the hope that (R - E) active pages are actually
+ * least (R - E) pages in the userspace workingset, the refaulting page
+ * is activated optimistically in the hope that (R - E) pages are actually
  * used less frequently than the refaulting page - or even not used at
  * all anymore.
  *
  * That means if inactive cache is refaulting with a suitable refault
  * distance, we assume the cache workingset is transitioning and put
- * pressure on the current active list.
+ * pressure on the current workingset.
  *
  * If this is wrong and demotion kicks in, the pages which are truly
  * used more frequently will be reactivated while the less frequently
@@ -468,7 +479,7 @@ void workingset_refault(struct folio *folio, void *shadow)
 	 * don't activate pages that couldn't stay resident even if
 	 * all the memory was available to the workingset. For page
 	 * cache whether workingset competition needs to consider
-	 * anon or not depends on having swap.
+	 * anon or not depends on having free swap sapce.
 	 */
 	workingset_size = lruvec_page_state(eviction_lruvec, NR_ACTIVE_FILE);
 	/* For anonymous page */
-- 
2.25.1

Re: [PATCH linux-next v2] mm: workingset: update description of the source file

[Vlastimil Babka]

On 4/13/23 10:34, yang.yang29@zte.com.cn wrote:
> From: Yang Yang <yang.yang29@zte.com.cn>
> 
> The calculation of workingset size is the core logic of handling refault,
> it had been updated several times[1][2] after workingset.c was created[3].
> But the description hadn't been updated accordingly, this mismatch may
> confuse the readers.
> So we update the description to make it consistent to the code. 
> 
> [1] commit 34e58cac6d8f ("mm: workingset: let cache workingset challenge anon")
> [2] commit aae466b0052e ("mm/swap: implement workingset detection for anonymous LRU")
> [3] commit a528910e12ec ("mm: thrash detection-based file cache sizing")
> 
> Signed-off-by: Yang Yang <yang.yang29@zte.com.cn>

I'm late but FWIW, not supper happy that while the updated calculations are
now accurate wrt the actual code, the explanation (which was written at the
time of page cache-only workinset) was more easier to follow in the simpler
form. Now it's still mostly talking about page cache and explaining the
balance between its active and inactive list only, and then suddenly the
anon lists appear out of nowhere in the final equations.

In other words, I think it would have been better to leave that explanation
as it was, and then add a new part describing the extension to anon pages.

But nevermind, not a reason to yank/revert the change from mm-stable, but I
should be eventually getting back to this and maybe move this to mm docs
while doing what I described above (CCing Mike so he can remind me later of
this yet another promise :)

Vlastimil

> ---
> change for v2 - Update commit of workingset_refault() suggested Johannes Weiner.
> See https://lore.kernel.org/all/20230316143007.GC116016@cmpxchg.org/
> ---
>  mm/workingset.c | 23 +++++++++++++++++------
>  1 file changed, 17 insertions(+), 6 deletions(-)
> 
> diff --git a/mm/workingset.c b/mm/workingset.c
> index a304e8571d54..b864eec49ddd 100644
> --- a/mm/workingset.c
> +++ b/mm/workingset.c
> @@ -111,9 +111,20 @@
>   *
>   *   NR_inactive + (R - E) <= NR_inactive + NR_active
>   *
> - * which can be further simplified to
> + * If we have swap we should consider about NR_inactive_anon and
> + * NR_active_anon, so for page cache and anonymous respectively:
>   *
> - *   (R - E) <= NR_active
> + *   NR_inactive_file + (R - E) <= NR_inactive_file + NR_active_file
> + *   + NR_inactive_anon + NR_active_anon
> + *
> + *   NR_inactive_anon + (R - E) <= NR_inactive_anon + NR_active_anon
> + *   + NR_inactive_file + NR_active_file
> + *
> + * Which can be further simplified to:
> + *
> + *   (R - E) <= NR_active_file + NR_inactive_anon + NR_active_anon
> + *
> + *   (R - E) <= NR_active_anon + NR_inactive_file + NR_active_file
>   *
>   * Put into words, the refault distance (out-of-cache) can be seen as
>   * a deficit in inactive list space (in-cache).  If the inactive list
> @@ -130,14 +141,14 @@
>   * are no longer in active use.
>   *
>   * So when a refault distance of (R - E) is observed and there are at
> - * least (R - E) active pages, the refaulting page is activated
> - * optimistically in the hope that (R - E) active pages are actually
> + * least (R - E) pages in the userspace workingset, the refaulting page
> + * is activated optimistically in the hope that (R - E) pages are actually
>   * used less frequently than the refaulting page - or even not used at
>   * all anymore.
>   *
>   * That means if inactive cache is refaulting with a suitable refault
>   * distance, we assume the cache workingset is transitioning and put
> - * pressure on the current active list.
> + * pressure on the current workingset.
>   *
>   * If this is wrong and demotion kicks in, the pages which are truly
>   * used more frequently will be reactivated while the less frequently
> @@ -468,7 +479,7 @@ void workingset_refault(struct folio *folio, void *shadow)
>  	 * don't activate pages that couldn't stay resident even if
>  	 * all the memory was available to the workingset. For page
>  	 * cache whether workingset competition needs to consider
> -	 * anon or not depends on having swap.
> +	 * anon or not depends on having free swap sapce.
>  	 */
>  	workingset_size = lruvec_page_state(eviction_lruvec, NR_ACTIVE_FILE);
>  	/* For anonymous page */

Re: [PATCH linux-next v2] mm: workingset: update description of the source file

[Vlastimil Babka]

On 4/21/23 13:31, Yang Yang wrote:
>> I'm late but FWIW, not supper happy that while the updated calculations are
>> now accurate wrt the actual code, the explanation (which was written at the
>> time of page cache-only workinset) was more easier to follow in the simpler
>> form. Now it's still mostly talking about page cache and explaining the
>>balance between its active and inactive list only, and then suddenly the
>> anon lists appear out of nowhere in the final equations.
> 
> Thanks for your reviewing! I should update the whole parts. Sorry for
> hadn't do it better, please drop the patch, I will try to submit patchv3
> to fix this.

I think it's too late to drop, and not worth rebasing the git tree in early
merge window for that.

>> In other words, I think it would have been better to leave that explanation
>> as it was, and then add a new part describing the extension to anon pages.
> 
> I read the description of the source file again carefully, and think that
> there is no need to creat a new part, if we explain at the begining that
> the word 'pages' include page cache and anonymous page, and do some minor 
> adjustments. For example:
> Per node, two kinds of clock lists are maintained for pages..

I think it would make following the explanation more complicated, and it's
already difficult enough. Reasoning just about active vs inactive file list
is much simpler to follow the equations and observations behind them. Wonder
what Johannes as the original author thinks, anyway.