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

[Richard Henderson]

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~