Re: [PATCH v3 2/3] mm: memcontrol: change val type to long in __mod_memcg_{lruvec_}state()

[Qi Zheng]

On 3/30/26 9:25 AM, Harry Yoo (Oracle) wrote:
> On Fri, Mar 27, 2026 at 06:16:29PM +0800, Qi Zheng wrote:
> > From: Qi Zheng <zhengqi.arch@xxxxxxxxxxxxx>
> >
> > The __mod_memcg_state() and __mod_memcg_lruvec_state() functions are also
> > used to reparent non-hierarchical stats. In this scenario, the values
> > passed to them are accumulated statistics that might be extremely large
> > and exceed the upper limit of a 32-bit integer.
> >
> > Change the val parameter type from int to long in these functions and
> > their corresponding tracepoints (memcg_rstat_stats) to prevent potential
> > overflow issues.
> >
> > After that, in memcg_state_val_in_pages(), if the passed val is negative,
> > the expression val * unit / PAGE_SIZE could be implicitly converted to a
> > massive positive number when compared with 1UL in the max() macro.
> > This leads to returning an incorrect massive positive value.
> >
> > Fix this by using abs(val) to calculate the magnitude first, and then
> > restoring the sign of the value before returning the result. Additionally,
> > use mult_frac() to prevent potential overflow during the multiplication of
> > val and unit.
> >
> > Reported-by: Harry Yoo (Oracle) <harry@xxxxxxxxxx>
> > Signed-off-by: Qi Zheng <zhengqi.arch@xxxxxxxxxxxxx>
> > Reviewed-by: Lorenzo Stoakes (Oracle) <ljs@xxxxxxxxxx>
> > ---
>
> Looks good to me,
> Reviewed-by: Harry Yoo (Oracle) <harry@xxxxxxxxxx>

Thanks!

> > @@ -831,7 +837,7 @@ static inline void get_non_dying_memcg_end(void)
> >   #endif
> >   
> >   static void __mod_memcg_state(struct mem_cgroup *memcg,
> > -			      enum memcg_stat_item idx, int val)
> > +			      enum memcg_stat_item idx, long val)
> >   {
> >   	int i = memcg_stats_index(idx);
> >   	int cpu;
> > @@ -896,7 +902,7 @@ void reparent_memcg_state_local(struct mem_cgroup *memcg,
> >   #endif
> >   
> >   static void __mod_memcg_lruvec_state(struct mem_cgroup_per_node *pn,
> > -				     enum node_stat_item idx, int val)
> > +				     enum node_stat_item idx, long val)
> >   {
> >   	struct mem_cgroup *memcg = pn->memcg;
> >   	int i = memcg_stats_index(idx);
>
> Some of code paths that calls mod_memcg{,_lruvec}_state still passes
> int values (which is quite subtle to notice), but it should be fine

Right, it happens in too many places, for example, the callers of
mod_lruvec_state().

> as reparenting is not involved in the path and could be cleaned up later.

Agree.

>