| Message ID | 20220421234837.3629927-9-kent.overstreet@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
On Thu 21-04-22 19:48:32, 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. What is the advantage of changing a well tested seq_buf with a different way to do the same thing here? I do not see this to be a noticeable simplification of the existing code. The only advantage I can see is that the string storage allocation is implicit and it would expand in case we ever overflow over a single page. But is this really worth the code churn? > Signed-off-by: Kent Overstreet <kent.overstreet@gmail.com> > --- > mm/memcontrol.c | 68 ++++++++++++++++++++++++------------------------- > 1 file changed, 33 insertions(+), 35 deletions(-) > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > index 36e9f38c91..4cb0b7bc1c 100644 > --- a/mm/memcontrol.c > +++ b/mm/memcontrol.c > @@ -61,7 +61,7 @@ > #include <linux/file.h> > #include <linux/tracehook.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> > @@ -1436,13 +1436,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. > @@ -1459,49 +1455,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); > + pr_buf(&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); > + pr_buf(&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)); > + pr_buf(&buf, "%s %lu\n", vm_event_name(PGFAULT), > + memcg_events(memcg, PGFAULT)); > + pr_buf(&buf, "%s %lu\n", vm_event_name(PGMAJFAULT), > + memcg_events(memcg, PGMAJFAULT)); > + pr_buf(&buf, "%s %lu\n", vm_event_name(PGREFILL), > + memcg_events(memcg, PGREFILL)); > + pr_buf(&buf, "pgscan %lu\n", > + memcg_events(memcg, PGSCAN_KSWAPD) + > + memcg_events(memcg, PGSCAN_DIRECT)); > + pr_buf(&buf, "pgsteal %lu\n", > + memcg_events(memcg, PGSTEAL_KSWAPD) + > + memcg_events(memcg, PGSTEAL_DIRECT)); > + pr_buf(&buf, "%s %lu\n", vm_event_name(PGACTIVATE), > + memcg_events(memcg, PGACTIVATE)); > + pr_buf(&buf, "%s %lu\n", vm_event_name(PGDEACTIVATE), > + memcg_events(memcg, PGDEACTIVATE)); > + pr_buf(&buf, "%s %lu\n", vm_event_name(PGLAZYFREE), > + memcg_events(memcg, PGLAZYFREE)); > + pr_buf(&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)); > + pr_buf(&buf, "%s %lu\n", vm_event_name(THP_FAULT_ALLOC), > + memcg_events(memcg, THP_FAULT_ALLOC)); > + pr_buf(&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.35.2
diff --git a/mm/memcontrol.c b/mm/memcontrol.c index 36e9f38c91..4cb0b7bc1c 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -61,7 +61,7 @@ #include <linux/file.h> #include <linux/tracehook.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> @@ -1436,13 +1436,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. @@ -1459,49 +1455,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); + pr_buf(&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); + pr_buf(&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)); + pr_buf(&buf, "%s %lu\n", vm_event_name(PGFAULT), + memcg_events(memcg, PGFAULT)); + pr_buf(&buf, "%s %lu\n", vm_event_name(PGMAJFAULT), + memcg_events(memcg, PGMAJFAULT)); + pr_buf(&buf, "%s %lu\n", vm_event_name(PGREFILL), + memcg_events(memcg, PGREFILL)); + pr_buf(&buf, "pgscan %lu\n", + memcg_events(memcg, PGSCAN_KSWAPD) + + memcg_events(memcg, PGSCAN_DIRECT)); + pr_buf(&buf, "pgsteal %lu\n", + memcg_events(memcg, PGSTEAL_KSWAPD) + + memcg_events(memcg, PGSTEAL_DIRECT)); + pr_buf(&buf, "%s %lu\n", vm_event_name(PGACTIVATE), + memcg_events(memcg, PGACTIVATE)); + pr_buf(&buf, "%s %lu\n", vm_event_name(PGDEACTIVATE), + memcg_events(memcg, PGDEACTIVATE)); + pr_buf(&buf, "%s %lu\n", vm_event_name(PGLAZYFREE), + memcg_events(memcg, PGLAZYFREE)); + pr_buf(&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)); + pr_buf(&buf, "%s %lu\n", vm_event_name(THP_FAULT_ALLOC), + memcg_events(memcg, THP_FAULT_ALLOC)); + pr_buf(&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(-)