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

[Markus Armbruster]

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.

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

>  
>  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 */