Re: [Qemu-devel] [PATCH 07/10] monitor: Expose pvrdma device statistics counters

[Yuval Shaia]

On Fri, Feb 01, 2019 at 11:42:57AM +0000, Dr. David Alan Gilbert wrote:
> * Markus Armbruster (address@hidden) wrote:
> > Eric Blake <address@hidden> writes:
> > 
> > > On 1/31/19 2:08 PM, Yuval Shaia wrote:
> > >> On Thu, Jan 31, 2019 at 07:17:16AM -0600, Eric Blake wrote:
> > >>> On 1/31/19 7:08 AM, Yuval Shaia wrote:
> > >>>> Signed-off-by: Yuval Shaia <address@hidden>
> > >>>> ---
> > >>>>  hmp-commands-info.hx | 14 ++++++++++++++
> > >>>>  monitor.c            |  6 ++++++
> > >>>>  2 files changed, 20 insertions(+)
> > >> 
> > >> Hi Eric,
> > >> 
> > >>>
> > >>> Commit message should state WHY this is being added as an HMP-only
> > >>> command, and does not have a QMP counterpart.  It may be okay if the
> > >>> interface is only designed to be useful to developers, but having that
> > >>> justification in the git log is important.
> > >> 
> > >> Thanks for your review.
> > >> 
> > >> See, i need this interface mainly for development/debug purposes, to help
> > >> troubleshot problems and to give insights to what device "is doing".
> > >> 
> > >> Trace points are great but not effective in high load.
> > >> QMP as i see it, and correct me if i'm wrong, is used to report 
> > >> management
> > >> events etc and also here, is not effective in high load.
> > 
> > If QMP is not effective, HMP won't be effective, either.  But I guess
> > you mean something else, namely QMP *events* aren't effective, but
> > *polling* is.
> > 
> > That's an argument for polling, not an argument for not supporting QMP.
> > 
> > >> I choose this interface as it is interactive, i.e. whenever i need the 
> > >> info
> > >> i trigger 'info pvrdmastats' command from the monitor console.
> > >> 
> > >> During my research i notice that some devices (or families) have nice 
> > >> user
> > >> interface via virsh (blkstat, ifstat, memstat etc). Is it the preferred 
> > >> way
> > >> for non-devel/debug purposes?
> > 
> > Libvirt interfaces like these are built on top of *QMP* interfaces.  If
> > a libvirt interface would be useful, that's another argument for
> > supporting QMP.
> > 
> > > Using existing HMP-only debug interfaces as the design you copied is
> > > indeed acceptable justification for making yours HMP-only as well.  So
> > > now you just need to copy the rationale from this email into your commit
> > > message, so it doesn't get lost.
> > 
> > Yes.  If we conclude HMP-only is okay, then the rationale for it goes
> > into your commit message.
> > 
> > If we conclude we want HMP and QMP, I'll be happy to assist you with
> > adapting your patch.
> > 
> > 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.
> > 
> > "Device statistics" sounds like it should have debugging uses.  But
> > statistics often have non-debugging uses as well.  What use cases can
> > you imagine for this command?
> 
> I think the question here partially comes down to how 'stable' the set
> of statistics is and how useful they are to non-developers.
> The 'stable' part is a question because when you expose something via
> QMP it's part of the ABI so people don't like it changing.
> If they're things that are generic and likely to stay the same then they
> should probably go via QMP (e.g. bandwidth, requests/second).
> If they're things that are about the internal state of your
> implementation and just for debug, then they're fine to be HMP only.
> You can add them to the qmp as well, even if they're not stable by
> prefixing the name with an x-.
> 
> Dave

Thanks for giving one more point of view, yes, this interface is not
"stable", I exposed the stuff i need now so it is highly reasonable that in
the future it will be enhanced and new stuff would be added.

Those things represent internal state of the device.

> 
> > >> If this is the correct method for this purpose then let me know and i'll
> > >> update the git log message accordingly.
> --
> Dr. David Alan Gilbert / address@hidden / Manchester, UK