[PATCH] tools/perf/tests/base_probe: Fix check for the count of existing probes in test_adding_kernel

[PATCH] tools/perf/tests/base_probe: Fix check for the count of existing probes in test_adding_kernel

[Athira Rajeev]

perftool-testsuite_probe fails in test_adding_kernel as below:
	Regexp not found: "probe:inode_permission_11"
	-- [ FAIL ] -- perf_probe :: test_adding_kernel :: force-adding probes ::
	second probe adding (with force) (output regexp parsing)
	event syntax error: 'probe:inode_permission_11'
	  \___ unknown tracepoint

	Error:  File /sys/kernel/tracing//events/probe/inode_permission_11
	not found.
	Hint:   Perhaps this kernel misses some CONFIG_ setting to
	enable this feature?.

The test does the following:
1) Adds a probe point first using :
    $CMD_PERF probe --add $TEST_PROBE
2) Then tries to add same probe again without —force
and expects it to fail. Next tries to add same probe again
with —force. In this case, perf probe succeeds and adds
the probe with a suffix number. Example:

 ./perf probe --add inode_permission
 Added new event:
  probe:inode_permission (on inode_permission)

 ./perf probe --add inode_permission --force
 Added new event:
  probe:inode_permission_1 (on inode_permission)

  ./perf probe --add inode_permission --force
 Added new event:
  probe:inode_permission_2 (on inode_permission)

Each time, suffix is added to existing probe name.
To get the suffix number, test cases uses :
NO_OF_PROBES=`$CMD_PERF probe -l | wc -l`

This will work if there is no other probe existing
in the system. If there are any other probes other than
kernel probes or inode_permission, ( example: any probe),
"perf probe -l" will include count for other probes too.

Example, in the system where this failed, already some
probes were default added. So count became 10
  ./perf probe -l | wc -l
  10

So to be specific for "inode_permission", restrict the
probe count check to that probe point alone using :
NO_OF_PROBES=`$CMD_PERF probe -l $TEST_PROBE| wc -l`

Similarly while removing the probe using "probe --del *",
( removing all probes ), check uses:

 ../common/check_all_lines_matched.pl "Removed event: probe:$TEST_PROBE"

But if there are other probes in the system, the log will
contain reference to other existing probe too. Hence change
usage of check_all_lines_matched.pl to check_all_patterns_found.pl
This will make sure expecting string comes in the result

Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
---
 tools/perf/tests/shell/base_probe/test_adding_kernel.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/perf/tests/shell/base_probe/test_adding_kernel.sh b/tools/perf/tests/shell/base_probe/test_adding_kernel.sh
index d541ffd44a93..f8b5f096d0d7 100755
--- a/tools/perf/tests/shell/base_probe/test_adding_kernel.sh
+++ b/tools/perf/tests/shell/base_probe/test_adding_kernel.sh
@@ -169,7 +169,7 @@ print_results $PERF_EXIT_CODE $CHECK_EXIT_CODE "force-adding probes :: second pr
 (( TEST_RESULT += $? ))
 
 # adding existing probe with '--force' should pass
-NO_OF_PROBES=`$CMD_PERF probe -l | wc -l`
+NO_OF_PROBES=`$CMD_PERF probe -l $TEST_PROBE| wc -l`
 $CMD_PERF probe --force --add $TEST_PROBE 2> $LOGS_DIR/adding_kernel_forceadd_03.err
 PERF_EXIT_CODE=$?
 
@@ -205,7 +205,7 @@ print_results $PERF_EXIT_CODE $CHECK_EXIT_CODE "using doubled probe"
 $CMD_PERF probe --del \* 2> $LOGS_DIR/adding_kernel_removing_wildcard.err
 PERF_EXIT_CODE=$?
 
-../common/check_all_lines_matched.pl "Removed event: probe:$TEST_PROBE" "Removed event: probe:${TEST_PROBE}_1" < $LOGS_DIR/adding_kernel_removing_wildcard.err
+../common/check_all_patterns_found.pl "Removed event: probe:$TEST_PROBE" "Removed event: probe:${TEST_PROBE}_1" < $LOGS_DIR/adding_kernel_removing_wildcard.err
 CHECK_EXIT_CODE=$?
 
 print_results $PERF_EXIT_CODE $CHECK_EXIT_CODE "removing multiple probes"
-- 
2.43.5

Re: [PATCH] tools/perf/tests/base_probe: Fix check for the count of existing probes in test_adding_kernel

[Athira Rajeev]

> On 14 Oct 2024, at 9:09 PM, Athira Rajeev <atrajeev@linux.vnet.ibm.com> wrote:
> 
> perftool-testsuite_probe fails in test_adding_kernel as below:
> Regexp not found: "probe:inode_permission_11"
> -- [ FAIL ] -- perf_probe :: test_adding_kernel :: force-adding probes ::
> second probe adding (with force) (output regexp parsing)
> event syntax error: 'probe:inode_permission_11'
>  \___ unknown tracepoint
> 
> Error:  File /sys/kernel/tracing//events/probe/inode_permission_11
> not found.
> Hint:   Perhaps this kernel misses some CONFIG_ setting to
> enable this feature?.
> 
> The test does the following:
> 1) Adds a probe point first using :
>    $CMD_PERF probe --add $TEST_PROBE
> 2) Then tries to add same probe again without —force
> and expects it to fail. Next tries to add same probe again
> with —force. In this case, perf probe succeeds and adds
> the probe with a suffix number. Example:
> 
> ./perf probe --add inode_permission
> Added new event:
>  probe:inode_permission (on inode_permission)
> 
> ./perf probe --add inode_permission --force
> Added new event:
>  probe:inode_permission_1 (on inode_permission)
> 
>  ./perf probe --add inode_permission --force
> Added new event:
>  probe:inode_permission_2 (on inode_permission)
> 
> Each time, suffix is added to existing probe name.
> To get the suffix number, test cases uses :
> NO_OF_PROBES=`$CMD_PERF probe -l | wc -l`
> 
> This will work if there is no other probe existing
> in the system. If there are any other probes other than
> kernel probes or inode_permission, ( example: any probe),
> "perf probe -l" will include count for other probes too.
> 
> Example, in the system where this failed, already some
> probes were default added. So count became 10
>  ./perf probe -l | wc -l
>  10
> 
> So to be specific for "inode_permission", restrict the
> probe count check to that probe point alone using :
> NO_OF_PROBES=`$CMD_PERF probe -l $TEST_PROBE| wc -l`
> 
> Similarly while removing the probe using "probe --del *",
> ( removing all probes ), check uses:
> 
> ../common/check_all_lines_matched.pl "Removed event: probe:$TEST_PROBE"
> 
> But if there are other probes in the system, the log will
> contain reference to other existing probe too. Hence change
> usage of check_all_lines_matched.pl to check_all_patterns_found.pl
> This will make sure expecting string comes in the result
> 
> Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>

