[PATCH] tests/plugin: Remove duplicate insn log from libinsn.so

[PATCH] tests/plugin: Remove duplicate insn log from libinsn.so

[Richard Henderson]

This is a perfectly natural occurrence for x86 "rep movb",
where the "rep" prefix forms a counted loop of the one insn.

During the tests/tcg/multiarch/memory test, this logging is
triggered over 350000 times.  Within the context of cross-i386-tci
build, which is already slow by nature, the logging is sufficient
to push the test into timeout.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
Irritatingly, it doesn't timeout locally, so I used staging to double-check:

Fail: https://gitlab.com/qemu-project/qemu/-/jobs/4450754282#L5062
Pass: https://gitlab.com/qemu-project/qemu/-/jobs/4450927108
---
 tests/plugin/insn.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/tests/plugin/insn.c b/tests/plugin/insn.c
index cd5ea5d4ae..9bd6e44f73 100644
--- a/tests/plugin/insn.c
+++ b/tests/plugin/insn.c
@@ -19,7 +19,6 @@ QEMU_PLUGIN_EXPORT int qemu_plugin_version = QEMU_PLUGIN_VERSION;
 #define MAX_CPUS 8 /* lets not go nuts */
 
 typedef struct {
-    uint64_t last_pc;
     uint64_t insn_count;
 } InstructionCount;
 
@@ -51,13 +50,7 @@ static void vcpu_insn_exec_before(unsigned int cpu_index, void *udata)
 {
     unsigned int i = cpu_index % MAX_CPUS;
     InstructionCount *c = &counts[i];
-    uint64_t this_pc = GPOINTER_TO_UINT(udata);
-    if (this_pc == c->last_pc) {
-        g_autofree gchar *out = g_strdup_printf("detected repeat execution @ 0x%"
-                                                PRIx64 "\n", this_pc);
-        qemu_plugin_outs(out);
-    }
-    c->last_pc = this_pc;
+
     c->insn_count++;
 }
 
-- 
2.34.1

Re: [PATCH] tests/plugin: Remove duplicate insn log from libinsn.so

[Alex Bennée]

Richard Henderson <richard.henderson@linaro.org> writes:

> This is a perfectly natural occurrence for x86 "rep movb",
> where the "rep" prefix forms a counted loop of the one insn.
>
> During the tests/tcg/multiarch/memory test, this logging is
> triggered over 350000 times.  Within the context of cross-i386-tci
> build, which is already slow by nature, the logging is sufficient
> to push the test into timeout.

How does this get triggered because I added these:

# non-inline runs will trigger the duplicate instruction heuristics in libinsn.so
run-plugin-%-with-libinsn.so:
	$(call run-test, $@, \
	  $(QEMU) -monitor none -display none \
		  -chardev file$(COMMA)path=$@.out$(COMMA)id=output \
                  -plugin ../../plugin/libinsn.so$(COMMA)inline=on \
	    	  -d plugin -D $*-with-libinsn.so.pout \
		  $(QEMU_OPTS) $*)

to prevent the callback versions from being called for x86. The original
intent of the check was to detect failures due to cpu_io_recompile, see
e025d799af (tests/plugin: expand insn test to detect duplicate instructions)

>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
> Irritatingly, it doesn't timeout locally, so I used staging to double-check:
>
> Fail: https://gitlab.com/qemu-project/qemu/-/jobs/4450754282#L5062
> Pass: https://gitlab.com/qemu-project/qemu/-/jobs/4450927108
> ---
>  tests/plugin/insn.c | 9 +--------
>  1 file changed, 1 insertion(+), 8 deletions(-)
>
> diff --git a/tests/plugin/insn.c b/tests/plugin/insn.c
> index cd5ea5d4ae..9bd6e44f73 100644
> --- a/tests/plugin/insn.c
> +++ b/tests/plugin/insn.c
> @@ -19,7 +19,6 @@ QEMU_PLUGIN_EXPORT int qemu_plugin_version = QEMU_PLUGIN_VERSION;
>  #define MAX_CPUS 8 /* lets not go nuts */
>  
>  typedef struct {
> -    uint64_t last_pc;
>      uint64_t insn_count;
>  } InstructionCount;
>  
> @@ -51,13 +50,7 @@ static void vcpu_insn_exec_before(unsigned int cpu_index, void *udata)
>  {
>      unsigned int i = cpu_index % MAX_CPUS;
>      InstructionCount *c = &counts[i];
> -    uint64_t this_pc = GPOINTER_TO_UINT(udata);
> -    if (this_pc == c->last_pc) {
> -        g_autofree gchar *out = g_strdup_printf("detected repeat execution @ 0x%"
> -                                                PRIx64 "\n", this_pc);
> -        qemu_plugin_outs(out);
> -    }
> -    c->last_pc = this_pc;
> +
>      c->insn_count++;
>  }


