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

[xiujianfeng]

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).