Re: [PATCH v2 4/4] mm: memcontrol: fix unexpected massive positive number in memcg_state_val_in_pages()

[Lorenzo Stoakes (Oracle)]

On Thu, Mar 26, 2026 at 05:32:05PM +0800, Qi Zheng wrote:
>
>
> On 3/26/26 5:16 PM, Lorenzo Stoakes (Oracle) wrote:
> > On Wed, Mar 25, 2026 at 10:13:25PM +0800, Qi Zheng wrote:
> > > ---
> > >   mm/memcontrol.c | 7 +++++--
> > >   1 file changed, 5 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > > index 04076a139dbe3..0c249255ebefb 100644
> > > --- a/mm/memcontrol.c
> > > +++ b/mm/memcontrol.c
> > > @@ -787,11 +787,14 @@ static int memcg_page_state_unit(int item);
> > >   static long memcg_state_val_in_pages(int idx, long val)
> > >   {
> > >   	int unit = memcg_page_state_unit(idx);
> > > +	long res;
> > >
> > >   	if (!val || unit == PAGE_SIZE)
> > >   		return val;
> > > -	else
> > > -		return max(val * unit / PAGE_SIZE, 1UL);
> >
> > Hm this was already fairly horrid, because we're comparing an unsigned long
> > value of 1 vs. a ULONG_MAX - abs(val) so this was intended to make 0 -> 1UL
> > but not what you'd mathematically think this was which was to make negative
> > values (logically < 1) -> 1.
> >
> > Of course before it was just broken and would promote (val * unit /
> > PAGE_SIZE) to unsigned long first (thus massive number) and return that :)
> >
> > > +
> > > +	res = max(mult_frac(abs(val), unit, PAGE_SIZE), 1UL);
> >
> > This is way too compressed into one line and retains the confusing
> > behaviour.
> >
> > Could we split this out and explain what we're doing (sign-extension,
> > integer promotion and all of this stuff is confusing - so let's just accept
> > that and spell it out):
> >
> > 	/* Get the absolute value of (val * unit / PAGE_SIZE). */
> > 	res = mult_frac(abs(val), unit, PAGE_SIZE);
> > 	/* Round up zero values. */
> > 	res = res ?: 1;
> > 	/* Retain sign. */
> > 	return val < 0 ? -res : res;
> >
> > This is functionally identical, but a lot more readable, I think.
>
> Make sense, I will update to v3.

Thanks!

>
> If Andrew needs me to merge this patchset into "[PATCH v6 00/33] Eliminate
> Dying Memory Cgroup" [1], then I will develop and send v7.
>
> [1].
> https://lore.kernel.org/all/cover.1772711148.git.zhengqi.arch@xxxxxxxxxxxxx/

Oh that's your series too :)

That would be ideal unless that's already in mm-stable, as the series ordering
will give us strict ordering on patches.

Anyway let's wait for Andrew on that!

Cheers, Lorenzo