[PATCH 2/2] tracing: Keep pid and comm[] in the same structure

[David Laight <david.laight.linux@gmail.com>]

Rather than have two separate dynamic arrays on the end of struct
saved_commandlines_buffer have a single dynamic array where each
entry contains the pid and associated task->comm[].
This simplifies the initialisation and lookup.

Don't bother trying to initialise the pid field no a non-zero value,
it only matters in the tracing_saved_cmdlines_seq_ops code.
Allocate entry [0] first so that the tracing_saved_cmdlines_seq_ops
code can just index the array with the file offset.

The code now uses the correct size when determining the page 'order'
to free the structure. The smaller size will always give the same
'order'.

Signed-off-by: David Laight <david.laight.linux@gmail.com>
---

Is there any reason why this code uses alloc_pages() rather
than vmalloc()?
map_pid_to_cmdline[] is 64k*sizeof(int) so the whole structure
expands to 512k with about 64k/20 (about 3200) pid entries even
though the default is 128.
AFAICT there is only one copy of the data - so it could be static.
Perhaps with pointers to map_pid_cmdline[] and (after this patch)
pid_comm[], both of which could be separately resized.

I also noticed that map_pid_to_cmdline[] contains indexes into
pid_comm[], restricting these to 16bits would half the data area.

 kernel/trace/trace_sched_switch.c | 97 +++++++++++++------------------
 1 file changed, 39 insertions(+), 58 deletions(-)

diff --git a/kernel/trace/trace_sched_switch.c b/kernel/trace/trace_sched_switch.c
index 972883643097..5e7c8cf444b8 100644
--- a/kernel/trace/trace_sched_switch.c
+++ b/kernel/trace/trace_sched_switch.c
@@ -167,27 +167,21 @@ static size_t tgid_map_max;
  * where interrupt is disabled.
  */
 static arch_spinlock_t trace_cmdline_lock = __ARCH_SPIN_LOCK_UNLOCKED;
+struct pid_comm {
+	pid_t pid;
+	struct trace_comm comm;
+};
 struct saved_cmdlines_buffer {
 	unsigned map_pid_to_cmdline[PID_MAX_DEFAULT+1];
-	unsigned *map_cmdline_to_pid;
 	unsigned cmdline_num;
 	int cmdline_idx;
-	struct trace_comm saved_cmdlines[];
+	struct pid_comm pid_comm[];
 };
 static struct saved_cmdlines_buffer *savedcmd;
 
-/* Holds the size of a cmdline and pid element */
-#define SAVED_CMDLINE_MAP_ELEMENT_SIZE(s)			\
-	(sizeof(struct trace_comm) + sizeof((s)->map_cmdline_to_pid[0]))
-
-static inline struct trace_comm *get_saved_cmdlines(int idx)
-{
-	return &savedcmd->saved_cmdlines[idx];
-}
-
-static inline void set_cmdline(int idx, const struct task_struct *tsk)
+static inline void set_cmdline(struct pid_comm *pid_comm, const struct task_struct *tsk)
 {
-	struct trace_comm *comm = get_saved_cmdlines(idx);
+	struct trace_comm *comm = &pid_comm->comm;
 
 	BUILD_BUG_ON(sizeof(comm->comm) > sizeof(tsk->comm));
 
@@ -212,7 +206,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 * SAVED_CMDLINE_MAP_ELEMENT_SIZE(s);
+	orig_size = sizeof(*s) + val * sizeof(s->pid_comm[0]);
 	order = get_order(orig_size);
 	size = 1 << (order + PAGE_SHIFT);
 	page = alloc_pages(GFP_KERNEL, order);
@@ -224,16 +218,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)) / SAVED_CMDLINE_MAP_ELEMENT_SIZE(s);
+	val = (size - sizeof(*s)) / sizeof(s->pid_comm[0]);
 	s->cmdline_num = val;
 
-	/* Place map_cmdline_to_pid array right after saved_cmdlines */
-	s->map_cmdline_to_pid = (unsigned *)&s->saved_cmdlines[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 s;
 }
