Received-SPF: pass (google.com: domain of linux-kernel+bounces-62563-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Mon, 12 Feb 2024 19:13:36 -0500
From: Steven Rostedt <rostedt@goodmis.org>
To: Tim Chen <tim.c.chen@linux.intel.com>
Cc: LKML <linux-kernel@vger.kernel.org>, Linux Trace Kernel
 <linux-trace-kernel@vger.kernel.org>, Masami Hiramatsu
 <mhiramat@kernel.org>, Mathieu Desnoyers <mathieu.desnoyers@efficios.com>,
 Mark Rutland <mark.rutland@arm.com>, Vincent Donnefort
 <vdonnefort@google.com>, Sven Schnelle <svens@linux.ibm.com>, Mete Durlu
 <meted@linux.ibm.com>
Subject: Re: [PATCH] tracing: Have saved_cmdlines arrays all in one
 allocation
Message-ID: <20240212191336.5c502f78@gandalf.local.home>
In-Reply-To: <ef94259e139e4579ee27db99975dc4cf397e2a06.camel@linux.intel.com>
References: <20240212180941.379c419b@gandalf.local.home>
	<ef94259e139e4579ee27db99975dc4cf397e2a06.camel@linux.intel.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

On Mon, 12 Feb 2024 15:39:03 -0800
Tim Chen <tim.c.chen@linux.intel.com> 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,