-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH] tests/plugin: Remove duplicate insn log from libinsn.so

[Richard Henderson]

On 6/11/23 02:14, Alex Bennée wrote:
> 
> Richard Henderson <richard.henderson@linaro.org> writes:
> 
>> This is a perfectly natural occurrence for x86 "rep movb",
>> where the "rep" prefix forms a counted loop of the one insn.
>>
>> During the tests/tcg/multiarch/memory test, this logging is
>> triggered over 350000 times.  Within the context of cross-i386-tci
>> build, which is already slow by nature, the logging is sufficient
>> to push the test into timeout.
> 
> How does this get triggered because I added these:
> 
> # non-inline runs will trigger the duplicate instruction heuristics in libinsn.so
> run-plugin-%-with-libinsn.so:
> 	$(call run-test, $@, \
> 	  $(QEMU) -monitor none -display none \
> 		  -chardev file$(COMMA)path=$@.out$(COMMA)id=output \
>                    -plugin ../../plugin/libinsn.so$(COMMA)inline=on \
> 	    	  -d plugin -D $*-with-libinsn.so.pout \
> 		  $(QEMU_OPTS) $*)
> 
> to prevent the callback versions from being called for x86. The original
> intent of the check was to detect failures due to cpu_io_recompile, see
> e025d799af (tests/plugin: expand insn test to detect duplicate instructions)

I have no idea how, but it's happening.


>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>> Irritatingly, it doesn't timeout locally, so I used staging to double-check:
>>
>> Fail: https://gitlab.com/qemu-project/qemu/-/jobs/4450754282#L5062
>> Pass: https://gitlab.com/qemu-project/qemu/-/jobs/4450927108

Note that in the pass case, we don't even log that the test ran.


r~

Re: [PATCH] tests/plugin: Remove duplicate insn log from libinsn.so

[Richard Henderson]

On 6/12/23 04:50, Richard Henderson wrote:
> On 6/11/23 02:14, Alex Bennée wrote:
>>
>> Richard Henderson <richard.henderson@linaro.org> writes:
>>
>>> This is a perfectly natural occurrence for x86 "rep movb",
>>> where the "rep" prefix forms a counted loop of the one insn.
>>>
>>> During the tests/tcg/multiarch/memory test, this logging is
>>> triggered over 350000 times.  Within the context of cross-i386-tci
>>> build, which is already slow by nature, the logging is sufficient
>>> to push the test into timeout.
>>
>> How does this get triggered because I added these:
>>
>> # non-inline runs will trigger the duplicate instruction heuristics in libinsn.so
>> run-plugin-%-with-libinsn.so:
>>     $(call run-test, $@, \
>>       $(QEMU) -monitor none -display none \
>>           -chardev file$(COMMA)path=$@.out$(COMMA)id=output \
>>                    -plugin ../../plugin/libinsn.so$(COMMA)inline=on \
>>               -d plugin -D $*-with-libinsn.so.pout \
>>           $(QEMU_OPTS) $*)
>>
>> to prevent the callback versions from being called for x86. The original
>> intent of the check was to detect failures due to cpu_io_recompile, see
>> e025d799af (tests/plugin: expand insn test to detect duplicate instructions)
> 
> I have no idea how, but it's happening.
> 
> 
>>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>>> ---
>>> Irritatingly, it doesn't timeout locally, so I used staging to double-check:
>>>
>>> Fail: https://gitlab.com/qemu-project/qemu/-/jobs/4450754282#L5062
>>> Pass: https://gitlab.com/qemu-project/qemu/-/jobs/4450927108
> 
> Note that in the pass case, we don't even log that the test ran.

