[v2] tracing: Fix wasted memory in saved_cmdlines logic

  From: "Steven Rostedt (Google)" <rostedt@goodmis.org>

While looking at improving the saved_cmdlines cache I found a huge amount
of wasted memory that should be used for the cmdlines.

The tracing data saves pids during the trace. At sched switch, if a trace
occurred, it will save the comm of the task that did the trace. This is
saved in a "cache" that maps pids to comms and exposed to user space via
the /sys/kernel/tracing/saved_cmdlines file. Currently it only caches by
default 128 comms.

The structure that uses this creates an array to store the pids using
PID_MAX_DEFAULT (which is usually set to 32768). This causes the structure
to be of the size of 131104 bytes on 64 bit machines.

In hex: 131104 = 0x20020, and since the kernel allocates generic memory in
powers of two, the kernel would allocate 0x40000 or 262144 bytes to store
this structure. That leaves 131040 bytes of wasted space.

Worse, the structure points to an allocated array to store the comm names,
which is 16 bytes times the amount of names to save (currently 128), which
is 2048 bytes. Instead of allocating a separate array, make the structure
end with a variable length string and use the extra space for that.

This is similar to a recommendation that Linus had made about eventfs_inode names:

  https://lore.kernel.org/all/20240130190355.11486-5-torvalds@linux-foundation.org/

Instead of allocating a separate string array to hold the saved comms,
have the structure end with: char saved_cmdlines[]; and round up to the
next power of two over sizeof(struct saved_cmdline_buffers) + num_cmdlines * TASK_COMM_LEN
It will use this extra space for the saved_cmdline portion.

Now, instead of saving only 128 comms by default, by using this wasted
space at the end of the structure it can save over 8000 comms and even
saves space by removing the need for allocating the other array.

Cc: stable@vger.kernel.org
Fixes: 939c7a4f04fcd ("tracing: Introduce saved_cmdlines_size file")
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
Changes since v1: https://lore.kernel.org/linux-trace-kernel/20240208105328.7e73f71d@rorschach.local.home

- Added back error check of s->map_cmdline_to_pid allocation failure

 kernel/trace/trace.c | 75 ++++++++++++++++++++++----------------------
 1 file changed, 37 insertions(+), 38 deletions(-)
  

On Fri, 9 Feb 2024 06:36:22 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
> 
> While looking at improving the saved_cmdlines cache I found a huge amount
> of wasted memory that should be used for the cmdlines.
> 
> The tracing data saves pids during the trace. At sched switch, if a trace
> occurred, it will save the comm of the task that did the trace. This is
> saved in a "cache" that maps pids to comms and exposed to user space via
> the /sys/kernel/tracing/saved_cmdlines file. Currently it only caches by
> default 128 comms.
> 
> The structure that uses this creates an array to store the pids using
> PID_MAX_DEFAULT (which is usually set to 32768). This causes the structure
> to be of the size of 131104 bytes on 64 bit machines.
> 
> In hex: 131104 = 0x20020, and since the kernel allocates generic memory in
> powers of two, the kernel would allocate 0x40000 or 262144 bytes to store
> this structure. That leaves 131040 bytes of wasted space.
> 
> Worse, the structure points to an allocated array to store the comm names,
> which is 16 bytes times the amount of names to save (currently 128), which
> is 2048 bytes. Instead of allocating a separate array, make the structure
> end with a variable length string and use the extra space for that.
> 
> This is similar to a recommendation that Linus had made about eventfs_inode names:
> 
>   https://lore.kernel.org/all/20240130190355.11486-5-torvalds@linux-foundation.org/
> 
> Instead of allocating a separate string array to hold the saved comms,
> have the structure end with: char saved_cmdlines[]; and round up to the
> next power of two over sizeof(struct saved_cmdline_buffers) + num_cmdlines * TASK_COMM_LEN
> It will use this extra space for the saved_cmdline portion.
> 
> Now, instead of saving only 128 comms by default, by using this wasted
> space at the end of the structure it can save over 8000 comms and even
> saves space by removing the need for allocating the other array.

This looks good to me. So this will allocate the saved_cmdlines in page-size
array instead of the power of two.

Acked-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>

Thank you,


> 
> Cc: stable@vger.kernel.org
> Fixes: 939c7a4f04fcd ("tracing: Introduce saved_cmdlines_size file")
> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
> ---
> Changes since v1: https://lore.kernel.org/linux-trace-kernel/20240208105328.7e73f71d@rorschach.local.home
> 
> - Added back error check of s->map_cmdline_to_pid allocation failure
> 
>  kernel/trace/trace.c | 75 ++++++++++++++++++++++----------------------
>  1 file changed, 37 insertions(+), 38 deletions(-)
> 
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index 2a7c6fd934e9..9ff8a439d674 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -2320,7 +2320,7 @@ struct saved_cmdlines_buffer {
>  	unsigned *map_cmdline_to_pid;
>  	unsigned cmdline_num;
>  	int cmdline_idx;
> -	char *saved_cmdlines;
> +	char saved_cmdlines[];
>  };
>  static struct saved_cmdlines_buffer *savedcmd;
>  
> @@ -2334,47 +2334,58 @@ static inline void set_cmdline(int idx, const char *cmdline)
>  	strncpy(get_saved_cmdlines(idx), cmdline, TASK_COMM_LEN);
>  }
>  
> -static int allocate_cmdlines_buffer(unsigned int val,
> -				    struct saved_cmdlines_buffer *s)
> +static void free_saved_cmdlines_buffer(struct saved_cmdlines_buffer *s)
> +{
> +	int order = get_order(sizeof(*s) + s->cmdline_num * TASK_COMM_LEN);
> +
> +	kfree(s->map_cmdline_to_pid);
> +	free_pages((unsigned long)s, order);
> +}
> +
> +static struct saved_cmdlines_buffer *allocate_cmdlines_buffer(unsigned int val)
>  {
> +	struct saved_cmdlines_buffer *s;
> +	struct page *page;
> +	int orig_size, size;
> +	int order;
> +
> +	/* Figure out how much is needed to hold the given number of cmdlines */
> +	orig_size = sizeof(*s) + val * TASK_COMM_LEN;
> +	order = get_order(orig_size);
> +	size = 1 << (order + PAGE_SHIFT);
> +	page = alloc_pages(GFP_KERNEL, order);
> +	if (!page)
> +		return NULL;
> +
> +	s = page_address(page);
> +	memset(s, 0, sizeof(*s));
> +
> +	/* Round up to actual allocation */
> +	val = (size - sizeof(*s)) / TASK_COMM_LEN;
> +	s->cmdline_num = val;
> +
>  	s->map_cmdline_to_pid = kmalloc_array(val,
>  					      sizeof(*s->map_cmdline_to_pid),
>  					      GFP_KERNEL);
> -	if (!s->map_cmdline_to_pid)
> -		return -ENOMEM;
> -
> -	s->saved_cmdlines = kmalloc_array(TASK_COMM_LEN, val, GFP_KERNEL);
> -	if (!s->saved_cmdlines) {
> -		kfree(s->map_cmdline_to_pid);
> -		return -ENOMEM;
> +	if (!s->map_cmdline_to_pid) {
> +		free_saved_cmdlines_buffer(s);
> +		return NULL;
>  	}
>  
>  	s->cmdline_idx = 0;
> -	s->cmdline_num = val;
>  	memset(&s->map_pid_to_cmdline, NO_CMDLINE_MAP,
>  	       sizeof(s->map_pid_to_cmdline));
>  	memset(s->map_cmdline_to_pid, NO_CMDLINE_MAP,
>  	       val * sizeof(*s->map_cmdline_to_pid));
>  
> -	return 0;
> +	return s;
>  }
>  
>  static int trace_create_savedcmd(void)
>  {
> -	int ret;
> -
> -	savedcmd = kmalloc(sizeof(*savedcmd), GFP_KERNEL);
> -	if (!savedcmd)
> -		return -ENOMEM;
> -
> -	ret = allocate_cmdlines_buffer(SAVED_CMDLINES_DEFAULT, savedcmd);
> -	if (ret < 0) {
> -		kfree(savedcmd);
> -		savedcmd = NULL;
> -		return -ENOMEM;
> -	}
> +	savedcmd = allocate_cmdlines_buffer(SAVED_CMDLINES_DEFAULT);
>  
> -	return 0;
> +	return savedcmd ? 0 : -ENOMEM;
>  }
>  
>  int is_tracing_stopped(void)
> @@ -6056,26 +6067,14 @@ tracing_saved_cmdlines_size_read(struct file *filp, char __user *ubuf,
>  	return simple_read_from_buffer(ubuf, cnt, ppos, buf, r);
>  }
>  
> -static void free_saved_cmdlines_buffer(struct saved_cmdlines_buffer *s)
> -{
> -	kfree(s->saved_cmdlines);
> -	kfree(s->map_cmdline_to_pid);
> -	kfree(s);
> -}
> -
>  static int tracing_resize_saved_cmdlines(unsigned int val)
>  {
>  	struct saved_cmdlines_buffer *s, *savedcmd_temp;
>  
> -	s = kmalloc(sizeof(*s), GFP_KERNEL);
> +	s = allocate_cmdlines_buffer(val);
>  	if (!s)
>  		return -ENOMEM;
>  
> -	if (allocate_cmdlines_buffer(val, s) < 0) {
> -		kfree(s);
> -		return -ENOMEM;
> -	}
> -
>  	preempt_disable();
>  	arch_spin_lock(&trace_cmdline_lock);
>  	savedcmd_temp = savedcmd;
> -- 
> 2.43.0
>
  

On Tue, 13 Feb 2024 00:40:38 +0900
Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:

> > Now, instead of saving only 128 comms by default, by using this wasted
> > space at the end of the structure it can save over 8000 comms and even
> > saves space by removing the need for allocating the other array.  
> 
> This looks good to me. So this will allocate the saved_cmdlines in page-size
> array instead of the power of two.

Once the size is greater than page-size, it still is a power of two ;-)

But I think you meant that it will be a reminder of extra data, and not a
power of two. Looking at the code, it didn't need to be a power of two as
the saved names were just in a ring buffer anyway. The recording of a PID
must be a power of two, as that is masked.

Basically we have three arrays. One for the PIDS that hold the index into the
COMM array. The COMM array really has no limitation. The PID array is
PID_MAX_DEFAULT. But PIDs can be more than PID_MAX_DEFAULT as that's just
the "default" setting. User space can increase it. I need to add a comment
to how this works, as there are three arrays:

 unsigned map_pid_to_cmdline[PID_MAX_DEFAULT + 1];

 /* The next two are allocated based on cmdline_num size */
 unsigned map_cmdline_to_pid[cmdline_num];
 char saved_cmdlines[cmdline_num][TASK_COMM_LEN];

map_pid_to_cmdline[task->pid & (PID_MAX_DEFAULT - 1)] = index into the other two arrays;

 [ Although I'm not sure I need that "+1" in the array. This code is ancient
   and I need to re-examine it. ]

To get back to the PID, you need the map_cmdline_to_pid[idx] which contains
the full PID. Really, that array is only needed if pid max is greater than
PID_MAX_DEFAULT (which I find is the case for most distros now).

To assign a new comm, we basically do:

	idx = map_pid_to_cmdline[pid & (PID_MAX_DEFAULT - 1)];
	if (idx == -1) {
		idx = // get next location in COMM ring buffer
		map_pid_to_cmdline[pid & (PID_MAX_DEFAULT - 1)] = idx;
	}
	map_cmdline_to_pid[idx] = pid;
	strcpy(saved_cmdlines[idx], task->comm);

And Mathieu pinged me about the non-commented use of that NO_CMDLINE_MAP
which is just -1. I have it as INT_MAX, but really maybe it should be -1U,
and add a comment around the memset().

> 
> Acked-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>

Thanks, but Linus already pulled this. But I have some patches to clean
this up a bit more.

-- Steve