| Message ID | 20240626094232.2432891-1-xiujianfeng@huawei.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [-next] mm: memcg: remove redundant seq_buf_has_overflowed() | expand |
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. > Signed-off-by: Xiu Jianfeng <xiujianfeng@huawei.com> > --- > mm/memcontrol.c | 3 --- > 1 file changed, 3 deletions(-) > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > index 974bd160838c..776d22bc66a2 100644 > --- a/mm/memcontrol.c > +++ b/mm/memcontrol.c > @@ -1846,9 +1846,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); > -- > 2.34.1
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. > >> Signed-off-by: Xiu Jianfeng <xiujianfeng@huawei.com> >> --- >> mm/memcontrol.c | 3 --- >> 1 file changed, 3 deletions(-) >> >> diff --git a/mm/memcontrol.c b/mm/memcontrol.c >> index 974bd160838c..776d22bc66a2 100644 >> --- a/mm/memcontrol.c >> +++ b/mm/memcontrol.c >> @@ -1846,9 +1846,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); >> -- >> 2.34.1 >
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.
On Thu, Jun 27, 2024 at 12:13 AM Michal Hocko <mhocko@suse.com> 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. Is it possible for userspace readers to break if the stats are incomplete? If yes, I think WARN_ON_ONCE() may be prompted to make it easier to catch and fix before deployment.
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. >
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).
On Thu 27-06-24 04:33:50, Yosry Ahmed wrote: > On Thu, Jun 27, 2024 at 12:13 AM Michal Hocko <mhocko@suse.com> 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. > > Is it possible for userspace readers to break if the stats are > incomplete? They will certainly get an imprecise picture. Sufficient to break I dunno. > If yes, I think WARN_ON_ONCE() may be prompted to make it > easier to catch and fix before deployment. The only advantage of WARN_ON_ONCE is that the splat is so verbose that it gets noticed. And also it panics the system if panic_on_warn is enabled. I do not particularly care about the latter but to me it seems like the warning is just an over reaction and a simple pr_warn should just achieve the similar effect - see my other reply
On Thu, Jun 27, 2024 at 4:56 AM Michal Hocko <mhocko@suse.com> wrote: > > On Thu 27-06-24 04:33:50, Yosry Ahmed wrote: > > On Thu, Jun 27, 2024 at 12:13 AM Michal Hocko <mhocko@suse.com> 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. > > > > Is it possible for userspace readers to break if the stats are > > incomplete? > > They will certainly get an imprecise picture. Sufficient to break I > dunno. If some stats go completely missing and a parser expects them to always be there, I think they may break. > > > If yes, I think WARN_ON_ONCE() may be prompted to make it > > easier to catch and fix before deployment. > > The only advantage of WARN_ON_ONCE is that the splat is so verbose that > it gets noticed. Right, that's exactly what I meant. > And also it panics the system if panic_on_warn is > enabled. I do not particularly care about the latter but to me it seems > like the warning is just an over reaction and a simple pr_warn should > just achieve the similar effect - see my other reply If pr_warn()'s usually get noticed in a timely manner (by testers or bots), then I think pr_warn() would be sufficient. If they can go unnoticed for a while, I think WARN_ON_ONCE() may be warranted to avoid the possibility of breaking a userspace parser.
On Thu 27-06-24 05:00:18, Yosry Ahmed wrote: > On Thu, Jun 27, 2024 at 4:56 AM Michal Hocko <mhocko@suse.com> wrote: > > > > On Thu 27-06-24 04:33:50, Yosry Ahmed wrote: > > > On Thu, Jun 27, 2024 at 12:13 AM Michal Hocko <mhocko@suse.com> 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. > > > > > > Is it possible for userspace readers to break if the stats are > > > incomplete? > > > > They will certainly get an imprecise picture. Sufficient to break I > > dunno. > > If some stats go completely missing and a parser expects them to > always be there, I think they may break. If they break, we will eventually learn about that with or without warning. It is true that WARN* is so vocal that people/tooling might just report that even without breakage but that to me sounds like abusing WARNING. There were times when this was not a big deal but now when WARN* are getting CVEs because panic_on_warn this useful debugging tool has become a new BUG on.
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).
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.
diff --git a/mm/memcontrol.c b/mm/memcontrol.c index 974bd160838c..776d22bc66a2 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -1846,9 +1846,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);
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. Signed-off-by: Xiu Jianfeng <xiujianfeng@huawei.com> --- mm/memcontrol.c | 3 --- 1 file changed, 3 deletions(-)