diff mbox series

[i-g-t,2/3] intel_gpu_top: Aggregate clients by PID by default

Message ID 20210210093756.61424-2-tvrtko.ursulin@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series [i-g-t,1/3] intel_gpu_top: Wrap interactive header | expand

Commit Message

Tvrtko Ursulin Feb. 10, 2021, 9:37 a.m. UTC
From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Implement a default view where clients are aggregated by their PID.

Toggled by pressing 'H' similar to top(1).

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 man/intel_gpu_top.rst |   1 +
 tools/intel_gpu_top.c | 109 +++++++++++++++++++++++++++++++++++++-----
 2 files changed, 97 insertions(+), 13 deletions(-)

Comments

Chris Wilson Feb. 10, 2021, 10:35 a.m. UTC | #1
Quoting Tvrtko Ursulin (2021-02-10 09:37:55)
> +static struct clients *aggregated_clients(struct clients *clients)
> +{
> +       struct client *ac, *c, *cp = NULL;
> +       struct clients *aggregated;
> +       int tmp, num = 0;
> +
> +       /* Sort by pid first to make it easy to aggregate while walking. */
> +       sort_clients(clients, client_pid_cmp);

You could eliminate this tiny bit of duplication by always calling
aggregated_clients() and returning here for !aggregate_pids.

> +       aggregated = calloc(1, sizeof(*clients));
> +       assert(aggregated);
> +
> +       ac = calloc(clients->num_clients, sizeof(*c));
> +       assert(ac);
> +
> +       aggregated->num_classes = clients->num_classes;
> +       aggregated->class = clients->class;
> +       aggregated->client = ac;
> +
> +       for_each_client(clients, c, tmp) {
> +               unsigned int i;
> +
> +               if (c->status == FREE)
> +                       break;
> +
> +               assert(c->status == ALIVE);
> +
> +               if ((cp && c->pid != cp->pid) || !cp) {
> +                       ac = &aggregated->client[num];
> +
> +                       /* New pid. */
> +                       ac->clients = aggregated;
> +                       ac->status = ALIVE;
> +                       ac->id = ++num;
> +                       ac->pid = c->pid;
> +                       strcpy(ac->name, c->name);
> +                       strcpy(ac->print_name, c->print_name);
> +                       ac->engines = c->engines;
> +                       ac->val = calloc(clients->num_classes,
> +                                        sizeof(ac->val[0]));
> +                       assert(ac->val);
> +                       ac->samples = 1;
> +               }
> +
> +               cp = c;
> +
> +               if (c->samples < 2)
> +                       continue;
> +
> +               ac->samples = 2; /* All what matters for display. */
> +               ac->total_runtime += c->total_runtime;
> +               ac->last_runtime += c->last_runtime;
> +
> +               for (i = 0; i < clients->num_classes; i++)
> +                       ac->val[i] += c->val[i];
> +       }
> +
> +       aggregated->num_clients = num;
> +       aggregated->active_clients = num;
> +
> +       return sort_clients(aggregated, client_cmp);
>  }

Ok, that works very well. Hmm. The sort order does seem a little jumpy
though. May I suggest ac->id = -c->pid; instead of num;
-Chris
Tvrtko Ursulin Feb. 10, 2021, 10:55 a.m. UTC | #2
On 10/02/2021 10:35, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2021-02-10 09:37:55)
>> +static struct clients *aggregated_clients(struct clients *clients)
>> +{
>> +       struct client *ac, *c, *cp = NULL;
>> +       struct clients *aggregated;
>> +       int tmp, num = 0;
>> +
>> +       /* Sort by pid first to make it easy to aggregate while walking. */
>> +       sort_clients(clients, client_pid_cmp);
> 
> You could eliminate this tiny bit of duplication by always calling
> aggregated_clients() and returning here for !aggregate_pids.

Okay, I did something like that.

>> +       aggregated = calloc(1, sizeof(*clients));
>> +       assert(aggregated);
>> +
>> +       ac = calloc(clients->num_clients, sizeof(*c));
>> +       assert(ac);
>> +
>> +       aggregated->num_classes = clients->num_classes;
>> +       aggregated->class = clients->class;
>> +       aggregated->client = ac;
>> +
>> +       for_each_client(clients, c, tmp) {
>> +               unsigned int i;
>> +
>> +               if (c->status == FREE)
>> +                       break;
>> +
>> +               assert(c->status == ALIVE);
>> +
>> +               if ((cp && c->pid != cp->pid) || !cp) {
>> +                       ac = &aggregated->client[num];
>> +
>> +                       /* New pid. */
>> +                       ac->clients = aggregated;
>> +                       ac->status = ALIVE;
>> +                       ac->id = ++num;
>> +                       ac->pid = c->pid;
>> +                       strcpy(ac->name, c->name);
>> +                       strcpy(ac->print_name, c->print_name);
>> +                       ac->engines = c->engines;
>> +                       ac->val = calloc(clients->num_classes,
>> +                                        sizeof(ac->val[0]));
>> +                       assert(ac->val);
>> +                       ac->samples = 1;
>> +               }
>> +
>> +               cp = c;
>> +
>> +               if (c->samples < 2)
>> +                       continue;
>> +
>> +               ac->samples = 2; /* All what matters for display. */
>> +               ac->total_runtime += c->total_runtime;
>> +               ac->last_runtime += c->last_runtime;
>> +
>> +               for (i = 0; i < clients->num_classes; i++)
>> +                       ac->val[i] += c->val[i];
>> +       }
>> +
>> +       aggregated->num_clients = num;
>> +       aggregated->active_clients = num;
>> +
>> +       return sort_clients(aggregated, client_cmp);
>>   }
> 
> Ok, that works very well. Hmm. The sort order does seem a little jumpy
> though. May I suggest ac->id = -c->pid; instead of num;

