Re: [PATCH i-g-t 1/2] lib/igt_drm_fdinfo: Use longest match first

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 01/02/2024 14:09, Lucas De Marchi wrote:
> On Thu, Feb 01, 2024 at 01:43:21PM +0100, Kamil Konieczny wrote:
>> Hi Lucas,
>> On 2024-01-31 at 16:46:24 -0800, Lucas De Marchi wrote:
>>> Instead of checking twice the match, just use longest match first.
>>
>> Nice catch, LGTM.
>>
>> Reviewed-by: Kamil Konieczny <kamil.konieczny@linux.intel.com>

+1 looks like that was a confused change I did when adding capacity.

Going forward feel free to copy me when changing this code, given I 
mostly wrote it after all..

>>
>>>
>>> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
>>> ---
>>>  lib/igt_drm_fdinfo.c | 19 +++++++++----------
>>>  1 file changed, 9 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/lib/igt_drm_fdinfo.c b/lib/igt_drm_fdinfo.c
>>> index 222ccbfb1..5c0ccf624 100644
>>> --- a/lib/igt_drm_fdinfo.c
>>> +++ b/lib/igt_drm_fdinfo.c
>>> @@ -239,8 +239,15 @@ __igt_parse_drm_fdinfo(int dir, const char *fd, 
>>> struct drm_client_fdinfo *info,
>>>                       strlen("drm-client-id")))) {
>>>              info->id = atol(v);
>>>              good++;
>>> -        } else if (!strncmp(l, "drm-engine-", 11) &&
>>> -               strncmp(l, "drm-engine-capacity-", 20)) {
>>> +        } else if (!strncmp(l, "drm-engine-capacity-", 20)) {
>>
>> One more thing to eventually fix is removing "20" which is exact
>> len of string, with something like
>>
>> bool str_begins(char *src, char *pattern)
>>
>> One source of errors is counting letters in strings or changing
>> string itself without changing corresponding len. This can be
>> done in separate patch.
> 
> I had a patch for something similar. I usually use a strstartswith()
> that's commonly used in other projects and also matches the python name.
> 
> However I decided to drop that for now. I may have a bigger refactor in
> this function soon reworking parse_engine() and other functions: Worse
> than manually counting the chars for the strncmp() is to call strlen()
> in the same string just in the line below that to feed to parse_engine()
> :-/. As I said, still undecided what to change here and if it's worth
> it.

AFAIR hardcoded strlens were because strangely const strings fail to be 
evaluated at compile time and when I tested it and strlen was showing in 
the profile. Given how gputop sucked with CPU usage quite badly 
(relative to top) I was motivated to improve it.

The "top-level" strncmp checks are the more expensive ones because they 
all run for all lines in a given fdinfo file, while the inside the 
branch strlen runs fewer times.

Nevertheless some cleanup of this function would probably be good, just 
while doing so keep in mind how bad gputop looks compared to top in the 
profiles, not to make it much worse.

Regards,

Tvrtko

> 
> 
> Lucas De Marchi
> 
>>
>> Regards,
>> Kamil
>>
>>> +            idx = parse_engine(l, info,
>>> +                       strlen("drm-engine-capacity-"),
>>> +                       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-"),
>>>                         name_map, map_entries, &val);
>>>              if (idx >= 0) {
>>> @@ -251,14 +258,6 @@ __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-engine-capacity-", 20)) {
>>> -            idx = parse_engine(l, info,
>>> -                       strlen("drm-engine-capacity-"),
>>> -                       name_map, map_entries, &val);
>>> -            if (idx >= 0) {
>>> -                info->capacity[idx] = val;
>>> -                num_capacity++;
>>> -            }
>>>          } else if (!strncmp(l, "drm-total-", 10)) {
>>>              idx = parse_region(l, info, strlen("drm-total-"),
>>>                         region_map, region_entries, &val);
>>> -- 
>>> 2.43.0
>>>