Re: [PATCH -next] mm: memcg: remove redundant seq_buf_has_overflowed()

[Michal Hocko <mhocko@suse.com>]

On Fri 28-06-24 10:20:23, xiujianfeng wrote:
> 
> 
> On 2024/6/27 19:54, Michal Hocko wrote:
> > On Thu 27-06-24 19:43:06, xiujianfeng wrote:
> >>
> >>
> >> On 2024/6/27 19:20, Michal Hocko wrote:
> >>> On Thu 27-06-24 16:33:00, xiujianfeng wrote:
> >>>>
> >>>>
> >>>> On 2024/6/27 15:13, Michal Hocko wrote:
> >>>>> On Wed 26-06-24 09:42:32, Xiu Jianfeng wrote:
> >>>>>> Both the end of memory_stat_format() and memcg_stat_format() will call
> >>>>>> WARN_ON_ONCE(seq_buf_has_overflowed()). However, memory_stat_format()
> >>>>>> is the only caller of memcg_stat_format(), when memcg is on the default
> >>>>>> hierarchy, seq_buf_has_overflowed() will be executed twice, so remove
> >>>>>> the reduntant one.
> >>>>>
> >>>>> Shouldn't we rather remove both? Are they giving us anything useful
> >>>>> actually? Would a simpl pr_warn be sufficient? Afterall all we care
> >>>>> about is to learn that we need to grow the buffer size because our stats
> >>>>> do not fit anymore. It is not really important whether that is an OOM or
> >>>>> cgroupfs interface path.
> >>>>
> >>>> I did a test, when I removed both of them and added a lot of prints in
> >>>> memcg_stat_format() to make the seq_buf overflow, and then cat
> >>>> memory.stat in user mode, no OOM occurred, and there were no warning
> >>>> logs in the kernel.
> >>>
> >>> The default buffer size is PAGE_SIZE.
> >>
> >> Hi Michal,
> >>
> >> I'm sorry, I didn't understand what you meant by this sentence. What I
> >> mean is that we can't remove both, otherwise, neither the kernel nor
> >> user space would be aware of a buffer overflow. From my test, there was
> >> no OOM or other exceptions when the overflow occurred; it just resulted
> >> in the displayed information being truncated. Therefore, we need to keep
> >> one.
> > 
> > I've had this in mind
> > 
> > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > index 71fe2a95b8bd..3e17b9c3a27a 100644
> > --- a/mm/memcontrol.c
> > +++ b/mm/memcontrol.c
> > @@ -1845,9 +1845,6 @@ static void memcg_stat_format(struct mem_cgroup *memcg, struct seq_buf *s)
> >  			       vm_event_name(memcg_vm_event_stat[i]),
> >  			       memcg_events(memcg, memcg_vm_event_stat[i]));
> >  	}
> > -
> > -	/* The above should easily fit into one page */
> > -	WARN_ON_ONCE(seq_buf_has_overflowed(s));
> >  }
> >  
> >  static void memcg1_stat_format(struct mem_cgroup *memcg, struct seq_buf *s);
> > @@ -1858,7 +1855,8 @@ static void memory_stat_format(struct mem_cgroup *memcg, struct seq_buf *s)
> >  		memcg_stat_format(memcg, s);
> >  	else
> >  		memcg1_stat_format(memcg, s);
> > -	WARN_ON_ONCE(seq_buf_has_overflowed(s));
> > +	if (seq_buf_has_overflowed(s))
> > +		pr_warn("%s: Stat buffer insufficient please report\n", __FUNCTION__);
> 
> I found that after the change, the effect is as follows:
> 
> # dmesg
> [   51.028327] memory_stat_format: Stat buffer insufficient please report
> 
> with no keywords such as "Failed", "Warning" to draw attention to this
> printout. Should we change it to the following?
> 
> if (seq_buf_has_overflowed(s))
>       pr_warn("%s: Warning, Stat buffer overflow, please report\n",
> __FUNCTION__);

LGTM.
-- 
Michal Hocko
SUSE Labs