Re: [PATCH v1 08/13] plugins: expand the bb plugin to be thread safe and track per-cpu

[Alex Bennée]

Emilio G. Cota <cota@braap.org> writes:

> On Thu, Jul 09, 2020 at 15:13:22 +0100, Alex Bennée wrote:
>> While there isn't any easy way to make the inline counts thread safe
>
> Why not? At least in 64-bit hosts TCG will emit a single write to
> update the 64-bit counter.

Well the operation is an add so that is a load/add/store cycle on
non-x86. If we want to do it properly we should expose an atomic add
operation which may be helper based or maybe generate an atomic
operation if the backend can support it easily.

>
>> we can ensure the callback based ones are. While we are at it we can
>> reduce introduce a new option ("idle") to dump a report of the current
>
> s/reduce//
>
>> bb and insn count each time a vCPU enters the idle state.
>> 
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> Cc: Dave Bort <dbort@dbort.com>
>> 
>> ---
>> v2
>>   - fixup for non-inline linux-user case
>>   - minor cleanup and re-factor
>> ---
>>  tests/plugin/bb.c | 96 ++++++++++++++++++++++++++++++++++++++++-------
>>  1 file changed, 83 insertions(+), 13 deletions(-)
>> 
>> diff --git a/tests/plugin/bb.c b/tests/plugin/bb.c
>> index df19fd359df3..89c373e19cd8 100644
>> --- a/tests/plugin/bb.c
>> +++ b/tests/plugin/bb.c
>> @@ -16,24 +16,67 @@
>>  
>>  QEMU_PLUGIN_EXPORT int qemu_plugin_version = QEMU_PLUGIN_VERSION;
>>  
>> -static uint64_t bb_count;
>> -static uint64_t insn_count;
>> +typedef struct {
>> +    GMutex lock;
>> +    int index;
>> +    uint64_t bb_count;
>> +    uint64_t insn_count;
>> +} CPUCount;
>
> Why use a mutex?
>
> Just have a per-vCPU struct that each vCPU thread updates with atomic_write.
> Then when we want to print a report we just have to collect the counts
> with atomic_read().
>
> Also, consider just adding a comment to bb.c noting that it is not 
> thread-safe,
> and having a separate bb-threadsafe.c plugin for patch. The reason is that 
> bb.c is
> very simple, which is useful to understand the interface.
>
> Thanks,
>               E.


-- 
Alex Bennée