Re: [PATCH i-g-t 01/12] lib/igt_drm_fdinfo: Stop passing key twice

[Tvrtko Ursulin <tursulin@ursulin.net>]

On 05/04/2024 07:00, Lucas De Marchi wrote:
> Change strstartswith() so it also returns the length, which then can be
> used inside the branch when it matches. A good compiler shall optimize
> out the strlen call since the argument is a constant literal.
> 
> With this, the find_kv() is not needed anymore and the difference with
> regard the other branches can be summarized with a new ignore_space()
> helper and the fact it matches the entire key by appending the ':'. The
> helper is added on top of the file so it can be reused later.
> 
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> ---
>   lib/igt_drm_fdinfo.c | 87 +++++++++++++++++++++++---------------------
>   1 file changed, 45 insertions(+), 42 deletions(-)
> 
> diff --git a/lib/igt_drm_fdinfo.c b/lib/igt_drm_fdinfo.c
> index 18dbb5d0b..bfc946f24 100644
> --- a/lib/igt_drm_fdinfo.c
> +++ b/lib/igt_drm_fdinfo.c
> @@ -53,6 +53,14 @@ static size_t read_fdinfo(char *buf, const size_t sz, int at, const char *name)
>   	return count > 0 ? count : 0;
>   }
>   
> +static const char *ignore_space(const char *s)
> +{
> +	for (; *s && isspace(*s); s++)
> +		;
> +
> +	return s;
> +}
> +
>   static int parse_engine(char *line, struct drm_client_fdinfo *info,
>   			size_t prefix_len,
>   			const char **name_map, unsigned int map_entries,
> @@ -104,23 +112,6 @@ static int parse_engine(char *line, struct drm_client_fdinfo *info,
>   	return found;
>   }
>   
> -static const char *find_kv(const char *buf, const char *key, size_t keylen)
> -{
> -	const char *p;
> -
> -	if (strncmp(buf, key, keylen))
> -		return NULL;
> -
> -	p = buf + keylen;
> -	if (*p != ':')
> -		return NULL;
> -
> -	for (p++; *p && isspace(*p); p++)
> -		;
> -
> -	return *p ? p : NULL;
> -}
> -
>   static int parse_region(char *line, struct drm_client_fdinfo *info,
>   			size_t prefix_len,
>   			const char **region_map, unsigned int region_entries,
> @@ -205,6 +196,11 @@ out:
>   		}							\
>   	} while (0)
>   
> +#define strstartswith(a, b, plen__) ({					\
> +		*plen__ = strlen(b);					\
> +		strncmp(a, b, *plen__) == 0;				\
> +})

Did it have to be a macro for optimization to work?

