Re: [Qemu-devel] [PATCH v3 5/6] monitor: adding info tb and tbs to monitor

[Dr. David Alan Gilbert]

* vandersonmr (address@hidden) wrote:
> adding options to list tbs by some metric and
> investigate their code.
> 
> Signed-off-by: Vanderson M. do Rosario <address@hidden>

As Markus said you need a short justification that it's for debug etc
to justify HMP only; it doesn't need to be huge, but we should have it.

> ---
>  hmp-commands-info.hx | 22 ++++++++++++++
>  monitor/misc.c       | 69 ++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 91 insertions(+)
> 
> diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
> index c59444c461..0b8c0de95d 100644
> --- a/hmp-commands-info.hx
> +++ b/hmp-commands-info.hx
> @@ -288,6 +288,28 @@ ETEXI
>          .params     = "",
>          .help       = "show dynamic compiler info",
>          .cmd        = hmp_info_jit,
> +    {
> +        .name       = "tbs",
> +        .args_type  = "number:i?,sortedby:s?",
> +        .params     = "[number sortedby]",
> +        .help       = "show a [number] translated blocks sorted by 
> [sortedby]"
> +                      "sortedby opts: hotness hg",

If this is showing 'number' blocks then I think it should be called
'count'

> +        .cmd        = hmp_info_tbs,
> +    },
> +    {
> +        .name       = "tb",
> +        .args_type  = "id:i,flags:s?",
> +        .params     = "id [log1[,...] flags]",
> +        .help       = "show information about one translated block by id",
> +        .cmd        = hmp_info_tb,

That doesn't say what those flags are for; qemu has lots of flags for
different things; I think you're  saying it's some log flag?

> +    },
> +    {
> +        .name       = "coverset",
> +        .args_type  = "number:i?",
> +        .params     = "[number]",
> +        .help       = "show hottest translated blocks neccesary to cover"
> +                      "[number]% of the execution count",
> +        .cmd        = hmp_info_coverset,
>      },

That 'number' should be something like 'percent' or 'coverage'
>  #endif
>  
> diff --git a/monitor/misc.c b/monitor/misc.c
> index bf9faceb86..1fb4d75871 100644
> --- a/monitor/misc.c
> +++ b/monitor/misc.c
> @@ -469,6 +469,75 @@ static void hmp_info_jit(Monitor *mon, const QDict 
> *qdict)
>      dump_drift_info();
>  }
>  
> +static void hmp_info_tbs(Monitor *mon, const QDict *qdict)
> +{
> +    int n;
> +    const char *s = NULL;
> +    if (!tcg_enabled()) {
> +        error_report("TB information is only available with accel=tcg");
> +        return;
> +    }
> +    if (!tb_ctx.tb_stats.map) {
> +        error_report("no TB information recorded");
> +        return;
> +    }
> +
> +    n = qdict_get_try_int(qdict, "number", 10);
> +    s = qdict_get_try_str(qdict, "sortedby");

No need to use single characters; sortedby_str  is fine for
example.

> +    int sortedby = 0;
> +    if (s == NULL || strcmp(s, "hotness") == 0) {
> +        sortedby = SORT_BY_HOTNESS;
> +    } else if (strcmp(s, "hg") == 0) {
> +        sortedby = SORT_BY_HG;
> +    }

You should error if there's another word in 's'

> +    dump_tbs_info(n, sortedby, true);
> +}
> +
> +static void hmp_info_tb(Monitor *mon, const QDict *qdict)
> +{
> +    const int id = qdict_get_int(qdict, "id");
> +    const char *flags = qdict_get_try_str(qdict, "flags");
> +    int mask;
> +
> +    if (!tcg_enabled()) {
> +        error_report("TB information is only available with accel=tcg");
> +        return;
> +    }
> +
> +    mask = flags ? qemu_str_to_log_mask(flags) : CPU_LOG_TB_IN_ASM;
> +
> +    if (!mask) {
> +        help_cmd(mon, "log");

That's not obvious - I'd perfer you said something like
     Unable to parse log flags, see 'help log'

> +        return;
> +    }
> +
> +    dump_tb_info(id, mask, true);
> +}
> +
> +static void hmp_info_coverset(Monitor *mon, const QDict *qdict)
> +{
> +    int n;
> +    if (!tcg_enabled()) {
> +        error_report("TB information is only available with accel=tcg");
> +        return;
> +    }
> +    if (!qemu_loglevel_mask(CPU_LOG_HOT_TBS)) {
> +        error_report("TB information not being recorded");
> +        return;
> +    }
> +
> +    n = qdict_get_try_int(qdict, "number", 90);
> +
> +    if (n < 0 || n > 100) {
> +        error_report("Coverset percentage should be between 0 and 100");
> +        return;
> +    }
> +
> +    dump_coverset_info(n, true);
> +}
> +
>  static void hmp_info_opcount(Monitor *mon, const QDict *qdict)
>  {
>      dump_opcount_info();
> -- 
> 2.22.0
> 
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK