[PATCH] lib/igt_drm_clients: lazy stat process

[Tvrtko Ursulin]

On 01/05/2024 18:38, Lucas De Marchi wrote:
> Only try to get task data like pid and name when that is required.
> Typically most of the processes in a system don't have a drm fd open.
> So only open and use /proc/<pid>/stat when it's needed.
> 
> For the data below, system is running with just a single drm client:
> 
> Before:
>   Performance counter stats for './build/tools/gputop -d 0.1 -n 10':
> 
>              507.19 msec task-clock                       #    0.334 CPUs utilized
>                  29      context-switches                 #   57.178 /sec
>                   4      cpu-migrations                   #    7.887 /sec
>                  75      page-faults                      #  147.874 /sec
>         (...)
>         1.518253637 seconds time elapsed
>         0.007989000 seconds user
>         0.503351000 seconds sys
> 
> After:
>   Performance counter stats for './build/tools/gputop -d 0.1 -n 10':
> 
>              384.01 msec task-clock                       #    0.275 CPUs utilized
>                  17      context-switches                 #   44.270 /sec
>                   1      cpu-migrations                   #    2.604 /sec
>                  74      page-faults                      #  192.705 /sec
>         (...)
>         1.397640800 seconds time elapsed
>         0.008106000 seconds user
>         0.385048000 seconds sys
> 
> Looking only at the execution time of igt_drm_clients_scan() over 10
> runs.
> Before: 47138890 nsec
> After:  33978388 nsec
> 
> So in both methods it shows a ~25% speedup. It is expected that in a
> system with more clients this won't be as high.
> 
> Signed-off-by: Lucas De Marchi <lucas.demarchi at intel.com>
> ---
> 
> Extracted as is from https://patchwork.freedesktop.org/series/132059/ as
> not really related.
> 
>   lib/igt_drm_clients.c | 50 +++++++++++++++++++++----------------------
>   1 file changed, 25 insertions(+), 25 deletions(-)
> 
> diff --git a/lib/igt_drm_clients.c b/lib/igt_drm_clients.c
> index 40d494441..ab0c2cec2 100644
> --- a/lib/igt_drm_clients.c
> +++ b/lib/igt_drm_clients.c
> @@ -368,26 +368,33 @@ static size_t readat2buf(int at, const char *name, char *buf, const size_t sz)
>   	}
>   }
>   
> -static bool get_task_name(const char *buffer, char *out, unsigned long sz)
> +static void get_task_data(int pid_dir, unsigned int *pid, char *task, size_t tasksz)
>   {
> -	char *s = index(buffer, '(');
> -	char *e = rindex(buffer, ')');
> -	unsigned int len;
> +	char buf[4096];
> +	char *s, *e;
> +	size_t len;
>   
> +	if (!readat2buf(pid_dir, "stat", buf, sizeof(buf)))
> +		return;
> +
> +	s = strchr(buf, '(');
> +	e = strchr(s, ')');
>   	if (!s || !e)
> -		return false;
> -	assert(e >= s);
> +		return;
>   
>   	len = e - ++s;
> -	if(!len || (len + 1) >= sz)
> -		return false;
> +	if (!len)
> +		return;
>   
> -	strncpy(out, s, len);
> -	out[len] = 0;
> +	if (len + 1 > tasksz)
> +		len = tasksz - 1;
>   
> -	return true;
> +	strncpy(task, s, len);
> +	task[len] = 0;
> +	*pid = atoi(buf);
>   }
>   
> +
>   static bool is_drm_fd(int fd_dir, const char *name, unsigned int *minor)
>   {
>   	struct stat stat;
> @@ -481,13 +488,11 @@ igt_drm_clients_scan(struct igt_drm_clients *clients,
>   		return clients;
>   
>   	while ((proc_dent = readdir(proc_dir)) != NULL) {
> -		unsigned int client_pid, minor = 0;
> +		unsigned int client_pid = 0, minor = 0;
>   		int pid_dir = -1, fd_dir = -1;
>   		struct dirent *fdinfo_dent;
>   		char client_name[64] = { };
>   		DIR *fdinfo_dir = NULL;
> -		char buf[4096];
> -		size_t count;
>   
>   		if (proc_dent->d_type != DT_DIR)
>   			continue;
> @@ -499,17 +504,6 @@ igt_drm_clients_scan(struct igt_drm_clients *clients,
>   		if (pid_dir < 0)
>   			continue;
>   
> -		count = readat2buf(pid_dir, "stat", buf, sizeof(buf));
> -		if (!count)
> -			goto next;
> -
> -		client_pid = atoi(buf);
> -		if (!client_pid)
> -			goto next;
> -
> -		if (!get_task_name(buf, client_name, sizeof(client_name)))
> -			goto next;
> -
>   		fd_dir = openat(pid_dir, "fd", O_DIRECTORY | O_RDONLY);
>   		if (fd_dir < 0)
>   			goto next;
> @@ -542,6 +536,12 @@ igt_drm_clients_scan(struct igt_drm_clients *clients,
>   						 minor, info.id))
>   				continue; /* Skip duplicate fds. */
>   
> +			if (!client_pid) {
> +				get_task_data(pid_dir, &client_pid, client_name,
> +					      sizeof(client_name));
> +				assert(client_pid > 0);
> +			}
> +
>   			c = igt_drm_clients_find(clients, IGT_DRM_CLIENT_PROBE,
>   						 minor, info.id);
>   			if (!c)

Nice improvement and well spotted!

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin at igalia.com>

Regards,

Tvrtko