Done it although I thought 1st pass being sort by pid already, num as id 
would follow a stable order. I guess your point was inversion to 
preserve order when cycling sort modes?

Regards,

Tvrtko
Chris Wilson Feb. 10, 2021, 11:03 a.m. UTC | #3
Quoting Tvrtko Ursulin (2021-02-10 10:55:44)
> 
> On 10/02/2021 10:35, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2021-02-10 09:37:55)
> > Ok, that works very well. Hmm. The sort order does seem a little jumpy
> > though. May I suggest ac->id = -c->pid; instead of num;
> 
> Done it although I thought 1st pass being sort by pid already, num as id 
> would follow a stable order. I guess your point was inversion to 
> preserve order when cycling sort modes?

I thought that makes more sense for 'aggregate-by-pid', that it should
either be in ascending/descending pid order for idle.

It just felt very jumpy; it may have been tiny amount of %busy, but I
suspected it may have been slightly unstable sorting with changing
clients.
-Chris
diff mbox series

Patch

diff --git a/man/intel_gpu_top.rst b/man/intel_gpu_top.rst
index b145d85c0440..20658e291db0 100644
--- a/man/intel_gpu_top.rst
+++ b/man/intel_gpu_top.rst
@@ -58,6 +58,7 @@  Supported keys:
     'n'    Toggle display of numeric client busyness overlay.
     's'    Toggle between sort modes (runtime, total runtime, pid, client id).
     'i'    Toggle display of clients which used no GPU time.
+    'H'    Toggle between per PID aggregation and individual clients.
 
 DEVICE SELECTION
 ================
diff --git a/tools/intel_gpu_top.c b/tools/intel_gpu_top.c
index 63ef77056341..018e28a66c10 100644
--- a/tools/intel_gpu_top.c
+++ b/tools/intel_gpu_top.c
@@ -979,17 +979,18 @@  static int client_pid_cmp(const void *_a, const void *_b)
 
 static int (*client_cmp)(const void *, const void *) = client_last_cmp;
 
-static void sort_clients(struct clients *clients)
+static struct clients *sort_clients(struct clients *clients,
+				    int (*cmp)(const void *, const void *))
 {
 	unsigned int active, free;
 	struct client *c;
 	int tmp;
 
 	if (!clients)
-		return;
+		return clients;
 
 	qsort(clients->client, clients->num_clients, sizeof(*clients->client),
-	      client_cmp);
+	      cmp);
 
 	/* Trim excessive array space. */
 	active = 0;
@@ -1011,9 +1012,76 @@  static void sort_clients(struct clients *clients)
 						  sizeof(*c));
 		}
 	}
