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