Adding Veronika in the mail. Sorry missed to add initially. Please share review comments on this fix.

Thanks
Athira
> ---
> tools/perf/tests/shell/base_probe/test_adding_kernel.sh | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/perf/tests/shell/base_probe/test_adding_kernel.sh b/tools/perf/tests/shell/base_probe/test_adding_kernel.sh
> index d541ffd44a93..f8b5f096d0d7 100755
> --- a/tools/perf/tests/shell/base_probe/test_adding_kernel.sh
> +++ b/tools/perf/tests/shell/base_probe/test_adding_kernel.sh
> @@ -169,7 +169,7 @@ print_results $PERF_EXIT_CODE $CHECK_EXIT_CODE "force-adding probes :: second pr
> (( TEST_RESULT += $? ))
> 
> # adding existing probe with '--force' should pass
> -NO_OF_PROBES=`$CMD_PERF probe -l | wc -l`
> +NO_OF_PROBES=`$CMD_PERF probe -l $TEST_PROBE| wc -l`
> $CMD_PERF probe --force --add $TEST_PROBE 2> $LOGS_DIR/adding_kernel_forceadd_03.err
> PERF_EXIT_CODE=$?
> 
> @@ -205,7 +205,7 @@ print_results $PERF_EXIT_CODE $CHECK_EXIT_CODE "using doubled probe"
> $CMD_PERF probe --del \* 2> $LOGS_DIR/adding_kernel_removing_wildcard.err
> PERF_EXIT_CODE=$?
> 
> -../common/check_all_lines_matched.pl "Removed event: probe:$TEST_PROBE" "Removed event: probe:${TEST_PROBE}_1" < $LOGS_DIR/adding_kernel_removing_wildcard.err
> +../common/check_all_patterns_found.pl "Removed event: probe:$TEST_PROBE" "Removed event: probe:${TEST_PROBE}_1" < $LOGS_DIR/adding_kernel_removing_wildcard.err
> CHECK_EXIT_CODE=$?
> 
> print_results $PERF_EXIT_CODE $CHECK_EXIT_CODE "removing multiple probes"
> -- 
> 2.43.5
> 
> 

Re: [PATCH] tools/perf/tests/base_probe: Fix check for the count of existing probes in test_adding_kernel

[Michael Petlan]

[-- Attachment #1: Type: text/plain, Size: 4655 bytes --]

On Mon, 14 Oct 2024, Athira Rajeev wrote:
> perftool-testsuite_probe fails in test_adding_kernel as below:
> 	Regexp not found: "probe:inode_permission_11"
> 	-- [ FAIL ] -- perf_probe :: test_adding_kernel :: force-adding probes ::
> 	second probe adding (with force) (output regexp parsing)
> 	event syntax error: 'probe:inode_permission_11'
> 	  \___ unknown tracepoint
> 
> 	Error:  File /sys/kernel/tracing//events/probe/inode_permission_11
> 	not found.
> 	Hint:   Perhaps this kernel misses some CONFIG_ setting to
> 	enable this feature?.
> 
> The test does the following:
> 1) Adds a probe point first using :
>     $CMD_PERF probe --add $TEST_PROBE
> 2) Then tries to add same probe again without —force
> and expects it to fail. Next tries to add same probe again
> with —force. In this case, perf probe succeeds and adds
> the probe with a suffix number. Example:
> 
>  ./perf probe --add inode_permission
>  Added new event:
>   probe:inode_permission (on inode_permission)
> 
>  ./perf probe --add inode_permission --force
>  Added new event:
>   probe:inode_permission_1 (on inode_permission)
> 
>   ./perf probe --add inode_permission --force
>  Added new event:
>   probe:inode_permission_2 (on inode_permission)
> 
> Each time, suffix is added to existing probe name.
> To get the suffix number, test cases uses :
> NO_OF_PROBES=`$CMD_PERF probe -l | wc -l`
> 
> This will work if there is no other probe existing
> in the system. If there are any other probes other than
> kernel probes or inode_permission, ( example: any probe),
> "perf probe -l" will include count for other probes too.

Hello.

When designing this test, I was relying on the fact that
there are no existing probes, because all should have been
removed at line 43 of the same test:

	40 ### basic probe adding
	41
	42 for opt in "" "-a" "--add"; do
-->	43 	   clear_all_probes
	44 	   $CMD_PERF probe $opt $TEST_PROBE 2> $LOGS_DIR/adding_kernel_add$opt.err
	45 	   PERF_EXIT_CODE=$?
	46
	47	../common/check_all_patterns_found.pl "Added new events?:" "probe:$TEST_PROBE" "on $TEST_PROBE" < $LOGS_DIR/adding_kernel_add$opt.err
	48 	CHECK_EXIT_CODE=$?
	49
	50	print_results $PERF_EXIT_CODE $CHECK_EXIT_CODE "adding probe $TEST_PROBE :: $opt"
	51 	(( TEST_RESULT += $? ))
	52 done
	53

