[PATCH i-g-t 01/12] lib/igt_drm_fdinfo: Stop passing key twice
Lucas De Marchi
lucas.demarchi at intel.com
Fri Apr 19 16:35:46 UTC 2024
On Fri, Apr 19, 2024 at 08:42:50AM GMT, Tvrtko Ursulin wrote:
>
>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 at 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?
if it's a function, it won't be handled as a string literal anymore and
may be a real strlen() call. The compiler may or may not detect that.
>
>>+
>> 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).
the ignore_space() is later removed (my bad on the patch split here
since I only noticed later when converting to strtoull())
>
>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))) {
with the downside that then there will be a real strlen() on the key.
I can try playing with the patches to bring find_kv() back.
Lucas De Marchi
> /* 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);
>> }
More information about the igt-dev
mailing list