> +
>   unsigned int
>   __igt_parse_drm_fdinfo(int dir, const char *fd, struct drm_client_fdinfo *info,
>   		       const char **name_map, unsigned int map_entries,
> @@ -222,31 +218,39 @@ __igt_parse_drm_fdinfo(int dir, const char *fd, struct drm_client_fdinfo *info,
>   
>   	while ((l = strtok_r(_buf, "\n", &ctx))) {
>   		uint64_t val = 0;
> +		size_t keylen;
>   		const char *v;
>   		int idx;
>   
>   		_buf = NULL;
>   
> -		if ((v = find_kv(l, "drm-driver", strlen("drm-driver")))) {
> -			strncpy(info->driver, v, sizeof(info->driver) - 1);
> -			good++;
> -		}  else if ((v = find_kv(l, "drm-client-id",
> -					 strlen("drm-client-id")))) {
> -			info->id = atol(v);
> -			good++;
> -		} else if ((v = find_kv(l, "drm-pdev", strlen("drm-pdev")))) {
> -			/* optional */
> +		if (strstartswith(l, "drm-driver:", &keylen)) {
> +			v = l + keylen;
> +			v = ignore_space(v);
> +			if (*v) {
> +				strncpy(info->driver, v, sizeof(info->driver) - 1);
> +				good++;
> +			}
> +		}  else if (strstartswith(l, "drm-client-id:", &keylen)) {
> +			v = l + keylen;
> +			v = ignore_space(v);
> +			if (*v) {
> +				info->id = atol(v);
> +				good++;
> +			}
> +		} else if (strstartswith(l, "drm-pdev:", &keylen)) {
> +			v = l + keylen;
> +			v = ignore_space(v);

Removing find_kv looks like a slight negative, given how now all 
branches have to repeat v =  + keylen; v = ignore_space(v).

I wonder if it would work by extending find_kv to allow prefix matching 
mode, something like this (only two branches show to illustrate the modes):

...
		} else if ((v = find_kv(l, "drm-pdev", MATCH_FULL, &keylen))) {
			/* optional */
			strncpy(info->pdev, v, sizeof(info->pdev) - 1);
		} else if (find_kv(l, "drm-engine-capacity-", MATCH_PREFIX)) {
			idx = parse_engine(l, info, keylen,
					   name_map, map_entries, &val);
			if (idx >= 0) {
				info->capacity[idx] = val;
				num_capacity++;
			}
...

?

Regards,

Tvrtko

>   			strncpy(info->pdev, v, sizeof(info->pdev) - 1);
> -		} else if (!strncmp(l, "drm-engine-capacity-", 20)) {
> -			idx = parse_engine(l, info,
> -					   strlen("drm-engine-capacity-"),
> +		} else if (strstartswith(l, "drm-engine-capacity-", &keylen)) {
> +			idx = parse_engine(l, info, keylen,
>   					   name_map, map_entries, &val);
>   			if (idx >= 0) {
>   				info->capacity[idx] = val;
>   				num_capacity++;
>   			}
> -		} else if (!strncmp(l, "drm-engine-", 11)) {
> -			idx = parse_engine(l, info, strlen("drm-engine-"),
> +		} else if (strstartswith(l, "drm-engine-", &keylen)) {
> +			idx = parse_engine(l, info, keylen,
>   					   name_map, map_entries, &val);
>   			if (idx >= 0) {
>   				if (!info->capacity[idx])
> @@ -256,25 +260,24 @@ __igt_parse_drm_fdinfo(int dir, const char *fd, struct drm_client_fdinfo *info,
>   				if (idx > info->last_engine_index)
>   					info->last_engine_index = idx;
>   			}
> -		} else if (!strncmp(l, "drm-total-", 10)) {
> -			idx = parse_region(l, info, strlen("drm-total-"),
> +		} else if (strstartswith(l, "drm-total-", &keylen)) {
> +			idx = parse_region(l, info, keylen,
>   					   region_map, region_entries, &val);
>   			UPDATE_REGION(idx, total, val);
> -		} else if (!strncmp(l, "drm-shared-", 11)) {
> -			idx = parse_region(l, info, strlen("drm-shared-"),
> +		} else if (strstartswith(l, "drm-shared-", &keylen)) {
> +			idx = parse_region(l, info, keylen,
>   					   region_map, region_entries, &val);
>   			UPDATE_REGION(idx, shared, val);
> -
> -		} else if (!strncmp(l, "drm-resident-", 13)) {
> -			idx = parse_region(l, info, strlen("drm-resident-"),
> +		} else if (strstartswith(l, "drm-resident-", &keylen)) {
> +			idx = parse_region(l, info, keylen,
>   					   region_map, region_entries, &val);
>   			UPDATE_REGION(idx, resident, val);
> -		} else if (!strncmp(l, "drm-purgeable-", 14)) {
> -			idx = parse_region(l, info, strlen("drm-purgeable-"),
> +		} else if (strstartswith(l, "drm-purgeable-", &keylen)) {
> +			idx = parse_region(l, info, keylen,
>   					   region_map, region_entries, &val);
>   			UPDATE_REGION(idx, purgeable, val);
> -		} else if (!strncmp(l, "drm-active-", 11)) {
> -			idx = parse_region(l, info, strlen("drm-active-"),
> +		} else if (strstartswith(l, "drm-active-", &keylen)) {
> +			idx = parse_region(l, info, keylen,
>   					   region_map, region_entries, &val);
>   			UPDATE_REGION(idx, active, val);
>   		}