Re: [PATCH 4/5] monitor: removed cpustats command

[Bruno Piazera Larsen]

On 27/05/2021 05:30, Greg Kurz wrote:

> On Thu, 27 May 2021 09:09:55 +0100
> "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
>
> > * Greg Kurz (groug@kaod.org) wrote:
> > > On Wed, 26 May 2021 17:21:03 -0300
> > > "Bruno Larsen (billionai)" <bruno.larsen@eldorado.org.br> wrote:
> > >
> > > > Since ppc was the last architecture to collect these statistics and
> > > > it is currently phasing this collection out, the command that would query
> > > > this information is being removed.
> > > >
> > > >           
> > > So this is removing an obviously user visible feature. This should be
> > > mentioned in docs/system/removed-features.rst... but, wait, I don't
> > > see anything for it in docs/system/deprecated.rst. This is dropping
> > > a feature without following the usual deprecation policy, i.e.
> > > marking the feature as deprecated and only remove it 2 QEMU versions
> > > later. Any justification for that ?
> > >         
> >       
>

The command called a function that ultimately called an empty callback unless you changed target/ppc/translate.c and removed the comments around #define DO_PPC_STATISTICS. And like I mentioned in the cover letter (which may not have been CC'ed to you, sorry) ppc was the last architecture to support this feature while they were using the legacy decode system, but as they move to decodetree, this data wouldn't even be collected.

That's the justification, basically.

> > As long as the command really isn't useful any more, I wouldn't object
> > to that from an HMP point of view.
> >
> >       
> Ok then this should be documented in docs/system/removed-features.rst at
> least.
>     
    Sure, will send a patch later today with the update
> > Dave
> >
> > > David,
> > >
> > > Unrelated, I saw that you already applied this to ppc-for-6.1 on gitlab
> > > but the commit title appears to be broken:
> > >
> > > '65;6401;1cmonitor: removed cpustats command
> > >
> > > https://gitlab.com/dgibson/qemu/-/commit/532be563eae6b8ae834ff7e9ebb1428f53569a69
> > >
> > > > Suggested-by: Richard Henderson <richard.henderson@linaro.org>
> > > > Signed-off-by: Bruno Larsen (billionai) <bruno.larsen@eldorado.org.br>
> > > > ---
> > > >  hmp-commands-info.hx | 13 -------------
> > > >  monitor/misc.c       | 11 -----------
> > > >  2 files changed, 24 deletions(-)
> > > >
> > > > diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
> > > > index ab0c7aa5ee..b2347a6aea 100644
> > > > --- a/hmp-commands-info.hx
> > > > +++ b/hmp-commands-info.hx
> > > > @@ -500,19 +500,6 @@ SRST
> > > >      Show the current VM UUID.
> > > >  ERST
> > > >  
> > > > -    {
> > > > -        .name       = "cpustats",
> > > > -        .args_type  = "",
> > > > -        .params     = "",
> > > > -        .help       = "show CPU statistics",
> > > > -        .cmd        = hmp_info_cpustats,
> > > > -    },
> > > > -
> > > > -SRST
> > > > -  ``info cpustats``
> > > > -    Show CPU statistics.
> > > > -ERST
> > > > -
> > > >  #if defined(CONFIG_SLIRP)
> > > >      {
> > > >          .name       = "usernet",
> > > > diff --git a/monitor/misc.c b/monitor/misc.c
> > > > index f3a393ea59..1539e18557 100644
> > > > --- a/monitor/misc.c
> > > > +++ b/monitor/misc.c
> > > > @@ -369,17 +369,6 @@ static void hmp_info_history(Monitor *mon, const QDict *qdict)
> > > >      }
> > > >  }
> > > >  
> > > > -static void hmp_info_cpustats(Monitor *mon, const QDict *qdict)
> > > > -{
> > > > -    CPUState *cs = mon_get_cpu(mon);
> > > > -
> > > > -    if (!cs) {
> > > > -        monitor_printf(mon, "No CPU available\n");
> > > > -        return;
> > > > -    }
> > > > -    cpu_dump_statistics(cs, 0);
> > > > -}
> > > > -
> > > >  static void hmp_info_trace_events(Monitor *mon, const QDict *qdict)
> > > >  {
> > > >      const char *name = qdict_get_try_str(qdict, "name");
> > > >           
> > >         
> >       
>

--
Bruno Piazera Larsen
Instituto de Pesquisas ELDORADO
Departamento Computação Embarcada
Analista de Software Trainee
Aviso Legal - Disclaimer