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

[Yuval Shaia]

On Fri, Feb 01, 2019 at 08:33:44AM +0100, Markus Armbruster 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.

Yeah.
I really meant to say "QMP is not effective *choice* for my needs".

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

I was asking in a context of what is the standard way to do it.

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

Thanks!
Well, for now i want only to expose debugging-related info and have no idea
yet what would be the best to expose to end-users via QMP events.
Device statistics for end-users are currently exposed by the device driver
in guest. If in the future we will see that this info is also needed in the
host then i'll revisit it.

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

One use case from real live example is that this interface helped me to
detect a leak in freeing a context of a resource.

> 
> >> If this is the correct method for this purpose then let me know and i'll
> >> update the git log message accordingly.