+
+	return clients;
+}
+
+static struct clients *aggregated_clients(struct clients *clients)
+{
+	struct client *ac, *c, *cp = NULL;
+	struct clients *aggregated;
+	int tmp, num = 0;
+
+	/* Sort by pid first to make it easy to aggregate while walking. */
+	sort_clients(clients, client_pid_cmp);
+
+	aggregated = calloc(1, sizeof(*clients));
+	assert(aggregated);
+
+	ac = calloc(clients->num_clients, sizeof(*c));
+	assert(ac);
+
+	aggregated->num_classes = clients->num_classes;
+	aggregated->class = clients->class;
+	aggregated->client = ac;
+
+	for_each_client(clients, c, tmp) {
+		unsigned int i;
+
+		if (c->status == FREE)
+			break;
+
+		assert(c->status == ALIVE);
+
+		if ((cp && c->pid != cp->pid) || !cp) {
+			ac = &aggregated->client[num];
+
+			/* New pid. */
+			ac->clients = aggregated;
+			ac->status = ALIVE;
+			ac->id = ++num;
+			ac->pid = c->pid;
+			strcpy(ac->name, c->name);
+			strcpy(ac->print_name, c->print_name);
+			ac->engines = c->engines;
+			ac->val = calloc(clients->num_classes,
+					 sizeof(ac->val[0]));
+			assert(ac->val);
+			ac->samples = 1;
+		}
+
+		cp = c;
+
+		if (c->samples < 2)
+			continue;
+
+		ac->samples = 2; /* All what matters for display. */
+		ac->total_runtime += c->total_runtime;
+		ac->last_runtime += c->last_runtime;
+
+		for (i = 0; i < clients->num_classes; i++)
+			ac->val[i] += c->val[i];
+	}
+
+	aggregated->num_clients = num;
+	aggregated->active_clients = num;
+
+	return sort_clients(aggregated, client_cmp);
 }
 
-static void scan_clients(struct clients *clients)
+static bool aggregate_pids = true;
+
+static struct clients *scan_clients(struct clients *clients)
 {
 	struct dirent *dent;
 	struct client *c;
@@ -1022,7 +1090,7 @@  static void scan_clients(struct clients *clients)
 	DIR *d;
 
 	if (!clients)
-		return;
+		return clients;
 
 	for_each_client(clients, c, tmp) {
 		assert(c->status != PROBE);
@@ -1034,7 +1102,7 @@  static void scan_clients(struct clients *clients)
 
 	d = opendir(clients->sysfs_root);
 	if (!d)
-		return;
+		return clients;
 
 	while ((dent = readdir(d)) != NULL) {
 		char name[24], pid[24];
@@ -1077,7 +1145,10 @@  static void scan_clients(struct clients *clients)
 			break;
 	}
 
-	sort_clients(clients);
+	if (aggregate_pids)
+		return aggregated_clients(clients);
+	else
+		return sort_clients(clients, client_cmp);
 }
 
 static const char *bars[] = { " ", "▏", "▎", "▍", "▌", "▋", "▊", "▉", "█" };
@@ -2227,6 +2298,13 @@  static void process_stdin(unsigned int timeout_us)
 		case 's':
 			select_client_sort();
 			break;
+		case 'H':
+			aggregate_pids ^= true;
+			if (aggregate_pids)
+				header_msg = "Aggregating clients.";
+			else
+				header_msg = "Showing individual clients.";
+			break;
 		};
 	}
 }
@@ -2378,6 +2456,7 @@  int main(int argc, char **argv)
 	codename = igt_device_get_pretty_name(&card, false);
 
 	while (!stop_top) {
+		struct clients *disp_clients;
 		bool consumed = false;
 		int j, lines = 0;
 		struct winsize ws;
@@ -2400,7 +2479,7 @@  int main(int argc, char **argv)
 		pmu_sample(engines);
 		t = (double)(engines->ts.cur - engines->ts.prev) / 1e9;
 
-		scan_clients(clients);
+		disp_clients = scan_clients(clients);
 
 		if (stop_top)
 			break;
@@ -2416,14 +2495,14 @@  int main(int argc, char **argv)
 
 			lines = print_engines(engines, t, lines, con_w, con_h);
 
-			if (clients) {
+			if (disp_clients) {
 				int class_w;
 
-				lines = print_clients_header(clients, lines,
+				lines = print_clients_header(disp_clients, lines,
 							     con_w, con_h,
 							     &class_w);
 
-				for_each_client(clients, c, j) {
+				for_each_client(disp_clients, c, j) {
 					assert(c->status != PROBE);
 					if (c->status != ALIVE)
 						break; /* Active clients are first in the array. */
@@ -2437,8 +2516,9 @@  int main(int argc, char **argv)
 							     &class_w);
 				}
 
-				lines = print_clients_footer(clients, t, lines,
-							     con_w, con_h);
+				lines = print_clients_footer(disp_clients, t,
+							     lines, con_w,
+							     con_h);
 			}
 
 			pops->close_struct();
@@ -2447,6 +2527,9 @@  int main(int argc, char **argv)
 		if (stop_top)
 			break;
 
+		if (disp_clients != clients)
+			free(disp_clients);
+
 		if (output_mode == INTERACTIVE)
 			process_stdin(period_us);
 		else