Re: [PATCH v14 04/10] accel/tcg: add jit stats and time to TBStatistics

[Wu, Fei]

On 5/30/2023 5:37 PM, Markus Armbruster wrote:
> Fei Wu <fei2.wu@intel.com> writes:
> 
>> This collects all the statistics for TBStatistics, not only for the
>> whole emulation but for each TB.
>>
>> Signed-off-by: Vanderson M. do Rosario <vandersonmr2@gmail.com>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> Signed-off-by: Fei Wu <fei2.wu@intel.com>
>> ---
>>  accel/tcg/monitor.c           |  20 ++++-
>>  accel/tcg/tb-stats.c          | 146 ++++++++++++++++++++++++++++++++++
>>  accel/tcg/tcg-accel-ops.c     |   7 ++
>>  accel/tcg/translate-all.c     |  70 +++++++++++++++-
>>  accel/tcg/translator.c        |   7 +-
>>  include/exec/tb-stats-flags.h |   2 +
>>  include/exec/tb-stats.h       |  46 +++++++++++
>>  include/qemu/timer.h          |   6 ++
>>  include/tcg/tcg.h             |  28 ++++++-
>>  softmmu/runstate.c            |   9 +++
>>  tcg/tcg.c                     |  88 ++++++++++++++++++--
>>  tests/qtest/qmp-cmd-test.c    |   3 +
>>  12 files changed, 417 insertions(+), 15 deletions(-)
>>
>> diff --git a/accel/tcg/monitor.c b/accel/tcg/monitor.c
>> index e903dd1d2e..2bc87f2642 100644
>> --- a/accel/tcg/monitor.c
>> +++ b/accel/tcg/monitor.c
>> @@ -15,6 +15,7 @@
>>  #include "sysemu/cpus.h"
>>  #include "sysemu/cpu-timers.h"
>>  #include "sysemu/tcg.h"
>> +#include "exec/tb-stats.h"
>>  #include "internal.h"
>>  
>>  
>> @@ -69,6 +70,11 @@ HumanReadableText *qmp_x_query_opcount(Error **errp)
>>  {
>>      g_autoptr(GString) buf = g_string_new("");
>>  
>> +    if (!tb_stats_collection_enabled()) {
>> +        error_setg(errp, "TB information not being recorded.");
> 
> From error_setg()'s contract in include/qapi/error.h:
> 
>      * The resulting message should be a single phrase, with no newline or
>      * trailing punctuation.
> 
> Please drop the period.  Same elsewhere, not flagging it again there.
> 
Got it, will do.

>> +        return NULL;
>> +    }
>> +
>>      if (!tcg_enabled()) {
>>          error_setg(errp,
>>                     "Opcode count information is only available with 
>> accel=tcg");
>> @@ -80,11 +86,23 @@ HumanReadableText *qmp_x_query_opcount(Error **errp)
>>      return human_readable_text_from_str(buf);
>>  }
>>  
>> +#ifdef CONFIG_TCG
>> +HumanReadableText *qmp_x_query_profile(Error **errp)
>> +{
>> +    g_autoptr(GString) buf = g_string_new("");
>> +
>> +    dump_jit_exec_time_info(dev_time, buf);
>> +    dev_time = 0;
>> +
>> +    return human_readable_text_from_str(buf);
>> +}
>> +#else
>>  HumanReadableText *qmp_x_query_profile(Error **errp)
>>  {
>> -    error_setg(errp, "Internal profiler not compiled");
>> +    error_setg(errp, "TCG should be enabled!");
>>      return NULL;
>>  }
>> +#endif
> 
> machine.json has
> 
>    ##
>    # @x-query-profile:
>    #
>    # Query TCG profiling information
>    #
>    # Features:
>    #
>    # @unstable: This command is meant for debugging.
>    #
>    # Returns: profile information
>    #
>    # Since: 6.2
>    ##
>    { 'command': 'x-query-profile',
>      'returns': 'HumanReadableText',
>      'if': 'CONFIG_TCG',
>      'features': [ 'unstable' ] }
> 
> Not changed in this series.
> 
> Note the command is conditional on CONFIG_TCG, i.e. code generated for
> it is #if defined(CONFIG_TCG).
> 
> The only other use is in hmp-commands-info.hx, and it is also guarded by
> CONFIG_TCG.
> 
> Therefore, your #else is unreachable.  You can delete it along with...
> 
OK, so qmp_x_query_profile won't get called #ifndef CONFIG_TCG, I will
remove it.

Thanks,
Fei.

>>  
>>  static void hmp_tcg_register(void)
>>  {
> 
> [...]
> 
>> diff --git a/tests/qtest/qmp-cmd-test.c b/tests/qtest/qmp-cmd-test.c
>> index 73a670e8fa..749aafe4da 100644
>> --- a/tests/qtest/qmp-cmd-test.c
>> +++ b/tests/qtest/qmp-cmd-test.c
>> @@ -46,6 +46,9 @@ static int query_error_class(const char *cmd)
>>          { "query-balloon", ERROR_CLASS_DEVICE_NOT_ACTIVE },
>>          { "query-hotpluggable-cpus", ERROR_CLASS_GENERIC_ERROR },
>>          { "query-vm-generation-id", ERROR_CLASS_GENERIC_ERROR },
>> +#ifndef CONFIG_TCG
>> +        { "x-query-profile", ERROR_CLASS_GENERIC_ERROR },
>> +#endif
> 
> ... this entry.
> 
>>          /* Only valid with a USB bus added */
>>          { "x-query-usb", ERROR_CLASS_GENERIC_ERROR },
>>          /* Only valid with accel=tcg */
>