| Message ID | 20220620004233.3805-25-kent.overstreet@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Printbufs - new data structure for building strings | expand |
On Sun 19-06-22 20:42:23, Kent Overstreet wrote: > This converts memory_stat_format() from seq_buf to printbuf. Printbuf is > simalar to seq_buf except that it heap allocates the string buffer: > here, we were already heap allocating the buffer with kmalloc() so the > conversion is trivial. > > Signed-off-by: Kent Overstreet <kent.overstreet@gmail.com> I have asked for a justification two times already not hearing anything. Please drop this patch. I do not see any actual advantage of the conversion. The primary downside of the existing code is that an internal buffer is exposed to the caller which is error prone and ugly. The conversion doesn't really address that part. Moreover there is an inconsistency between failrure case where the printbuf is destroyed by a docummented way (printbuf_exit) and when the caller frees the buffer directly. If printbuf_exit evers need to do more than just kfree (e.g. kvfree) then this is a subtle bug hard to find. > --- > mm/memcontrol.c | 68 ++++++++++++++++++++++++------------------------- > 1 file changed, 33 insertions(+), 35 deletions(-) > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > index 598fece89e..57861dc9fe 100644 > --- a/mm/memcontrol.c > +++ b/mm/memcontrol.c > @@ -62,7 +62,7 @@ > #include <linux/file.h> > #include <linux/resume_user_mode.h> > #include <linux/psi.h> > -#include <linux/seq_buf.h> > +#include <linux/printbuf.h> > #include "internal.h" > #include <net/sock.h> > #include <net/ip.h> > @@ -1461,13 +1461,9 @@ static inline unsigned long memcg_page_state_output(struct mem_cgroup *memcg, > > static char *memory_stat_format(struct mem_cgroup *memcg) > { > - struct seq_buf s; > + struct printbuf buf = PRINTBUF; > int i; > > - seq_buf_init(&s, kmalloc(PAGE_SIZE, GFP_KERNEL), PAGE_SIZE); > - if (!s.buffer) > - return NULL; > - > /* > * Provide statistics on the state of the memory subsystem as > * well as cumulative event counters that show past behavior. > @@ -1484,49 +1480,51 @@ static char *memory_stat_format(struct mem_cgroup *memcg) > u64 size; > > size = memcg_page_state_output(memcg, memory_stats[i].idx); > - seq_buf_printf(&s, "%s %llu\n", memory_stats[i].name, size); > + prt_printf(&buf, "%s %llu\n", memory_stats[i].name, size); > > if (unlikely(memory_stats[i].idx == NR_SLAB_UNRECLAIMABLE_B)) { > size += memcg_page_state_output(memcg, > NR_SLAB_RECLAIMABLE_B); > - seq_buf_printf(&s, "slab %llu\n", size); > + prt_printf(&buf, "slab %llu\n", size); > } > } > > /* Accumulated memory events */ > > - seq_buf_printf(&s, "%s %lu\n", vm_event_name(PGFAULT), > - memcg_events(memcg, PGFAULT)); > - seq_buf_printf(&s, "%s %lu\n", vm_event_name(PGMAJFAULT), > - memcg_events(memcg, PGMAJFAULT)); > - seq_buf_printf(&s, "%s %lu\n", vm_event_name(PGREFILL), > - memcg_events(memcg, PGREFILL)); > - seq_buf_printf(&s, "pgscan %lu\n", > - memcg_events(memcg, PGSCAN_KSWAPD) + > - memcg_events(memcg, PGSCAN_DIRECT)); > - seq_buf_printf(&s, "pgsteal %lu\n", > - memcg_events(memcg, PGSTEAL_KSWAPD) + > - memcg_events(memcg, PGSTEAL_DIRECT)); > - seq_buf_printf(&s, "%s %lu\n", vm_event_name(PGACTIVATE), > - memcg_events(memcg, PGACTIVATE)); > - seq_buf_printf(&s, "%s %lu\n", vm_event_name(PGDEACTIVATE), > - memcg_events(memcg, PGDEACTIVATE)); > - seq_buf_printf(&s, "%s %lu\n", vm_event_name(PGLAZYFREE), > - memcg_events(memcg, PGLAZYFREE)); > - seq_buf_printf(&s, "%s %lu\n", vm_event_name(PGLAZYFREED), > - memcg_events(memcg, PGLAZYFREED)); > + prt_printf(&buf, "%s %lu\n", vm_event_name(PGFAULT), > + memcg_events(memcg, PGFAULT)); > + prt_printf(&buf, "%s %lu\n", vm_event_name(PGMAJFAULT), > + memcg_events(memcg, PGMAJFAULT)); > + prt_printf(&buf, "%s %lu\n", vm_event_name(PGREFILL), > + memcg_events(memcg, PGREFILL)); > + prt_printf(&buf, "pgscan %lu\n", > + memcg_events(memcg, PGSCAN_KSWAPD) + > + memcg_events(memcg, PGSCAN_DIRECT)); > + prt_printf(&buf, "pgsteal %lu\n", > + memcg_events(memcg, PGSTEAL_KSWAPD) + > + memcg_events(memcg, PGSTEAL_DIRECT)); > + prt_printf(&buf, "%s %lu\n", vm_event_name(PGACTIVATE), > + memcg_events(memcg, PGACTIVATE)); > + prt_printf(&buf, "%s %lu\n", vm_event_name(PGDEACTIVATE), > + memcg_events(memcg, PGDEACTIVATE)); > + prt_printf(&buf, "%s %lu\n", vm_event_name(PGLAZYFREE), > + memcg_events(memcg, PGLAZYFREE)); > + prt_printf(&buf, "%s %lu\n", vm_event_name(PGLAZYFREED), > + memcg_events(memcg, PGLAZYFREED)); > > #ifdef CONFIG_TRANSPARENT_HUGEPAGE > - seq_buf_printf(&s, "%s %lu\n", vm_event_name(THP_FAULT_ALLOC), > - memcg_events(memcg, THP_FAULT_ALLOC)); > - seq_buf_printf(&s, "%s %lu\n", vm_event_name(THP_COLLAPSE_ALLOC), > - memcg_events(memcg, THP_COLLAPSE_ALLOC)); > + prt_printf(&buf, "%s %lu\n", vm_event_name(THP_FAULT_ALLOC), > + memcg_events(memcg, THP_FAULT_ALLOC)); > + prt_printf(&buf, "%s %lu\n", vm_event_name(THP_COLLAPSE_ALLOC), > + memcg_events(memcg, THP_COLLAPSE_ALLOC)); > #endif /* CONFIG_TRANSPARENT_HUGEPAGE */ > > - /* The above should easily fit into one page */ > - WARN_ON_ONCE(seq_buf_has_overflowed(&s)); > + if (buf.allocation_failure) { > + printbuf_exit(&buf); > + return NULL; > + } > > - return s.buffer; > + return buf.buf; > } > > #define K(x) ((x) << (PAGE_SHIFT-10)) > -- > 2.36.1
On Mon, Jun 20, 2022 at 01:37:56PM +0200, Michal Hocko wrote: > On Sun 19-06-22 20:42:23, Kent Overstreet wrote: > > This converts memory_stat_format() from seq_buf to printbuf. Printbuf is > > simalar to seq_buf except that it heap allocates the string buffer: > > here, we were already heap allocating the buffer with kmalloc() so the > > conversion is trivial. > > > > Signed-off-by: Kent Overstreet <kent.overstreet@gmail.com> > > I have asked for a justification two times already not hearing anything. > Please drop this patch. I do not see any actual advantage of the > conversion. The primary downside of the existing code is that an > internal buffer is exposed to the caller which is error prone and ugly. > The conversion doesn't really address that part. Do you want to tone down the hostility? Yeesh. This patch is part of a wider series that deletes seq_buf, if you missed it here you go: https://lore.kernel.org/all/20220620004233.3805-1-kent.overstreet@gmail.com/ > Moreover there is an inconsistency between failrure case where the > printbuf is destroyed by a docummented way (printbuf_exit) and when the > caller frees the buffer directly. If printbuf_exit evers need to do more > than just kfree (e.g. kvfree) then this is a subtle bug hard to find. Ok, _that's_ a technical point we can talk about and address. I'll add documentation to the printbuf code that the buffer must be freeable with kfree().
On Mon 20-06-22 11:13:56, Kent Overstreet wrote: > On Mon, Jun 20, 2022 at 01:37:56PM +0200, Michal Hocko wrote: > > On Sun 19-06-22 20:42:23, Kent Overstreet wrote: > > > This converts memory_stat_format() from seq_buf to printbuf. Printbuf is > > > simalar to seq_buf except that it heap allocates the string buffer: > > > here, we were already heap allocating the buffer with kmalloc() so the > > > conversion is trivial. > > > > > > Signed-off-by: Kent Overstreet <kent.overstreet@gmail.com> > > > > I have asked for a justification two times already not hearing anything. > > Please drop this patch. I do not see any actual advantage of the > > conversion. The primary downside of the existing code is that an > > internal buffer is exposed to the caller which is error prone and ugly. > > The conversion doesn't really address that part. > > Do you want to tone down the hostility? Yeesh. I have merely pointed out you have ignored my review feedback _twice_ already. Ignoring the review feedback and posting new versions without questions being addressed is wasting other people's time. > This patch is part of a wider series that deletes seq_buf, if you missed it here > you go: https://lore.kernel.org/all/20220620004233.3805-1-kent.overstreet@gmail.com/ Each patch should have its justification. If the reasoning is that seq_buf is going away then I can live with that. That is not obvious from this patch which I care about because it falls into area I maintain and review. Unlike the rest of the large patchset which I do not really have time to review in its entirety. > > Moreover there is an inconsistency between failrure case where the > > printbuf is destroyed by a docummented way (printbuf_exit) and when the > > caller frees the buffer directly. If printbuf_exit evers need to do more > > than just kfree (e.g. kvfree) then this is a subtle bug hard to find. > > Ok, _that's_ a technical point we can talk about and address. I'll add > documentation to the printbuf code that the buffer must be freeable with > kfree(). Hmm, wouldn't that be just too restrictive without any good reasons? Maybe there are no seq_buf users currently but if there ever raises a need for larger buffers then you might want to use kvmalloc for the allocation and you will need to change all users to use kvfree (potentially missing some). You could either start requiring kvfree since the beginning or get rid of exposing internal buffer altogether and use printbuf_exit in all cases.
diff --git a/mm/memcontrol.c b/mm/memcontrol.c index 598fece89e..57861dc9fe 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -62,7 +62,7 @@ #include <linux/file.h> #include <linux/resume_user_mode.h> #include <linux/psi.h> -#include <linux/seq_buf.h> +#include <linux/printbuf.h> #include "internal.h" #include <net/sock.h> #include <net/ip.h> @@ -1461,13 +1461,9 @@ static inline unsigned long memcg_page_state_output(struct mem_cgroup *memcg, static char *memory_stat_format(struct mem_cgroup *memcg) { - struct seq_buf s; + struct printbuf buf = PRINTBUF; int i; - seq_buf_init(&s, kmalloc(PAGE_SIZE, GFP_KERNEL), PAGE_SIZE); - if (!s.buffer) - return NULL; - /* * Provide statistics on the state of the memory subsystem as * well as cumulative event counters that show past behavior. @@ -1484,49 +1480,51 @@ static char *memory_stat_format(struct mem_cgroup *memcg) u64 size; size = memcg_page_state_output(memcg, memory_stats[i].idx); - seq_buf_printf(&s, "%s %llu\n", memory_stats[i].name, size); + prt_printf(&buf, "%s %llu\n", memory_stats[i].name, size); if (unlikely(memory_stats[i].idx == NR_SLAB_UNRECLAIMABLE_B)) { size += memcg_page_state_output(memcg, NR_SLAB_RECLAIMABLE_B); - seq_buf_printf(&s, "slab %llu\n", size); + prt_printf(&buf, "slab %llu\n", size); } } /* Accumulated memory events */ - seq_buf_printf(&s, "%s %lu\n", vm_event_name(PGFAULT), - memcg_events(memcg, PGFAULT)); - seq_buf_printf(&s, "%s %lu\n", vm_event_name(PGMAJFAULT), - memcg_events(memcg, PGMAJFAULT)); - seq_buf_printf(&s, "%s %lu\n", vm_event_name(PGREFILL), - memcg_events(memcg, PGREFILL)); - seq_buf_printf(&s, "pgscan %lu\n", - memcg_events(memcg, PGSCAN_KSWAPD) + - memcg_events(memcg, PGSCAN_DIRECT)); - seq_buf_printf(&s, "pgsteal %lu\n", - memcg_events(memcg, PGSTEAL_KSWAPD) + - memcg_events(memcg, PGSTEAL_DIRECT)); - seq_buf_printf(&s, "%s %lu\n", vm_event_name(PGACTIVATE), - memcg_events(memcg, PGACTIVATE)); - seq_buf_printf(&s, "%s %lu\n", vm_event_name(PGDEACTIVATE), - memcg_events(memcg, PGDEACTIVATE)); - seq_buf_printf(&s, "%s %lu\n", vm_event_name(PGLAZYFREE), - memcg_events(memcg, PGLAZYFREE)); - seq_buf_printf(&s, "%s %lu\n", vm_event_name(PGLAZYFREED), - memcg_events(memcg, PGLAZYFREED)); + prt_printf(&buf, "%s %lu\n", vm_event_name(PGFAULT), + memcg_events(memcg, PGFAULT)); + prt_printf(&buf, "%s %lu\n", vm_event_name(PGMAJFAULT), + memcg_events(memcg, PGMAJFAULT)); + prt_printf(&buf, "%s %lu\n", vm_event_name(PGREFILL), + memcg_events(memcg, PGREFILL)); + prt_printf(&buf, "pgscan %lu\n", + memcg_events(memcg, PGSCAN_KSWAPD) + + memcg_events(memcg, PGSCAN_DIRECT)); + prt_printf(&buf, "pgsteal %lu\n", + memcg_events(memcg, PGSTEAL_KSWAPD) + + memcg_events(memcg, PGSTEAL_DIRECT)); + prt_printf(&buf, "%s %lu\n", vm_event_name(PGACTIVATE), + memcg_events(memcg, PGACTIVATE)); + prt_printf(&buf, "%s %lu\n", vm_event_name(PGDEACTIVATE), + memcg_events(memcg, PGDEACTIVATE)); + prt_printf(&buf, "%s %lu\n", vm_event_name(PGLAZYFREE), + memcg_events(memcg, PGLAZYFREE)); + prt_printf(&buf, "%s %lu\n", vm_event_name(PGLAZYFREED), + memcg_events(memcg, PGLAZYFREED)); #ifdef CONFIG_TRANSPARENT_HUGEPAGE - seq_buf_printf(&s, "%s %lu\n", vm_event_name(THP_FAULT_ALLOC), - memcg_events(memcg, THP_FAULT_ALLOC)); - seq_buf_printf(&s, "%s %lu\n", vm_event_name(THP_COLLAPSE_ALLOC), - memcg_events(memcg, THP_COLLAPSE_ALLOC)); + prt_printf(&buf, "%s %lu\n", vm_event_name(THP_FAULT_ALLOC), + memcg_events(memcg, THP_FAULT_ALLOC)); + prt_printf(&buf, "%s %lu\n", vm_event_name(THP_COLLAPSE_ALLOC), + memcg_events(memcg, THP_COLLAPSE_ALLOC)); #endif /* CONFIG_TRANSPARENT_HUGEPAGE */ - /* The above should easily fit into one page */ - WARN_ON_ONCE(seq_buf_has_overflowed(&s)); + if (buf.allocation_failure) { + printbuf_exit(&buf); + return NULL; + } - return s.buffer; + return buf.buf; } #define K(x) ((x) << (PAGE_SHIFT-10))
This converts memory_stat_format() from seq_buf to printbuf. Printbuf is simalar to seq_buf except that it heap allocates the string buffer: here, we were already heap allocating the buffer with kmalloc() so the conversion is trivial. Signed-off-by: Kent Overstreet <kent.overstreet@gmail.com> --- mm/memcontrol.c | 68 ++++++++++++++++++++++++------------------------- 1 file changed, 33 insertions(+), 35 deletions(-)