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

[Tvrtko Ursulin]

On 19/04/2024 17:35, Lucas De Marchi wrote:
> 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 thought you said it worked. And I thought I tried and saw it working. 
Bummer..

> I can try playing with the patches to bring find_kv() back.

If you don't see any readability savings by the end of the series then 
don't bother.

Regards,

Tvrtko