Any further thoughts on this?  Otherwise I'll merge it to get rid of the cross-i386-tci 
failure...


r~

Re: [PATCH] tests/plugin: Remove duplicate insn log from libinsn.so

[Alex Bennée]

Richard Henderson <richard.henderson@linaro.org> writes:

> On 6/12/23 04:50, Richard Henderson wrote:
>> On 6/11/23 02:14, Alex Bennée wrote:
>>>
>>> Richard Henderson <richard.henderson@linaro.org> writes:
>>>
>>>> This is a perfectly natural occurrence for x86 "rep movb",
>>>> where the "rep" prefix forms a counted loop of the one insn.
>>>>
>>>> During the tests/tcg/multiarch/memory test, this logging is
>>>> triggered over 350000 times.  Within the context of cross-i386-tci
>>>> build, which is already slow by nature, the logging is sufficient
>>>> to push the test into timeout.
>>>
>>> How does this get triggered because I added these:
>>>
>>> # non-inline runs will trigger the duplicate instruction heuristics in libinsn.so
>>> run-plugin-%-with-libinsn.so:
>>>     $(call run-test, $@, \
>>>       $(QEMU) -monitor none -display none \
>>>           -chardev file$(COMMA)path=$@.out$(COMMA)id=output \
>>>                    -plugin ../../plugin/libinsn.so$(COMMA)inline=on \
>>>               -d plugin -D $*-with-libinsn.so.pout \
>>>           $(QEMU_OPTS) $*)
>>>
>>> to prevent the callback versions from being called for x86. The original
>>> intent of the check was to detect failures due to cpu_io_recompile, see
>>> e025d799af (tests/plugin: expand insn test to detect duplicate instructions)
>> I have no idea how, but it's happening.
>> 
>>>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>>>> ---
>>>> Irritatingly, it doesn't timeout locally, so I used staging to double-check:
>>>>
>>>> Fail: https://gitlab.com/qemu-project/qemu/-/jobs/4450754282#L5062
>>>> Pass: https://gitlab.com/qemu-project/qemu/-/jobs/4450927108
>> Note that in the pass case, we don't even log that the test ran.
>
> Any further thoughts on this?  Otherwise I'll merge it to get rid of
> the cross-i386-tci failure...
>
>
> r~

I'm happy to drop the feature from the plugin but the clean-up also
needs to be applied to the run-plugin-%-with-libinsn.so: rules for i386
and x86_64.

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH] tests/plugin: Remove duplicate insn log from libinsn.so

[Richard Henderson]

On 6/19/23 19:34, Alex Bennée wrote:
> 
> Richard Henderson <richard.henderson@linaro.org> writes:
> 
>> On 6/12/23 04:50, Richard Henderson wrote:
>>> On 6/11/23 02:14, Alex Bennée wrote:
>>>>
>>>> Richard Henderson <richard.henderson@linaro.org> writes:
>>>>
>>>>> This is a perfectly natural occurrence for x86 "rep movb",
>>>>> where the "rep" prefix forms a counted loop of the one insn.
>>>>>
>>>>> During the tests/tcg/multiarch/memory test, this logging is
>>>>> triggered over 350000 times.  Within the context of cross-i386-tci
>>>>> build, which is already slow by nature, the logging is sufficient
>>>>> to push the test into timeout.
>>>>
>>>> How does this get triggered because I added these:
>>>>
>>>> # non-inline runs will trigger the duplicate instruction heuristics in libinsn.so
>>>> run-plugin-%-with-libinsn.so:
>>>>      $(call run-test, $@, \
>>>>        $(QEMU) -monitor none -display none \
>>>>            -chardev file$(COMMA)path=$@.out$(COMMA)id=output \
>>>>                     -plugin ../../plugin/libinsn.so$(COMMA)inline=on \
>>>>                -d plugin -D $*-with-libinsn.so.pout \
>>>>            $(QEMU_OPTS) $*)
>>>>
>>>> to prevent the callback versions from being called for x86. The original
>>>> intent of the check was to detect failures due to cpu_io_recompile, see
>>>> e025d799af (tests/plugin: expand insn test to detect duplicate instructions)
>>> I have no idea how, but it's happening.
>>>
>>>>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>>>>> ---
>>>>> Irritatingly, it doesn't timeout locally, so I used staging to double-check:
>>>>>
>>>>> Fail: https://gitlab.com/qemu-project/qemu/-/jobs/4450754282#L5062
>>>>> Pass: https://gitlab.com/qemu-project/qemu/-/jobs/4450927108
>>> Note that in the pass case, we don't even log that the test ran.
>>
>> Any further thoughts on this?  Otherwise I'll merge it to get rid of
>> the cross-i386-tci failure...
>>
>>
>> r~
> 
> I'm happy to drop the feature from the plugin but the clean-up also
> needs to be applied to the run-plugin-%-with-libinsn.so: rules for i386
> and x86_64.

