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