I am wondering how it could happen that there were other
probes in the system?

Cheers,

Michael

> 
> Example, in the system where this failed, already some
> probes were default added. So count became 10
>   ./perf probe -l | wc -l
>   10
> 
> So to be specific for "inode_permission", restrict the
> probe count check to that probe point alone using :
> NO_OF_PROBES=`$CMD_PERF probe -l $TEST_PROBE| wc -l`
> 
> Similarly while removing the probe using "probe --del *",
> ( removing all probes ), check uses:
> 
>  ../common/check_all_lines_matched.pl "Removed event: probe:$TEST_PROBE"
> 
> But if there are other probes in the system, the log will
> contain reference to other existing probe too. Hence change
> usage of check_all_lines_matched.pl to check_all_patterns_found.pl
> This will make sure expecting string comes in the result
> 
> Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
> ---
>  tools/perf/tests/shell/base_probe/test_adding_kernel.sh | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/perf/tests/shell/base_probe/test_adding_kernel.sh b/tools/perf/tests/shell/base_probe/test_adding_kernel.sh
> index d541ffd44a93..f8b5f096d0d7 100755
> --- a/tools/perf/tests/shell/base_probe/test_adding_kernel.sh
> +++ b/tools/perf/tests/shell/base_probe/test_adding_kernel.sh
> @@ -169,7 +169,7 @@ print_results $PERF_EXIT_CODE $CHECK_EXIT_CODE "force-adding probes :: second pr
>  (( TEST_RESULT += $? ))
>  
>  # adding existing probe with '--force' should pass
> -NO_OF_PROBES=`$CMD_PERF probe -l | wc -l`
> +NO_OF_PROBES=`$CMD_PERF probe -l $TEST_PROBE| wc -l`
>  $CMD_PERF probe --force --add $TEST_PROBE 2> $LOGS_DIR/adding_kernel_forceadd_03.err
>  PERF_EXIT_CODE=$?
>  
> @@ -205,7 +205,7 @@ print_results $PERF_EXIT_CODE $CHECK_EXIT_CODE "using doubled probe"
>  $CMD_PERF probe --del \* 2> $LOGS_DIR/adding_kernel_removing_wildcard.err
>  PERF_EXIT_CODE=$?
>  
> -../common/check_all_lines_matched.pl "Removed event: probe:$TEST_PROBE" "Removed event: probe:${TEST_PROBE}_1" < $LOGS_DIR/adding_kernel_removing_wildcard.err
> +../common/check_all_patterns_found.pl "Removed event: probe:$TEST_PROBE" "Removed event: probe:${TEST_PROBE}_1" < $LOGS_DIR/adding_kernel_removing_wildcard.err
>  CHECK_EXIT_CODE=$?
>  
>  print_results $PERF_EXIT_CODE $CHECK_EXIT_CODE "removing multiple probes"
> -- 
> 2.43.5
> 
> 
> 

Re: [PATCH] tools/perf/tests/base_probe: Fix check for the count of existing probes in test_adding_kernel

[Athira Rajeev]

> On 17 Oct 2024, at 3:44 PM, Michael Petlan <mpetlan@redhat.com> wrote:
> 
> On Mon, 14 Oct 2024, Athira Rajeev wrote:
>> perftool-testsuite_probe fails in test_adding_kernel as below:
>> Regexp not found: "probe:inode_permission_11"
>> -- [ FAIL ] -- perf_probe :: test_adding_kernel :: force-adding probes ::
>> second probe adding (with force) (output regexp parsing)
>> event syntax error: 'probe:inode_permission_11'
>>   \___ unknown tracepoint
>> 
>> Error:  File /sys/kernel/tracing//events/probe/inode_permission_11
>> not found.
>> Hint:   Perhaps this kernel misses some CONFIG_ setting to
>> enable this feature?.
>> 
>> The test does the following:
>> 1) Adds a probe point first using :
>>    $CMD_PERF probe --add $TEST_PROBE
>> 2) Then tries to add same probe again without —force
>> and expects it to fail. Next tries to add same probe again
>> with —force. In this case, perf probe succeeds and adds
>> the probe with a suffix number. Example:
>> 
>> ./perf probe --add inode_permission
>> Added new event:
>>  probe:inode_permission (on inode_permission)
>> 
>> ./perf probe --add inode_permission --force
>> Added new event:
>>  probe:inode_permission_1 (on inode_permission)
>> 
>>  ./perf probe --add inode_permission --force
>> Added new event:
>>  probe:inode_permission_2 (on inode_permission)
>> 
>> Each time, suffix is added to existing probe name.
>> To get the suffix number, test cases uses :
>> NO_OF_PROBES=`$CMD_PERF probe -l | wc -l`
>> 
>> This will work if there is no other probe existing
>> in the system. If there are any other probes other than
>> kernel probes or inode_permission, ( example: any probe),
>> "perf probe -l" will include count for other probes too.
> 
> Hello.
> 
> When designing this test, I was relying on the fact that
> there are no existing probes, because all should have been
> removed at line 43 of the same test:
> 
> 40 ### basic probe adding
> 41
> 42 for opt in "" "-a" "--add"; do
> --> 43     clear_all_probes
> 44     $CMD_PERF probe $opt $TEST_PROBE 2> $LOGS_DIR/adding_kernel_add$opt.err
> 45     PERF_EXIT_CODE=$?
> 46
> 47 ../common/check_all_patterns_found.pl "Added new events?:" "probe:$TEST_PROBE" "on $TEST_PROBE" < $LOGS_DIR/adding_kernel_add$opt.err
> 48  CHECK_EXIT_CODE=$?
> 49
> 50 print_results $PERF_EXIT_CODE $CHECK_EXIT_CODE "adding probe $TEST_PROBE :: $opt"
> 51  (( TEST_RESULT += $? ))
> 52 done
> 53
> 
> I am wondering how it could happen that there were other
> probes in the system?

