Re: [PATCH v14 06/10] monitor: adding tb_stats hmp command

[Richard Henderson <richard.henderson@linaro.org>]

On 6/1/23 00:20, Wu, Fei wrote:
> On 6/1/2023 9:23 AM, Richard Henderson wrote:
>> On 5/30/23 01:35, Fei Wu wrote:
>>> From: "Vanderson M. do Rosario" <vandersonmr2@gmail.com>
>>>
>>> Adding tb_stats [start|pause|stop|filter] command to hmp.
>>> This allows controlling the collection of statistics.
>>> It is also possible to set the level of collection:
>>> all, jit, or exec.
>>>
>>> tb_stats filter allow to only collect statistics for the TB
>>> in the last_search list.
>>>
>>> The goal of this command is to allow the dynamic exploration
>>> of the TCG behavior and quality. Therefore, for now, a
>>> corresponding QMP command is not worthwhile.
>>>
>>> Acked-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>>> Signed-off-by: Vanderson M. do Rosario <vandersonmr2@gmail.com>
>>> Message-Id: <20190829173437.5926-8-vandersonmr2@gmail.com>
>>> Message-Id: <20190829173437.5926-9-vandersonmr2@gmail.com>
>>> [AJB: fix authorship]
>>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>>> Signed-off-by: Fei Wu <fei2.wu@intel.com>
>>> ---
>>
>>
>> I still don't see what pause does.
>>
> maybe it's necessary to discuss the user scenario, I suppose the
> original design is for this case:
> * start
> * pause - there are some interesting stats we want to keep
> * not interested timeline
> * start again - continue to investigate the interesting ones

This use case seems very complicated.  Perhaps start with something simpler.

>>>    enum TBStatsStatus {
>>>        TB_STATS_DISABLED = 0,
>>>        TB_STATS_RUNNING,
>>> -    TB_STATS_PAUSED,
>>> -    TB_STATS_STOPPED
>>> +    TB_STATS_PAUSED
>>>    };
>>
>> Why?
>>
> STOPPED is the same as DISABLED.

Then this should be squashed back to patch 2, so that it is never added.

>> Why isn't all of this in monitor.c?
>> It's not used or usable from user-only mode.
>>
> closer to tb-stats, or closer to monitor? It seems to me monitor.c only
> contains the shim layer of qmp/hmp, the real stuffs which are large
> enough are put together with their logic part, e.g. dump_exec_info().

I mean accel/tcg/monitor.c, which is built only when there exists a monitor.


r~