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__);
> }
> > /**
>
> 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).
