Re: [PATCH v3 2/4] mm: memcg: read root lruvec stats from the node counters

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] mm: memcg: missing NR_VMSCAN_WRITE and folio stats in
  lruvec_use_node_stat()
--

commit 95f274e85e4bf7a42c96a69c0a4509c8d43a4417
Author: Joanne Koong <joannelkoong@gmail.com>

mm: memcg: read root lruvec stats from the node counters

This commit makes lruvec_page_state() and lruvec_page_state_monotonic() read
the node counter directly for the root memcg on stats whose node counter
equals the root memcg's hierarchical value.

> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 091401cecf397..196c0382181b1 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -504,13 +504,67 @@ struct lruvec_stats {
>  	long state_pending[NR_MEMCG_NODE_STAT_ITEMS];
>  };
>  
> +/*
> + * Whether to read the node counter for @idx instead of @lruvec's rstat value.
> + * This is only done for the root memcg for stats where every update also
> + * updates a memcg's stats, so the root memcg's hierarchical value equals the
> + * node counter. The node counter is preferred since it is always up to date,
> + * and avoids an rstat flush which for the root memcg flushes every cgroup in
> + * the system.
> + */
> +static bool lruvec_use_node_stat(const struct lruvec *lruvec,
> +				 enum node_stat_item idx)
> +{
> +	if (!mem_cgroup_is_root(lruvec_memcg(lruvec)))
> +		return false;
> +
> +	switch (idx) {
> +	case NR_INACTIVE_ANON:
> +	case NR_ACTIVE_ANON:
> +	case NR_INACTIVE_FILE:
> +	case NR_ACTIVE_FILE:
> +	case NR_UNEVICTABLE:
> +	case WORKINGSET_REFAULT_ANON:
> +	case WORKINGSET_REFAULT_FILE:
> +	case WORKINGSET_ACTIVATE_ANON:
> +	case WORKINGSET_ACTIVATE_FILE:
> +	case WORKINGSET_RESTORE_ANON:
> +	case WORKINGSET_RESTORE_FILE:
> +#ifdef CONFIG_NUMA_BALANCING
> +	case PGPROMOTE_SUCCESS:
> +#endif

[Severity: Medium]
Should lruvec_use_node_stat() also include NR_VMSCAN_WRITE, NR_SWAPCACHE, and
the other charged-folio stats between WORKINGSET_RESTORE_FILE and
PGPROMOTE_SUCCESS in memcg_node_stat_items[]?

The switch statement drops the entire contiguous block from
WORKINGSET_NODERECLAIM through NR_SWAPCACHE rather than only the kernel
memory items (WORKINGSET_NODERECLAIM and NR_VMALLOC..NR_SECONDARY_PAGETABLE):

mm/memcontrol.c:memcg_node_stat_items[] {
    ...
	WORKINGSET_RESTORE_FILE,
	WORKINGSET_NODERECLAIM,
	NR_ANON_MAPPED,
	NR_FILE_MAPPED,
	NR_FILE_PAGES,
	NR_FILE_DIRTY,
	NR_WRITEBACK,
	NR_SHMEM,
	NR_SHMEM_THPS,
	NR_FILE_THPS,
	NR_ANON_THPS,
	NR_VMSCAN_WRITE,
	NR_VMALLOC,
	NR_KERNEL_STACK_KB,
	NR_PAGETABLE,
	NR_SECONDARY_PAGETABLE,
#ifdef CONFIG_SWAP
	NR_SWAPCACHE,
#endif
#ifdef CONFIG_NUMA_BALANCING
	PGPROMOTE_SUCCESS,
#endif
    ...
}

In prepare_scan_control() (the only caller of lruvec_page_state_monotonic()),
anon IO cost is computed as WORKINGSET_RESTORE_ANON + NR_VMSCAN_WRITE.
Because NR_VMSCAN_WRITE is only updated via lruvec_stat_mod_folio() in
pageout() and is omitted here, root-lruvec reclaim mixes live node counters
for PGROTATE_ANON and WORKINGSET_RESTORE_ANON with the rstat counter for
NR_VMSCAN_WRITE.  Similarly, reclaimable_anon_is_low() on root_mem_cgroup
compares live node counters for NR_INACTIVE_ANON + NR_ACTIVE_ANON against the
unflushed rstat counter for NR_SWAPCACHE.

NR_ANON_MAPPED, NR_ANON_THPS, NR_SHMEM, NR_SHMEM_THPS, NR_FILE_THPS, and
NR_FILE_DIRTY are also only updated on charged folios, whereas NR_FILE_PAGES
(updated via node_stat_mod_folio() for swapcache), NR_WRITEBACK (updated via
node_stat_mod_folio() for NFS unstable writes), and NR_FILE_MAPPED (updated
for uncharged pages in vm_insert_page()) can exceed the root memcg's rstat
value and would need to be noted as exceptions if excluded.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261114002822.1043378-1-joannelkoong@gmail.com?part=2