@@ -247,6 +236,7 @@ int trace_create_savedcmd(void)
 
 int trace_save_cmdline(struct task_struct *tsk)
 {
+	struct pid_comm *pid_comm;
 	unsigned tpid, idx;
 
 	/* treat recording of idle task as a success */
@@ -272,14 +262,16 @@ int trace_save_cmdline(struct task_struct *tsk)
 
 	idx = savedcmd->map_pid_to_cmdline[tpid];
 	if (idx == NO_CMDLINE_MAP) {
-		idx = (savedcmd->cmdline_idx + 1) % savedcmd->cmdline_num;
-
+		idx = savedcmd->cmdline_idx;
 		savedcmd->map_pid_to_cmdline[tpid] = idx;
+		if (++idx >= savedcmd->cmdline_num)
+			idx = 0;
 		savedcmd->cmdline_idx = idx;
 	}
 
-	savedcmd->map_cmdline_to_pid[idx] = tsk->pid;
-	set_cmdline(idx, tsk);
+	pid_comm = savedcmd->pid_comm + idx;
+	pid_comm->pid = tsk->pid;
+	set_cmdline(pid_comm, tsk);
 
 	arch_spin_unlock(&trace_cmdline_lock);
 
@@ -288,8 +280,8 @@ int trace_save_cmdline(struct task_struct *tsk)
 
 static void __trace_find_cmdline(int pid, struct trace_comm *comm)
 {
+	struct pid_comm *pid_comm;
 	unsigned map;
-	int tpid;
 
 	if (!pid) {
 		strcpy(comm->comm, "<idle>");
@@ -301,12 +293,11 @@ static void __trace_find_cmdline(int pid, struct trace_comm *comm)
 		return;
 	}
 
-	tpid = pid & (PID_MAX_DEFAULT - 1);
-	map = savedcmd->map_pid_to_cmdline[tpid];
+	map = savedcmd->map_pid_to_cmdline[pid & (PID_MAX_DEFAULT - 1)];
 	if (map != NO_CMDLINE_MAP) {
-		tpid = savedcmd->map_cmdline_to_pid[map];
-		if (tpid == pid) {
-			*comm = *get_saved_cmdlines(map);
+		pid_comm = savedcmd->pid_comm + map;
+		if (pid_comm->pid == pid) {
+			*comm = pid_comm->comm;;
 			return;
 		}
 	}
@@ -521,42 +512,34 @@ const struct file_operations tracing_saved_tgids_fops = {
 	.release	= seq_release,
 };
 
-static void *saved_cmdlines_next(struct seq_file *m, void *v, loff_t *pos)
+static struct pid_comm *saved_cmdlines_entry(loff_t off)
 {
-	unsigned int *ptr = v;
+	struct pid_comm *pid_comm;
 
-	if (*pos || m->count)
-		ptr++;
-
-	(*pos)++;
+	if (off >= savedcmd->cmdline_num)
+		return NULL;
 
-	for (; ptr < &savedcmd->map_cmdline_to_pid[savedcmd->cmdline_num];
-	     ptr++) {
-		if (*ptr == -1 || *ptr == NO_CMDLINE_MAP)
-			continue;
+	/* Entries are used in sequence and never freed */
+	pid_comm = &savedcmd->pid_comm[off];
+	return pid_comm->pid ? pid_comm : NULL;
+}
 
-		return ptr;
-	}
+static void *saved_cmdlines_next(struct seq_file *m, void *v, loff_t *pos)
+{
+	loff_t off = *pos;
 
-	return NULL;
+	v = saved_cmdlines_entry(off);
+	if (v)
+		*pos = off + 1;
+	return v;
 }
 
 static void *saved_cmdlines_start(struct seq_file *m, loff_t *pos)
 {
-	void *v;
-	loff_t l = 0;
-
 	preempt_disable();
 	arch_spin_lock(&trace_cmdline_lock);
 
-	v = &savedcmd->map_cmdline_to_pid[0];
-	while (l <= *pos) {
-		v = saved_cmdlines_next(m, v, &l);
-		if (!v)
-			return NULL;
-	}
-
-	return v;
+	return saved_cmdlines_entry(*pos);
 }
 
 static void saved_cmdlines_stop(struct seq_file *m, void *v)
@@ -567,11 +550,9 @@ static void saved_cmdlines_stop(struct seq_file *m, void *v)
 
 static int saved_cmdlines_show(struct seq_file *m, void *v)
 {
-	struct trace_comm buf;
-	unsigned int *pid = v;
+	struct pid_comm *ptr = v;
 
-	__trace_find_cmdline(*pid, &buf);
-	seq_printf(m, "%d %s\n", *pid, buf.comm);
+	seq_printf(m, "%d %s\n", ptr->pid, ptr->comm.comm);
 	return 0;
 }
 
-- 
2.39.5