Hi Michael,

Sorry for the late response.

Yes, there are uprobes listed as part of “perf probe” in the environment where I saw the test needing this change. Sharing the result below from perf probe: 

# ./perf probe -l
  uprobes:p_uprobe_dns_events_osquery4026531841 (on getaddrinfo in XX)
  uprobes:p_uprobe_dns_events_osquery4026532336 (on 0x129a60 in XX)
  uprobes:p_uprobe_dns_events_osquery4026532344 (on 0x129a60 in XX)
  uprobes:p_uprobe_ebpf_compat_check_osquery (on __GI___backtrace in XX)
  uprobes:p_uprobe_sys_hook_osquery (on backtrace_symbols in XX)

These can’t be removed.

# ./perf probe -d uprobes:p_uprobe_dns_events_osquery4026531841
Removed event: uprobes:p_uprobe_dns_events_osquery4026531841
Failed to delete event: Device or resource busy
  Error: Failed to delete events.


Considering above scenario, patch here takes the probe count using:
NO_OF_PROBES=`$CMD_PERF probe -l $TEST_PROBE| wc -l`

Also similarly looks for TEST_PROBE in result log in case of probe —del as well

Any comments Michael ?

Thanks
Athira

> 
> Cheers,
> 
> Michael
> 
>> 
>> Example, in the system where this failed, already some
>> probes were default added. So count became 10
>>  ./perf probe -l | wc -l
>>  10
>> 
>> So to be specific for "inode_permission", restrict the
>> probe count check to that probe point alone using :
>> NO_OF_PROBES=`$CMD_PERF probe -l $TEST_PROBE| wc -l`
>> 
>> Similarly while removing the probe using "probe --del *",
>> ( removing all probes ), check uses:
>> 
>> ../common/check_all_lines_matched.pl "Removed event: probe:$TEST_PROBE"
>> 
>> But if there are other probes in the system, the log will
>> contain reference to other existing probe too. Hence change
>> usage of check_all_lines_matched.pl to check_all_patterns_found.pl
>> This will make sure expecting string comes in the result
>> 
>> Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
>> ---
>> tools/perf/tests/shell/base_probe/test_adding_kernel.sh | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>> 
>> diff --git a/tools/perf/tests/shell/base_probe/test_adding_kernel.sh b/tools/perf/tests/shell/base_probe/test_adding_kernel.sh
>> index d541ffd44a93..f8b5f096d0d7 100755
>> --- a/tools/perf/tests/shell/base_probe/test_adding_kernel.sh
>> +++ b/tools/perf/tests/shell/base_probe/test_adding_kernel.sh
>> @@ -169,7 +169,7 @@ print_results $PERF_EXIT_CODE $CHECK_EXIT_CODE "force-adding probes :: second pr
>> (( TEST_RESULT += $? ))
>> 
>> # adding existing probe with '--force' should pass
>> -NO_OF_PROBES=`$CMD_PERF probe -l | wc -l`
>> +NO_OF_PROBES=`$CMD_PERF probe -l $TEST_PROBE| wc -l`
>> $CMD_PERF probe --force --add $TEST_PROBE 2> $LOGS_DIR/adding_kernel_forceadd_03.err
>> PERF_EXIT_CODE=$?
>> 
>> @@ -205,7 +205,7 @@ print_results $PERF_EXIT_CODE $CHECK_EXIT_CODE "using doubled probe"
>> $CMD_PERF probe --del \* 2> $LOGS_DIR/adding_kernel_removing_wildcard.err
>> PERF_EXIT_CODE=$?
>> 
>> -../common/check_all_lines_matched.pl "Removed event: probe:$TEST_PROBE" "Removed event: probe:${TEST_PROBE}_1" < $LOGS_DIR/adding_kernel_removing_wildcard.err
>> +../common/check_all_patterns_found.pl "Removed event: probe:$TEST_PROBE" "Removed event: probe:${TEST_PROBE}_1" < $LOGS_DIR/adding_kernel_removing_wildcard.err
>> CHECK_EXIT_CODE=$?
>> 
>> print_results $PERF_EXIT_CODE $CHECK_EXIT_CODE "removing multiple probes"
>> -- 
>> 2.43.5

Re: [PATCH] tools/perf/tests/base_probe: Fix check for the count of existing probes in test_adding_kernel

[Michael Petlan]

