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

[Dr. David Alan Gilbert]

* 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

> >> 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