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__); } /** Because WARN_ON_ONCE doesn't buy us anything actually. It will dump stack trace and it seems really mouthfull (and it will panic when panic_on_warn is enabled which is likely not a great thing). -- Michal Hocko SUSE Labs