[-- Attachment #1: Type: text/plain, Size: 4429 bytes --]

On Sun, 3 Nov 2024, Athira Rajeev wrote:
> > On 17 Oct 2024, at 3:44 PM, Michael Petlan <mpetlan@redhat.com> wrote:
> > 
> > On Mon, 14 Oct 2024, Athira Rajeev wrote:
[...]
> > 
> > I am wondering how it could happen that there were other
> > probes in the system?
> 
> Hi Michael,
>
Hello Athira.

> Sorry for the late response.
> 
> Yes, there are uprobes listed as part of “perf probe” in the environment where I saw the test needing this change. Sharing the result below from perf probe: 
> 
> # ./perf probe -l
>   uprobes:p_uprobe_dns_events_osquery4026531841 (on getaddrinfo in XX)
>   uprobes:p_uprobe_dns_events_osquery4026532336 (on 0x129a60 in XX)
>   uprobes:p_uprobe_dns_events_osquery4026532344 (on 0x129a60 in XX)
>   uprobes:p_uprobe_ebpf_compat_check_osquery (on __GI___backtrace in XX)
>   uprobes:p_uprobe_sys_hook_osquery (on backtrace_symbols in XX)
> 
> These can’t be removed.
> 
> # ./perf probe -d uprobes:p_uprobe_dns_events_osquery4026531841
> Removed event: uprobes:p_uprobe_dns_events_osquery4026531841
> Failed to delete event: Device or resource busy
>   Error: Failed to delete events.
>

Ah, this is interesting, I have never hit that. However, it makes sense,
if the resource is busy.

However, in that case it comes to my mind that in general, these tests
should not be run in any production environment, where one could rely on
some probes will exist, etc. In case some of the probes above was not
busy, it'd be probably cleaned up by the testcase, which might be unexpected
by the creator/user of the uprobes... Maybe we should get rid of the
probe cleaning for that cases, but I'd prefer to keep it.

> Considering above scenario, patch here takes the probe count using:
> NO_OF_PROBES=`$CMD_PERF probe -l $TEST_PROBE| wc -l`
> 
> Also similarly looks for TEST_PROBE in result log in case of probe —del as well
> 
> Any comments Michael ?
>
Yes, we probably should tweak it as you suggest.

Thanks,
Michael


> Thanks
> Athira
> 
> > 
> > Cheers,
> > 
> > Michael
> > 
> >> 
> >> Example, in the system where this failed, already some
> >> probes were default added. So count became 10
> >>  ./perf probe -l | wc -l
> >>  10
> >> 
> >> So to be specific for "inode_permission", restrict the
> >> probe count check to that probe point alone using :
> >> NO_OF_PROBES=`$CMD_PERF probe -l $TEST_PROBE| wc -l`
> >> 
> >> Similarly while removing the probe using "probe --del *",
> >> ( removing all probes ), check uses:
> >> 
> >> ../common/check_all_lines_matched.pl "Removed event: probe:$TEST_PROBE"
> >> 
> >> But if there are other probes in the system, the log will
> >> contain reference to other existing probe too. Hence change
> >> usage of check_all_lines_matched.pl to check_all_patterns_found.pl
> >> This will make sure expecting string comes in the result
> >> 
> >> Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
> >> ---
> >> tools/perf/tests/shell/base_probe/test_adding_kernel.sh | 4 ++--
> >> 1 file changed, 2 insertions(+), 2 deletions(-)
> >> 
> >> diff --git a/tools/perf/tests/shell/base_probe/test_adding_kernel.sh b/tools/perf/tests/shell/base_probe/test_adding_kernel.sh
> >> index d541ffd44a93..f8b5f096d0d7 100755
> >> --- a/tools/perf/tests/shell/base_probe/test_adding_kernel.sh
> >> +++ b/tools/perf/tests/shell/base_probe/test_adding_kernel.sh
> >> @@ -169,7 +169,7 @@ print_results $PERF_EXIT_CODE $CHECK_EXIT_CODE "force-adding probes :: second pr
> >> (( TEST_RESULT += $? ))
> >> 
> >> # adding existing probe with '--force' should pass
> >> -NO_OF_PROBES=`$CMD_PERF probe -l | wc -l`
> >> +NO_OF_PROBES=`$CMD_PERF probe -l $TEST_PROBE| wc -l`
> >> $CMD_PERF probe --force --add $TEST_PROBE 2> $LOGS_DIR/adding_kernel_forceadd_03.err
> >> PERF_EXIT_CODE=$?
> >> 
> >> @@ -205,7 +205,7 @@ print_results $PERF_EXIT_CODE $CHECK_EXIT_CODE "using doubled probe"
> >> $CMD_PERF probe --del \* 2> $LOGS_DIR/adding_kernel_removing_wildcard.err
> >> PERF_EXIT_CODE=$?
> >> 
> >> -../common/check_all_lines_matched.pl "Removed event: probe:$TEST_PROBE" "Removed event: probe:${TEST_PROBE}_1" < $LOGS_DIR/adding_kernel_removing_wildcard.err
> >> +../common/check_all_patterns_found.pl "Removed event: probe:$TEST_PROBE" "Removed event: probe:${TEST_PROBE}_1" < $LOGS_DIR/adding_kernel_removing_wildcard.err
> >> CHECK_EXIT_CODE=$?
> >> 
> >> print_results $PERF_EXIT_CODE $CHECK_EXIT_CODE "removing multiple probes"
> >> -- 
> >> 2.43.5
> 
> 
> 

Re: [PATCH] tools/perf/tests/base_probe: Fix check for the count of existing probes in test_adding_kernel

[Athira Rajeev]

> On 14 Nov 2024, at 3:35 PM, Michael Petlan <mpetlan@redhat.com> wrote:
> 
> On Sun, 3 Nov 2024, Athira Rajeev wrote:
>>> On 17 Oct 2024, at 3:44 PM, Michael Petlan <mpetlan@redhat.com> wrote:
>>> 
>>> On Mon, 14 Oct 2024, Athira Rajeev wrote:
> [...]
>>> 
>>> I am wondering how it could happen that there were other
>>> probes in the system?
>> 
>> Hi Michael,
>> 
> Hello Athira.
> 
>> Sorry for the late response.
>> 
>> Yes, there are uprobes listed as part of “perf probe” in the environment where I saw the test needing this change. Sharing the result below from perf probe: 
>> 
>> # ./perf probe -l
>>  uprobes:p_uprobe_dns_events_osquery4026531841 (on getaddrinfo in XX)
>>  uprobes:p_uprobe_dns_events_osquery4026532336 (on 0x129a60 in XX)
>>  uprobes:p_uprobe_dns_events_osquery4026532344 (on 0x129a60 in XX)
>>  uprobes:p_uprobe_ebpf_compat_check_osquery (on __GI___backtrace in XX)
>>  uprobes:p_uprobe_sys_hook_osquery (on backtrace_symbols in XX)
>> 
>> These can’t be removed.
>> 
>> # ./perf probe -d uprobes:p_uprobe_dns_events_osquery4026531841
>> Removed event: uprobes:p_uprobe_dns_events_osquery4026531841
>> Failed to delete event: Device or resource busy
>>  Error: Failed to delete events.
>> 
> 
> Ah, this is interesting, I have never hit that. However, it makes sense,
> if the resource is busy.
> 
> However, in that case it comes to my mind that in general, these tests
> should not be run in any production environment, where one could rely on
> some probes will exist, etc. In case some of the probes above was not
> busy, it'd be probably cleaned up by the testcase, which might be unexpected
> by the creator/user of the uprobes... Maybe we should get rid of the
> probe cleaning for that cases, but I'd prefer to keep it.
> 
>> Considering above scenario, patch here takes the probe count using:
>> NO_OF_PROBES=`$CMD_PERF probe -l $TEST_PROBE| wc -l`
>> 
>> Also similarly looks for TEST_PROBE in result log in case of probe —del as well
>> 
>> Any comments Michael ?
>> 
> Yes, we probably should tweak it as you suggest.

Hi,

Thanks for checking Michael.

If the patch looks good, can we please get this pulled in ?

Thanks
Athira
> 
> Thanks,
> Michael
> 
> 
>> Thanks
>> Athira
>> 
>>> 
>>> Cheers,
>>> 
>>> Michael
>>> 
>>>> 
>>>> Example, in the system where this failed, already some
>>>> probes were default added. So count became 10
>>>> ./perf probe -l | wc -l
>>>> 10
>>>> 
>>>> So to be specific for "inode_permission", restrict the
>>>> probe count check to that probe point alone using :
>>>> NO_OF_PROBES=`$CMD_PERF probe -l $TEST_PROBE| wc -l`
>>>> 
>>>> Similarly while removing the probe using "probe --del *",
>>>> ( removing all probes ), check uses:
>>>> 
>>>> ../common/check_all_lines_matched.pl "Removed event: probe:$TEST_PROBE"
>>>> 
>>>> But if there are other probes in the system, the log will
>>>> contain reference to other existing probe too. Hence change
>>>> usage of check_all_lines_matched.pl to check_all_patterns_found.pl
>>>> This will make sure expecting string comes in the result
>>>> 
>>>> Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
>>>> ---
>>>> tools/perf/tests/shell/base_probe/test_adding_kernel.sh | 4 ++--
>>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>> 
>>>> diff --git a/tools/perf/tests/shell/base_probe/test_adding_kernel.sh b/tools/perf/tests/shell/base_probe/test_adding_kernel.sh
>>>> index d541ffd44a93..f8b5f096d0d7 100755
>>>> --- a/tools/perf/tests/shell/base_probe/test_adding_kernel.sh
>>>> +++ b/tools/perf/tests/shell/base_probe/test_adding_kernel.sh
>>>> @@ -169,7 +169,7 @@ print_results $PERF_EXIT_CODE $CHECK_EXIT_CODE "force-adding probes :: second pr
>>>> (( TEST_RESULT += $? ))
>>>> 
>>>> # adding existing probe with '--force' should pass
>>>> -NO_OF_PROBES=`$CMD_PERF probe -l | wc -l`
>>>> +NO_OF_PROBES=`$CMD_PERF probe -l $TEST_PROBE| wc -l`
>>>> $CMD_PERF probe --force --add $TEST_PROBE 2> $LOGS_DIR/adding_kernel_forceadd_03.err
>>>> PERF_EXIT_CODE=$?
>>>> 
>>>> @@ -205,7 +205,7 @@ print_results $PERF_EXIT_CODE $CHECK_EXIT_CODE "using doubled probe"
>>>> $CMD_PERF probe --del \* 2> $LOGS_DIR/adding_kernel_removing_wildcard.err
>>>> PERF_EXIT_CODE=$?
>>>> 
>>>> -../common/check_all_lines_matched.pl "Removed event: probe:$TEST_PROBE" "Removed event: probe:${TEST_PROBE}_1" < $LOGS_DIR/adding_kernel_removing_wildcard.err
>>>> +../common/check_all_patterns_found.pl "Removed event: probe:$TEST_PROBE" "Removed event: probe:${TEST_PROBE}_1" < $LOGS_DIR/adding_kernel_removing_wildcard.err
>>>> CHECK_EXIT_CODE=$?
>>>> 
>>>> print_results $PERF_EXIT_CODE $CHECK_EXIT_CODE "removing multiple probes"
>>>> -- 
>>>> 2.43.5
>> 
>> 

Re: [PATCH] tools/perf/tests/base_probe: Fix check for the count of existing probes in test_adding_kernel

[Athira Rajeev]

> On 5 Dec 2024, at 11:16 PM, Athira Rajeev <atrajeev@linux.vnet.ibm.com> wrote:
> 
> 
> 
>> On 14 Nov 2024, at 3:35 PM, Michael Petlan <mpetlan@redhat.com> wrote:
>> 
>> On Sun, 3 Nov 2024, Athira Rajeev wrote:
>>>> On 17 Oct 2024, at 3:44 PM, Michael Petlan <mpetlan@redhat.com> wrote:
>>>> 
>>>> On Mon, 14 Oct 2024, Athira Rajeev wrote:
>> [...]
>>>> 
>>>> I am wondering how it could happen that there were other
>>>> probes in the system?
>>> 
>>> Hi Michael,
>>> 
>> Hello Athira.
>> 
>>> Sorry for the late response.
>>> 
>>> Yes, there are uprobes listed as part of “perf probe” in the environment where I saw the test needing this change. Sharing the result below from perf probe: 
>>> 
>>> # ./perf probe -l
>>> uprobes:p_uprobe_dns_events_osquery4026531841 (on getaddrinfo in XX)
>>> uprobes:p_uprobe_dns_events_osquery4026532336 (on 0x129a60 in XX)
>>> uprobes:p_uprobe_dns_events_osquery4026532344 (on 0x129a60 in XX)
>>> uprobes:p_uprobe_ebpf_compat_check_osquery (on __GI___backtrace in XX)
>>> uprobes:p_uprobe_sys_hook_osquery (on backtrace_symbols in XX)
>>> 
>>> These can’t be removed.
>>> 
>>> # ./perf probe -d uprobes:p_uprobe_dns_events_osquery4026531841
>>> Removed event: uprobes:p_uprobe_dns_events_osquery4026531841
>>> Failed to delete event: Device or resource busy
>>> Error: Failed to delete events.
>>> 
>> 
>> Ah, this is interesting, I have never hit that. However, it makes sense,
>> if the resource is busy.
>> 
>> However, in that case it comes to my mind that in general, these tests
>> should not be run in any production environment, where one could rely on
>> some probes will exist, etc. In case some of the probes above was not
>> busy, it'd be probably cleaned up by the testcase, which might be unexpected
>> by the creator/user of the uprobes... Maybe we should get rid of the
>> probe cleaning for that cases, but I'd prefer to keep it.
>> 
>>> Considering above scenario, patch here takes the probe count using:
>>> NO_OF_PROBES=`$CMD_PERF probe -l $TEST_PROBE| wc -l`
>>> 
>>> Also similarly looks for TEST_PROBE in result log in case of probe —del as well
>>> 
>>> Any comments Michael ?
>>> 
>> Yes, we probably should tweak it as you suggest.
> 
> Hi,
> 
> Thanks for checking Michael.
> 
> If the patch looks good, can we please get this pulled in ?
> 

Hi,

Can we please pull in this patch if it looks fine.

Thanks
Athira
> Thanks
> Athira
>> 
>> Thanks,
>> Michael
>> 
>> 
>>> Thanks
>>> Athira
>>> 
>>>> 
>>>> Cheers,
>>>> 
>>>> Michael
>>>> 
>>>>> 
>>>>> Example, in the system where this failed, already some
>>>>> probes were default added. So count became 10
>>>>> ./perf probe -l | wc -l
>>>>> 10
>>>>> 
>>>>> So to be specific for "inode_permission", restrict the
>>>>> probe count check to that probe point alone using :
>>>>> NO_OF_PROBES=`$CMD_PERF probe -l $TEST_PROBE| wc -l`
>>>>> 
>>>>> Similarly while removing the probe using "probe --del *",
>>>>> ( removing all probes ), check uses:
>>>>> 
>>>>> ../common/check_all_lines_matched.pl "Removed event: probe:$TEST_PROBE"
>>>>> 
>>>>> But if there are other probes in the system, the log will
>>>>> contain reference to other existing probe too. Hence change
>>>>> usage of check_all_lines_matched.pl to check_all_patterns_found.pl
>>>>> This will make sure expecting string comes in the result
>>>>> 
>>>>> Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
>>>>> ---
>>>>> tools/perf/tests/shell/base_probe/test_adding_kernel.sh | 4 ++--
>>>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>>> 
>>>>> diff --git a/tools/perf/tests/shell/base_probe/test_adding_kernel.sh b/tools/perf/tests/shell/base_probe/test_adding_kernel.sh
>>>>> index d541ffd44a93..f8b5f096d0d7 100755
>>>>> --- a/tools/perf/tests/shell/base_probe/test_adding_kernel.sh
>>>>> +++ b/tools/perf/tests/shell/base_probe/test_adding_kernel.sh
>>>>> @@ -169,7 +169,7 @@ print_results $PERF_EXIT_CODE $CHECK_EXIT_CODE "force-adding probes :: second pr
>>>>> (( TEST_RESULT += $? ))
>>>>> 
>>>>> # adding existing probe with '--force' should pass
>>>>> -NO_OF_PROBES=`$CMD_PERF probe -l | wc -l`
>>>>> +NO_OF_PROBES=`$CMD_PERF probe -l $TEST_PROBE| wc -l`
>>>>> $CMD_PERF probe --force --add $TEST_PROBE 2> $LOGS_DIR/adding_kernel_forceadd_03.err
>>>>> PERF_EXIT_CODE=$?
>>>>> 
>>>>> @@ -205,7 +205,7 @@ print_results $PERF_EXIT_CODE $CHECK_EXIT_CODE "using doubled probe"
>>>>> $CMD_PERF probe --del \* 2> $LOGS_DIR/adding_kernel_removing_wildcard.err
>>>>> PERF_EXIT_CODE=$?
>>>>> 
>>>>> -../common/check_all_lines_matched.pl "Removed event: probe:$TEST_PROBE" "Removed event: probe:${TEST_PROBE}_1" < $LOGS_DIR/adding_kernel_removing_wildcard.err
>>>>> +../common/check_all_patterns_found.pl "Removed event: probe:$TEST_PROBE" "Removed event: probe:${TEST_PROBE}_1" < $LOGS_DIR/adding_kernel_removing_wildcard.err
>>>>> CHECK_EXIT_CODE=$?
>>>>> 
>>>>> print_results $PERF_EXIT_CODE $CHECK_EXIT_CODE "removing multiple probes"
>>>>> -- 
>>>>> 2.43.5

Re: [PATCH] tools/perf/tests/base_probe: Fix check for the count of existing probes in test_adding_kernel

[Athira Rajeev]

> On 17 Dec 2024, at 12:27 AM, Athira Rajeev <atrajeev@linux.vnet.ibm.com> wrote:
> 
> 
> 
>> On 5 Dec 2024, at 11:16 PM, Athira Rajeev <atrajeev@linux.vnet.ibm.com> wrote:
>> 
>> 
>> 
>>> On 14 Nov 2024, at 3:35 PM, Michael Petlan <mpetlan@redhat.com> wrote:
>>> 
>>> On Sun, 3 Nov 2024, Athira Rajeev wrote:
>>>>> On 17 Oct 2024, at 3:44 PM, Michael Petlan <mpetlan@redhat.com> wrote:
>>>>> 
>>>>> On Mon, 14 Oct 2024, Athira Rajeev wrote:
>>> [...]
>>>>> 
>>>>> I am wondering how it could happen that there were other
>>>>> probes in the system?
>>>> 
>>>> Hi Michael,
>>>> 
>>> Hello Athira.
>>> 
>>>> Sorry for the late response.
>>>> 
>>>> Yes, there are uprobes listed as part of “perf probe” in the environment where I saw the test needing this change. Sharing the result below from perf probe: 
>>>> 
>>>> # ./perf probe -l
>>>> uprobes:p_uprobe_dns_events_osquery4026531841 (on getaddrinfo in XX)
>>>> uprobes:p_uprobe_dns_events_osquery4026532336 (on 0x129a60 in XX)
>>>> uprobes:p_uprobe_dns_events_osquery4026532344 (on 0x129a60 in XX)
>>>> uprobes:p_uprobe_ebpf_compat_check_osquery (on __GI___backtrace in XX)
>>>> uprobes:p_uprobe_sys_hook_osquery (on backtrace_symbols in XX)
>>>> 
>>>> These can’t be removed.
>>>> 
>>>> # ./perf probe -d uprobes:p_uprobe_dns_events_osquery4026531841
>>>> Removed event: uprobes:p_uprobe_dns_events_osquery4026531841
>>>> Failed to delete event: Device or resource busy
>>>> Error: Failed to delete events.
>>>> 
>>> 
>>> Ah, this is interesting, I have never hit that. However, it makes sense,
>>> if the resource is busy.
>>> 
>>> However, in that case it comes to my mind that in general, these tests
>>> should not be run in any production environment, where one could rely on
>>> some probes will exist, etc. In case some of the probes above was not
>>> busy, it'd be probably cleaned up by the testcase, which might be unexpected
>>> by the creator/user of the uprobes... Maybe we should get rid of the
>>> probe cleaning for that cases, but I'd prefer to keep it.
>>> 
>>>> Considering above scenario, patch here takes the probe count using:
>>>> NO_OF_PROBES=`$CMD_PERF probe -l $TEST_PROBE| wc -l`
>>>> 
>>>> Also similarly looks for TEST_PROBE in result log in case of probe —del as well
>>>> 
>>>> Any comments Michael ?
>>>> 
>>> Yes, we probably should tweak it as you suggest.
>> 
>> Hi,
>> 
>> Thanks for checking Michael.
>> 
>> If the patch looks good, can we please get this pulled in ?
>> 
> 
> Hi,
> 
> Can we please pull in this patch if it looks fine.
> 
> Thanks
> Athira

Hi,

Looking for review comments on this

Athira
>> Thanks
>> Athira
>>> 
>>> Thanks,
>>> Michael
>>> 
>>> 
>>>> Thanks
>>>> Athira
>>>> 
>>>>> 
>>>>> Cheers,
>>>>> 
>>>>> Michael
>>>>> 
>>>>>> 
>>>>>> Example, in the system where this failed, already some
>>>>>> probes were default added. So count became 10
>>>>>> ./perf probe -l | wc -l
>>>>>> 10
>>>>>> 
>>>>>> So to be specific for "inode_permission", restrict the
>>>>>> probe count check to that probe point alone using :
>>>>>> NO_OF_PROBES=`$CMD_PERF probe -l $TEST_PROBE| wc -l`
>>>>>> 
>>>>>> Similarly while removing the probe using "probe --del *",
>>>>>> ( removing all probes ), check uses:
>>>>>> 
>>>>>> ../common/check_all_lines_matched.pl "Removed event: probe:$TEST_PROBE"
>>>>>> 
>>>>>> But if there are other probes in the system, the log will
>>>>>> contain reference to other existing probe too. Hence change
>>>>>> usage of check_all_lines_matched.pl to check_all_patterns_found.pl
>>>>>> This will make sure expecting string comes in the result
>>>>>> 
>>>>>> Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
>>>>>> ---
>>>>>> tools/perf/tests/shell/base_probe/test_adding_kernel.sh | 4 ++--
>>>>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>> 
>>>>>> diff --git a/tools/perf/tests/shell/base_probe/test_adding_kernel.sh b/tools/perf/tests/shell/base_probe/test_adding_kernel.sh
>>>>>> index d541ffd44a93..f8b5f096d0d7 100755
>>>>>> --- a/tools/perf/tests/shell/base_probe/test_adding_kernel.sh
>>>>>> +++ b/tools/perf/tests/shell/base_probe/test_adding_kernel.sh
>>>>>> @@ -169,7 +169,7 @@ print_results $PERF_EXIT_CODE $CHECK_EXIT_CODE "force-adding probes :: second pr
>>>>>> (( TEST_RESULT += $? ))
>>>>>> 
>>>>>> # adding existing probe with '--force' should pass
>>>>>> -NO_OF_PROBES=`$CMD_PERF probe -l | wc -l`
>>>>>> +NO_OF_PROBES=`$CMD_PERF probe -l $TEST_PROBE| wc -l`
>>>>>> $CMD_PERF probe --force --add $TEST_PROBE 2> $LOGS_DIR/adding_kernel_forceadd_03.err
>>>>>> PERF_EXIT_CODE=$?
>>>>>> 
>>>>>> @@ -205,7 +205,7 @@ print_results $PERF_EXIT_CODE $CHECK_EXIT_CODE "using doubled probe"
>>>>>> $CMD_PERF probe --del \* 2> $LOGS_DIR/adding_kernel_removing_wildcard.err
>>>>>> PERF_EXIT_CODE=$?
>>>>>> 
>>>>>> -../common/check_all_lines_matched.pl "Removed event: probe:$TEST_PROBE" "Removed event: probe:${TEST_PROBE}_1" < $LOGS_DIR/adding_kernel_removing_wildcard.err
>>>>>> +../common/check_all_patterns_found.pl "Removed event: probe:$TEST_PROBE" "Removed event: probe:${TEST_PROBE}_1" < $LOGS_DIR/adding_kernel_removing_wildcard.err
>>>>>> CHECK_EXIT_CODE=$?
>>>>>> 
>>>>>> print_results $PERF_EXIT_CODE $CHECK_EXIT_CODE "removing multiple probes"
>>>>>> -- 
>>>>>> 2.43.5