Re: [PATCH 3/4] memcg: int16_t for cached slab stats

[Harry Yoo]

On 5/20/26 2:31 PM, Shakeel Butt wrote:
> Currently struct obj_stock_pcp stores cached slab stats in 'int' which
> is 4 bytes per counter on 64-bit machines. Switch them to int16_t to
> shrink the cached metadata.
>
> The existing PAGE_SIZE flush in __account_obj_stock() bounds *bytes at
> PAGE_SIZE on 4KiB and 16KiB page archs, well within int16_t. On 64KiB
> pages PAGE_SIZE is well above S16_MAX so that flush never fires, and a
> sufficiently long run of accumulations would overflow the cache. Add
> an explicit S16_MAX guard before each add: when the next add would
> push abs(*bytes) past S16_MAX, fold the cached value into @nr and
> flush directly via mod_objcg_mlstate() before the accumulation.
>
> Signed-off-by: Shakeel Butt <shakeel.butt@xxxxxxxxx>
> Tested-by: kernel test robot <oliver.sang@xxxxxxxxx>
> ---

So arches with 64KiB sizes won't benefit from

"Even for large object >= PAGE_SIZE, the vmstat data will still be 
cached locally at least once before pushing it out" case.

But its benefit is questionable (to me), might be ok.

>   mm/memcontrol.c | 16 +++++++++++++---
>   1 file changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index b3d63d9f267c..1ed27fd06850 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -3158,7 +3158,7 @@ static void __account_obj_stock(struct obj_cgroup *objcg,
>   				struct obj_stock_pcp *stock, int nr,
>   				struct pglist_data *pgdat, enum node_stat_item idx)
>   {
> -	int *bytes;
> +	int16_t *bytes;
>   
>   	/*
>   	 * Though at the moment MAX_NUMNODES <= 1024 in all archs but let's make
> @@ -3195,6 +3195,16 @@ static void __account_obj_stock(struct obj_cgroup *objcg,
>   
>   	bytes = (idx == NR_SLAB_RECLAIMABLE_B) ? &stock->nr_slab_reclaimable_b
>   					       : &stock->nr_slab_unreclaimable_b;
> +	/*
> +	 * To avoid overflow or underflow, flush directly if accumulating @nr
> +	 * would push the cached value past S16_MAX.
> +	 */
> +	if (abs(nr + *bytes) >= S16_MAX) {
nit: should be > S16_MAX?

> +		nr += *bytes;
> +		*bytes = 0;
> +		goto direct;
> +	}
> +
>   	/*
>   	 * Even for large object >= PAGE_SIZE, the vmstat data will still be
>   	 * cached locally at least once before pushing it out.

FWIW:
Reviewed-by: Harry Yoo (Oracle) <harry@xxxxxxxxxx>

--
Cheers,
Harry / Hyeonggon