Pardon?  I don't know what you mean wrt changing the makefile.


r~

Re: [PATCH] tests/plugin: Remove duplicate insn log from libinsn.so

[Alex Bennée]

Richard Henderson <richard.henderson@linaro.org> writes:

> On 6/19/23 19:34, Alex Bennée wrote:
>> Richard Henderson <richard.henderson@linaro.org> writes:
>> 
>>> On 6/12/23 04:50, Richard Henderson wrote:
>>>> On 6/11/23 02:14, Alex Bennée wrote:
>>>>>
>>>>> Richard Henderson <richard.henderson@linaro.org> writes:
>>>>>
>>>>>> This is a perfectly natural occurrence for x86 "rep movb",
>>>>>> where the "rep" prefix forms a counted loop of the one insn.
>>>>>>
>>>>>> During the tests/tcg/multiarch/memory test, this logging is
>>>>>> triggered over 350000 times.  Within the context of cross-i386-tci
>>>>>> build, which is already slow by nature, the logging is sufficient
>>>>>> to push the test into timeout.
>>>>>
>>>>> How does this get triggered because I added these:
>>>>>
>>>>> # non-inline runs will trigger the duplicate instruction heuristics in libinsn.so
>>>>> run-plugin-%-with-libinsn.so:
>>>>>      $(call run-test, $@, \
>>>>>        $(QEMU) -monitor none -display none \
>>>>>            -chardev file$(COMMA)path=$@.out$(COMMA)id=output \
>>>>>                     -plugin ../../plugin/libinsn.so$(COMMA)inline=on \
>>>>>                -d plugin -D $*-with-libinsn.so.pout \
>>>>>            $(QEMU_OPTS) $*)
>>>>>
>>>>> to prevent the callback versions from being called for x86. The original
>>>>> intent of the check was to detect failures due to cpu_io_recompile, see
>>>>> e025d799af (tests/plugin: expand insn test to detect duplicate instructions)
>>>> I have no idea how, but it's happening.
>>>>
>>>>>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>>>>>> ---
>>>>>> Irritatingly, it doesn't timeout locally, so I used staging to double-check:
>>>>>>
>>>>>> Fail: https://gitlab.com/qemu-project/qemu/-/jobs/4450754282#L5062
>>>>>> Pass: https://gitlab.com/qemu-project/qemu/-/jobs/4450927108
>>>> Note that in the pass case, we don't even log that the test ran.
>>>
>>> Any further thoughts on this?  Otherwise I'll merge it to get rid of
>>> the cross-i386-tci failure...
>>>
>>>
>>> r~
>> I'm happy to drop the feature from the plugin but the clean-up also
>> needs to be applied to the run-plugin-%-with-libinsn.so: rules for i386
>> and x86_64.
>
> Pardon?  I don't know what you mean wrt changing the makefile.

There are a couple of places that do:

# non-inline runs will trigger the duplicate instruction heuristics in libinsn.so
run-plugin-%-with-libinsn.so:
	$(call run-test, $@, $(QEMU) $(QEMU_OPTS) \
	       -plugin ../../plugin/libinsn.so$(COMMA)inline=on \
	       -d plugin -D $*-with-libinsn.so.pout $*)

to prevent triggering the assert for x86

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro