| Message ID | 20190131130850.6850-8-yuval.shaia@oracle.com |
|---|---|
| State | New |
| Headers | show |
| Series | Misc fixes to pvrdma device | expand |
On 1/31/19 7:08 AM, Yuval Shaia wrote: > Signed-off-by: Yuval Shaia <yuval.shaia@oracle.com> > --- > hmp-commands-info.hx | 14 ++++++++++++++ > monitor.c | 6 ++++++ > 2 files changed, 20 insertions(+) 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.
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 <yuval.shaia@oracle.com> > > --- > > 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. 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? If this is the correct method for this purpose then let me know and i'll update the git log message accordingly. Thanks. > > -- > Eric Blake, Principal Software Engineer > Red Hat, Inc. +1-919-301-3226 > Virtualization: qemu.org | libvirt.org >
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 <yuval.shaia@oracle.com> >>> --- >>> 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. > > 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? 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. > > If this is the correct method for this purpose then let me know and i'll > update the git log message accordingly. >
Eric Blake <eblake@redhat.com> 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 <yuval.shaia@oracle.com> >>>> --- >>>> 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? >> If this is the correct method for this purpose then let me know and i'll >> update the git log message accordingly.
* Markus Armbruster (armbru@redhat.com) wrote: > Eric Blake <eblake@redhat.com> 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 <yuval.shaia@oracle.com> > >>>> --- > >>>> 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 / dgilbert@redhat.com / Manchester, UK
On Fri, Feb 01, 2019 at 08:33:44AM +0100, Markus Armbruster wrote: > Eric Blake <eblake@redhat.com> 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 <yuval.shaia@oracle.com> > >>>> --- > >>>> 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.
On Fri, Feb 01, 2019 at 11:42:57AM +0000, Dr. David Alan Gilbert wrote: > * Markus Armbruster (armbru@redhat.com) wrote: > > Eric Blake <eblake@redhat.com> 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 <yuval.shaia@oracle.com> > > >>>> --- > > >>>> 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 / dgilbert@redhat.com / Manchester, UK
Yuval Shaia <yuval.shaia@oracle.com> writes: > 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 <yuval.shaia@oracle.com> >> > --- >> > 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. I figure that depends on the trace backend. The "log" backend can be problematic for high tracing rates. "ftrace", "dtrace" and "ust" backends should support such cases. Stefan, any advice? > 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. > > 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? > > If this is the correct method for this purpose then let me know and i'll > update the git log message accordingly. > > Thanks.
Yuval Shaia <yuval.shaia@oracle.com> writes: > On Fri, Feb 01, 2019 at 08:33:44AM +0100, Markus Armbruster wrote: >> Eric Blake <eblake@redhat.com> 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 <yuval.shaia@oracle.com> >> >>>> --- >> >>>> 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. You were right to ask 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. For me, there are four cases of the "get information from QEMU to the user": 1. Information that is of use only for developers a. When tracing works, use tracing b. When it doesn't, we can consider QMP + HMP command, or HMP command only. A QMP command would carry an x- prefix to mark it unstable. 2. Information that is of use only for human users Provide an HMP command. 3. Information a management application such as libvirt wants to provide, but not monitor The QEMU part is just like for 4a below. The difference is that the management application doesn't poll automatically. 4. Information a management application such as libvirt wants to monitor This is not the case here, but I mention it for completeness. a. The obvious way to monitor is regular polling via QMP. Provide a QMP command to poll. b. Another way is tracking a QMP event that reports changes, plus polling on reconnect. This is generally more efficient. Provide a QMP event tracking changes, and a command to poll. The event may have to be rate-limited. If the information is also useful for human users, throw in a command to poll via HMP. I'm not yet sure tracing doesn't work for your use case. I replied to your claim it's not effective upthread. Let's discuss it there. >> 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. Sounds like you can't think of a use other than debugging. That means we should think harder about using tracepoints. >> >> If this is the correct method for this purpose then let me know and i'll >> >> update the git log message accordingly.
On Mon, Feb 04, 2019 at 09:23:49AM +0100, Markus Armbruster wrote: > Yuval Shaia <yuval.shaia@oracle.com> writes: > > > On Fri, Feb 01, 2019 at 08:33:44AM +0100, Markus Armbruster wrote: > >> Eric Blake <eblake@redhat.com> 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 <yuval.shaia@oracle.com> > >> >>>> --- > >> >>>> 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. > > You were right to ask 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. > > For me, there are four cases of the "get information from QEMU to the > user": > > 1. Information that is of use only for developers > > a. When tracing works, use tracing > > b. When it doesn't, we can consider QMP + HMP command, or HMP command > only. A QMP command would carry an x- prefix to mark it unstable. > > 2. Information that is of use only for human users > > Provide an HMP command. > > 3. Information a management application such as libvirt wants to > provide, but not monitor > > The QEMU part is just like for 4a below. The difference is that the > management application doesn't poll automatically. > > 4. Information a management application such as libvirt wants to monitor > > This is not the case here, but I mention it for completeness. > > a. The obvious way to monitor is regular polling via QMP. Provide a > QMP command to poll. > > b. Another way is tracking a QMP event that reports changes, plus > polling on reconnect. This is generally more efficient. Provide > a QMP event tracking changes, and a command to poll. The event > may have to be rate-limited. > > If the information is also useful for human users, throw in a command > to poll via HMP. > > I'm not yet sure tracing doesn't work for your use case. I replied > to your claim it's not effective upthread. Let's discuss it there. We conclude there, and correct me if i misunderstood you, that for 'polling' it make sense to use HMP only. > > >> 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. > > Sounds like you can't think of a use other than debugging. That means > we should think harder about using tracepoints. Here is a case where tracepoint cannot help. Consider a counter that counts the number of sent buffers. Now, this counter get increased on every buffer that is sent. Let's assume that we have tracepoint at the function send_buff that prints it. So first just imagine how the trace buffer will look when burst of data is sent. But then on the other, if no data is sent then i will never have the chance to see the counter. This is where i left the tracepoints and moved to HMP. > > >> >> If this is the correct method for this purpose then let me know and i'll > >> >> update the git log message accordingly.
Yuval Shaia <yuval.shaia@oracle.com> writes: > On Mon, Feb 04, 2019 at 09:23:49AM +0100, Markus Armbruster wrote: >> Yuval Shaia <yuval.shaia@oracle.com> writes: >> >> > On Fri, Feb 01, 2019 at 08:33:44AM +0100, Markus Armbruster wrote: >> >> Eric Blake <eblake@redhat.com> 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 <yuval.shaia@oracle.com> >> >> >>>> --- >> >> >>>> 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. >> >> You were right to ask 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. >> >> For me, there are four cases of the "get information from QEMU to the >> user": >> >> 1. Information that is of use only for developers >> >> a. When tracing works, use tracing >> >> b. When it doesn't, we can consider QMP + HMP command, or HMP command >> only. A QMP command would carry an x- prefix to mark it unstable. >> >> 2. Information that is of use only for human users >> >> Provide an HMP command. >> >> 3. Information a management application such as libvirt wants to >> provide, but not monitor >> >> The QEMU part is just like for 4a below. The difference is that the >> management application doesn't poll automatically. >> >> 4. Information a management application such as libvirt wants to monitor >> >> This is not the case here, but I mention it for completeness. >> >> a. The obvious way to monitor is regular polling via QMP. Provide a >> QMP command to poll. >> >> b. Another way is tracking a QMP event that reports changes, plus >> polling on reconnect. This is generally more efficient. Provide >> a QMP event tracking changes, and a command to poll. The event >> may have to be rate-limited. >> >> If the information is also useful for human users, throw in a command >> to poll via HMP. >> >> I'm not yet sure tracing doesn't work for your use case. I replied >> to your claim it's not effective upthread. Let's discuss it there. > > We conclude there, and correct me if i misunderstood you, that for > 'polling' it make sense to use HMP only. Not in general; case 3. exists. But as long as there's no interest in plumbing the polling through a management application or debugging tools, HMP only is okay. >> >> 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. >> >> Sounds like you can't think of a use other than debugging. That means >> we should think harder about using tracepoints. > > Here is a case where tracepoint cannot help. > Consider a counter that counts the number of sent buffers. Now, this > counter get increased on every buffer that is sent. Let's assume that we > have tracepoint at the function send_buff that prints it. So first just > imagine how the trace buffer will look when burst of data is sent. But then > on the other, if no data is sent then i will never have the chance to see > the counter. > > This is where i left the tracepoints and moved to HMP. "Tracepoints cannot help" is factually incorrect. Tracepoints *can* count, but it takes a sufficiently sophisticated backend such as DTrace or SystemTap. For an example, check out https://sourceware.org/systemtap/examples/#network/netfilter_summary.stp With a query command, you bake the counting into the code. You can count exactly what the developer thought you'll want to count. With tracepoints, you can count things the developer has never thought of. Finally, there's efficiency. Efficiency rarely really matters, but when it matters, you better know how to get it. Tracepoints can be more efficient when tracing is off, and DTrace / Systemtap should be competitively efficient when tracing is on. Mind, the above isn't an attempt to shoot down your patch. It's an attempt to correct misconceptions about tracing! Your patch may be useful all the same. Since your patch's stated purpose is to help with debugging PVRDMA, the PVRDMA maintainers (Marcel and you) get to decide whether it's useful for debugging PVRDMA. The HMP maintainer (David) gets to decide whether to accept your debugging code into master. All the QMP maintainer (me) gets to decide is whether to demand a QMP command (right now I don't), and whether the commit message adequately explains the lack of a QMP command (so far it doesn't, but that shouldn't be hard to fix).
diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx index cbee8b944d..32f6215619 100644 --- a/hmp-commands-info.hx +++ b/hmp-commands-info.hx @@ -522,6 +522,20 @@ STEXI @item info cpustats @findex info cpustats Show CPU statistics. +ETEXI + + { + .name = "pvrdmastats", + .args_type = "", + .params = "", + .help = "show pvrdma device statistics", + .cmd = hmp_info_pvrdmastats, + }, + +STEXI +@item info pvrdmastats +@findex info pvrdmastats +Show pvrdma device statistics. ETEXI #if defined(CONFIG_SLIRP) diff --git a/monitor.c b/monitor.c index eb39fb015b..7a96c02438 100644 --- a/monitor.c +++ b/monitor.c @@ -84,6 +84,7 @@ #include "sysemu/iothread.h" #include "qemu/cutils.h" #include "tcg/tcg.h" +#include "hw/rdma/vmw/pvrdma.h" #if defined(TARGET_S390X) #include "hw/s390x/storage-keys.h" @@ -1399,6 +1400,11 @@ static void hmp_info_cpustats(Monitor *mon, const QDict *qdict) cpu_dump_statistics(cs, (FILE *)mon, &monitor_fprintf, 0); } +static void hmp_info_pvrdmastats(Monitor *mon, const QDict *qdict) +{ + pvrdma_dump_statistics((FILE *)mon, &monitor_fprintf); +} + static void hmp_info_trace_events(Monitor *mon, const QDict *qdict) { const char *name = qdict_get_try_str(qdict, "name");
Signed-off-by: Yuval Shaia <yuval.shaia@oracle.com> --- hmp-commands-info.hx | 14 ++++++++++++++ monitor.c | 6 ++++++ 2 files changed, 20 insertions(+)