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

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

On 07/02/2024 17:50, Lucas De Marchi wrote:
> On Wed, Feb 07, 2024 at 10:32:24AM +0000, Tvrtko Ursulin wrote:
>>
>> 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..
> 
> will do, sorry for missing that for this series.
> 
> 
>>
>>>>
>>>>>
>>>>> 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 
> 
> humn... strlen("foo") should always be optimized out.
> Testing with gcc 12.3 here, but it's something I always looked at:
> 
>      $ cat /tmp/a.c
>      #include <string.h>
> 
>      int foo(void)
>      {
>          return strlen("foo");
>      }
>      $ gcc -c -g -O0 -o /tmp/a.o /tmp/a.c
>      $ objdump -d /tmp/a.o
>      /tmp/a.o:     file format elf64-x86-64
> 
> 
>      Disassembly of section .text:
> 
>      0000000000000000 <foo>:
>         0:   f3 0f 1e fa             endbr64
>         4:   55                      push   %rbp
>         5:   48 89 e5                mov    %rsp,%rbp
>         8:   b8 03 00 00 00          mov    $0x3,%eax
>         d:   5d                      pop    %rbp
>         e:   c3                      ret
> 
>

Yeah I don't know, I was perplexed too. I tried it now and it works as 
expected. Maybe some weirdness on my local build machine at the time.

>> 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.
> 
> 
> will keep that in mind, but I'm not sure I will be able to work on this
> soon.

No rush, I only mentioned this since you announced you have reworks in 
your sights.

Regards,

Tvrtko