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

[Markus Armbruster]

vandersonmr <address@hidden> writes:

> adding options to list tbs by some metric and
> investigate their code.

What's "tbs"?

Why is listing them useful?

What do you mean by "some metric"?

What do you mean by "and investigate their code?"

> Signed-off-by: Vanderson M. do Rosario <address@hidden>
> ---
>  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",

Your use of [square brackets] in .help is unusual.  Please try to have
your commands' help blend into the existing help.

> +        .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,
> +    },
> +    {
> +        .name       = "coverset",
> +        .args_type  = "number:i?",
> +        .params     = "[number]",
> +        .help       = "show hottest translated blocks neccesary to cover"
> +                      "[number]% of the execution count",

When a parameter takes a percentage, "number" is a suboptimal name :)

> +        .cmd        = hmp_info_coverset,
>      },
>  #endif
>  

Standard request for new HMP commands without corresponding QMP
commands: please state in the commit message why the QMP command is not
worthwhile.

HMP commands without a QMP equivalent are okay if their functionality
makes no sense in QMP, or is of use only for human users.

Example for "makes no sense in QMP": setting the current CPU, because a
QMP monitor doesn't have a current CPU.

Examples for "is of use only for human users": HMP command "help", the
integrated pocket calculator.

Debugging commands are kind of borderline.  Debugging is commonly a
human activity, where HMP is just fine.  However, humans create tools to
assist with their activities, and then QMP is useful.  While I wouldn't
encourage HMP-only for the debugging use case, I wouldn't veto it.

Your (overly terse!) commit message and help texts make me guess the
commands are for gathering statistics.  Statistics can have debugging
uses.  But they often have non-debugging uses as well.  What use cases
can you imagine for these commands?

[...]