Re: [PATCH] tracing: Have saved_cmdlines arrays all in one allocation

[Steven Rostedt]

On Mon, 12 Feb 2024 15:39:03 -0800
Tim Chen <tim.c.chen@xxxxxxxxxxxxxxx> wrote:

> > diff --git a/kernel/trace/trace_sched_switch.c b/kernel/trace/trace_sched_switch.c
> > index e4fbcc3bede5..210c74dcd016 100644
> > --- a/kernel/trace/trace_sched_switch.c
> > +++ b/kernel/trace/trace_sched_switch.c
> > @@ -201,7 +201,7 @@ static struct saved_cmdlines_buffer *allocate_cmdlines_buffer(unsigned int val)
> >  	int order;
> >  
> >  	/* Figure out how much is needed to hold the given number of cmdlines */
> > -	orig_size = sizeof(*s) + val * TASK_COMM_LEN;
> > +	orig_size = sizeof(*s) + val * (TASK_COMM_LEN + sizeof(int));  
> 
> Strictly speaking, *map_cmdline_to_pid is unsigned int so it is more consistent
> to use sizeof(unsigned) in line above.  But I'm nitpicking and I'm fine to
> leave it as is.

I was thinking about making that into a macro as it is used in two places.

/* Holds the size of a cmdline and pid element */
#define SAVED_CMDLINE_MAP_ELEMENT_SIZE(s)		\
	(TASK_COMM_LEN + sizeof((s)->map_cmdline_to_pid[0]))

	orig_size = sizeof(*s) + val * SAVED_CMDLINE_MAP_ELEMENT_SIZE(s);

> 
> >  	order = get_order(orig_size);
> >  	size = 1 << (order + PAGE_SHIFT);
> >  	page = alloc_pages(GFP_KERNEL, order);
> > @@ -212,16 +212,11 @@ static struct saved_cmdlines_buffer *allocate_cmdlines_buffer(unsigned int val)
> >  	memset(s, 0, sizeof(*s));
> >  
> >  	/* Round up to actual allocation */
> > -	val = (size - sizeof(*s)) / TASK_COMM_LEN;
> > +	val = (size - sizeof(*s)) / (TASK_COMM_LEN + sizeof(int));

	val = (size - sizeof(*s)) / SAVED_CMDLINE_MAP_ELEMENT_SIZE(s);

-- Steve

> >  	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) {
> > -		free_saved_cmdlines_buffer(s);
> > -		return NULL;
> > -	}
> > +	/* Place map_cmdline_to_pid array right after saved_cmdlines */
> > +	s->map_cmdline_to_pid = (unsigned *)&s->saved_cmdlines[val * TASK_COMM_LEN];
> >  
> >  	s->cmdline_idx = 0;
> >  	memset(&s->map_pid_to_cmdline, NO_CMDLINE_MAP,