Re: [PATCH 2/4] memcg: uint16_t for nr_bytes in obj_stock_pcp

[David Laight <david.laight.linux@gmail.com>]

On Tue, 19 May 2026 22:31:20 -0700
Shakeel Butt <shakeel.butt@linux.dev> wrote:

> Currently struct obj_stock_pcp stores nr_bytes in an 'unsigned int'
> which is 4 bytes on 64-bit machines. Switch the field to uint16_t to
> shrink the per-CPU cache.
> 
> The kernel supports PAGE_SIZE_4KB, _8KB, _16KB, _32KB, _64KB and
> _256KB (see HAVE_PAGE_SIZE_* in arch/Kconfig). After the
> PAGE_SIZE-aligned flush in __refill_obj_stock(), the sub-page
> remainder fits in uint16_t up through 64KiB pages where PAGE_SIZE - 1
> == U16_MAX, but on 256KiB pages PAGE_SIZE - 1 == 0x3FFFF exceeds
> U16_MAX. The accumulator also needs to stay within uint16_t between
> page-aligned flushes on 64KiB pages where PAGE_SIZE itself is
> U16_MAX + 1.
> 
> Accumulate the new total in an 'unsigned int' local, then:
> 
>   1. Flush whenever the accumulator would hit U16_MAX. Together with
>      the existing allow_uncharge flush at PAGE_SIZE, this keeps the
>      uint16_t safe on PAGE_SIZE <= 64KiB.
> 
>   2. On configs with PAGE_SHIFT > 16 (PAGE_SIZE_256KB on hexagon and
>      powerpc 44x), push any sub-page remainder above U16_MAX into
>      objcg->nr_charged_bytes via atomic_add before storing back, so
>      the store cannot silently truncate. The PAGE_SHIFT > 16 guard
>      folds the branch out at compile time on smaller page sizes.
> 
> Signed-off-by: Shakeel Butt <shakeel.butt@linux.dev>
> Tested-by: kernel test robot <oliver.sang@intel.com>
> ---
>  mm/memcontrol.c | 33 +++++++++++++++++++++++++++------
>  1 file changed, 27 insertions(+), 6 deletions(-)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index d7c162946719..b3d63d9f267c 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -2019,7 +2019,7 @@ static DEFINE_PER_CPU_ALIGNED(struct memcg_stock_pcp, memcg_stock) = {
>  
>  struct obj_stock_pcp {
>  	local_trylock_t lock;
> -	unsigned int nr_bytes;
> +	uint16_t nr_bytes;
>  	struct obj_cgroup *cached_objcg;
>  	int16_t node_id;

You might want to move it to this hole.
The size of 'lock' depends on kernel build options.

-- David

>  	int nr_slab_reclaimable_b;
> @@ -3331,6 +3331,7 @@ static void __refill_obj_stock(struct obj_cgroup *objcg,
>  			       bool allow_uncharge)
>  {
>  	unsigned int nr_pages = 0;
> +	unsigned int stock_nr_bytes;
>  
>  	if (!stock) {
>  		nr_pages = nr_bytes >> PAGE_SHIFT;
> @@ -3339,21 +3340,41 @@ static void __refill_obj_stock(struct obj_cgroup *objcg,
>  		goto out;
>  	}
>  
> +	stock_nr_bytes = stock->nr_bytes;
>  	if (READ_ONCE(stock->cached_objcg) != objcg) { /* reset if necessary */
>  		drain_obj_stock(stock);
>  		obj_cgroup_get(objcg);
> -		stock->nr_bytes = atomic_read(&objcg->nr_charged_bytes)
> +		stock_nr_bytes = atomic_read(&objcg->nr_charged_bytes)
>  				? atomic_xchg(&objcg->nr_charged_bytes, 0) : 0;
>  		WRITE_ONCE(stock->cached_objcg, objcg);
>  
>  		allow_uncharge = true;	/* Allow uncharge when objcg changes */
>  	}
> -	stock->nr_bytes += nr_bytes;
> +	stock_nr_bytes += nr_bytes;
> +
> +	/* Since stock->nr_bytes is uint16_t, don't refill >= U16_MAX */
> +	if ((allow_uncharge && (stock_nr_bytes > PAGE_SIZE)) ||
> +	    stock_nr_bytes >= U16_MAX) {
> +		nr_pages = stock_nr_bytes >> PAGE_SHIFT;
> +		stock_nr_bytes &= (PAGE_SIZE - 1);
> +
> +		/*
> +		 * On configs with PAGE_SHIFT > 16 (PAGE_SIZE_256KB on
> +		 * hexagon and powerpc 44x), the sub-page remainder can
> +		 * still exceed U16_MAX. Push the excess back to
> +		 * objcg->nr_charged_bytes so the store into uint16_t
> +		 * cannot silently truncate; folded out at compile time
> +		 * on smaller page sizes.
> +		 */
> +		if (PAGE_SHIFT > 16 && stock_nr_bytes > U16_MAX) {
> +			unsigned int kept = stock_nr_bytes & U16_MAX;
>  
> -	if (allow_uncharge && (stock->nr_bytes > PAGE_SIZE)) {
> -		nr_pages = stock->nr_bytes >> PAGE_SHIFT;
> -		stock->nr_bytes &= (PAGE_SIZE - 1);
> +			atomic_add(stock_nr_bytes - kept,
> +				   &objcg->nr_charged_bytes);
> +			stock_nr_bytes = kept;
> +		}
>  	}
> +	stock->nr_bytes = stock_nr_bytes;
>  
>  out:
>  	if (nr_pages)