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

[Lucas De Marchi]

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 at 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 at 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



>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.

thanks
Lucas De Marchi