Re: [PATCH v4 24/34] mm/memcontrol.c: Convert to printbuf

[Michal Hocko]

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@xxxxxxxxx>

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

-- 
Michal Hocko
SUSE Labs