[Qemu-devel] [PATCH v2 1/1] monitor: improve tracing in handle_qmp_command

[Qemu-devel] [PATCH v2 1/1] monitor: improve tracing in handle_qmp_command

[Denis V. Lunev]

Calculate req_json only if trace_handle_qmp_command enabled.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Stefan Hajnoczi <stefanha@redhat.com>
CC: Lluís Vilanova <vilanova@ac.upc.edu>
CC: Dr. David Alan Gilbert <dgilbert@redhat.com>
CC: Markus Armbruster <armbru@redhat.com>
---
Changes from v1:
- written in the explicit for, as discussed in the mailing list

 monitor.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/monitor.c b/monitor.c
index d8ac20f6ca..2bfeb9bbcc 100644
--- a/monitor.c
+++ b/monitor.c
@@ -3822,7 +3822,6 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
     QDict *qdict = NULL;
     Monitor *mon = cur_mon;
     Error *err = NULL;
-    QString *req_json;
 
     req = json_parser_parse_err(tokens, NULL, &err);
     if (!req && !err) {
@@ -3840,9 +3839,11 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
         qdict_del(qdict, "id");
     } /* else will fail qmp_dispatch() */
 
-    req_json = qobject_to_json(req);
-    trace_handle_qmp_command(mon, qstring_get_str(req_json));
-    qobject_decref(QOBJECT(req_json));
+    if (trace_event_get_state(TRACE_HANDLE_QMP_COMMAND)) {
+        QString *req_json = qobject_to_json(req);
+        trace_handle_qmp_command(mon, qstring_get_str(req_json));
+        qobject_decref(QOBJECT(req_json));
+    }
 
     rsp = qmp_dispatch(cur_mon->qmp.commands, req);
 
-- 
2.11.0

Re: [Qemu-devel] [PATCH v2 1/1] monitor: improve tracing in handle_qmp_command

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1924 bytes --]

On 07/25/2017 09:39 AM, Denis V. Lunev wrote:
> Calculate req_json only if trace_handle_qmp_command enabled.
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> CC: Lluís Vilanova <vilanova@ac.upc.edu>
> CC: Dr. David Alan Gilbert <dgilbert@redhat.com>
> CC: Markus Armbruster <armbru@redhat.com>
> ---
> Changes from v1:
> - written in the explicit for, as discussed in the mailing list

I could live with this being considered a bug-fix for 2.10, as we
regressed in speed/memory usage during normal untraced QMP commands.

Reviewed-by: Eric Blake <eblake@redhat.com>

> 
>  monitor.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/monitor.c b/monitor.c
> index d8ac20f6ca..2bfeb9bbcc 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -3822,7 +3822,6 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
>      QDict *qdict = NULL;
>      Monitor *mon = cur_mon;
>      Error *err = NULL;
> -    QString *req_json;
>  
>      req = json_parser_parse_err(tokens, NULL, &err);
>      if (!req && !err) {
> @@ -3840,9 +3839,11 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
>          qdict_del(qdict, "id");
>      } /* else will fail qmp_dispatch() */
>  
> -    req_json = qobject_to_json(req);
> -    trace_handle_qmp_command(mon, qstring_get_str(req_json));
> -    qobject_decref(QOBJECT(req_json));
> +    if (trace_event_get_state(TRACE_HANDLE_QMP_COMMAND)) {
> +        QString *req_json = qobject_to_json(req);
> +        trace_handle_qmp_command(mon, qstring_get_str(req_json));
> +        qobject_decref(QOBJECT(req_json));
> +    }
>  
>      rsp = qmp_dispatch(cur_mon->qmp.commands, req);
>  
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH v2 1/1] monitor: improve tracing in handle_qmp_command

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1908 bytes --]

On Tue, Jul 25, 2017 at 05:39:23PM +0300, Denis V. Lunev wrote:
> Calculate req_json only if trace_handle_qmp_command enabled.
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> CC: Lluís Vilanova <vilanova@ac.upc.edu>
> CC: Dr. David Alan Gilbert <dgilbert@redhat.com>
> CC: Markus Armbruster <armbru@redhat.com>
> ---
> Changes from v1:
> - written in the explicit for, as discussed in the mailing list
> 
>  monitor.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/monitor.c b/monitor.c
> index d8ac20f6ca..2bfeb9bbcc 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -3822,7 +3822,6 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
>      QDict *qdict = NULL;
>      Monitor *mon = cur_mon;
>      Error *err = NULL;
> -    QString *req_json;
>  
>      req = json_parser_parse_err(tokens, NULL, &err);
>      if (!req && !err) {
> @@ -3840,9 +3839,11 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
>          qdict_del(qdict, "id");
>      } /* else will fail qmp_dispatch() */
>  
> -    req_json = qobject_to_json(req);
> -    trace_handle_qmp_command(mon, qstring_get_str(req_json));
> -    qobject_decref(QOBJECT(req_json));
> +    if (trace_event_get_state(TRACE_HANDLE_QMP_COMMAND)) {
> +        QString *req_json = qobject_to_json(req);
> +        trace_handle_qmp_command(mon, qstring_get_str(req_json));
> +        qobject_decref(QOBJECT(req_json));
> +    }
>  
>      rsp = qmp_dispatch(cur_mon->qmp.commands, req);

Thanks for the patch.  Another fix is needed first so that SystemTap and
LTTng UST users can use this trace event.  Currently
trace_event_get_state(TRACE_HANDLE_QMP_COMMAND) will return false
because SystemTap and LTTng UST maintain their own state.  I have CCed
you on the fix.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH v2 1/1] monitor: improve tracing in handle_qmp_command

[Markus Armbruster]

"Denis V. Lunev" <den@openvz.org> writes:

> Calculate req_json only if trace_handle_qmp_command enabled.
>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> CC: Lluís Vilanova <vilanova@ac.upc.edu>
> CC: Dr. David Alan Gilbert <dgilbert@redhat.com>
> CC: Markus Armbruster <armbru@redhat.com>
> ---
> Changes from v1:
> - written in the explicit for, as discussed in the mailing list
>
>  monitor.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/monitor.c b/monitor.c
> index d8ac20f6ca..2bfeb9bbcc 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -3822,7 +3822,6 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
>      QDict *qdict = NULL;
>      Monitor *mon = cur_mon;
>      Error *err = NULL;
> -    QString *req_json;
>  
>      req = json_parser_parse_err(tokens, NULL, &err);
>      if (!req && !err) {
> @@ -3840,9 +3839,11 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
>          qdict_del(qdict, "id");
>      } /* else will fail qmp_dispatch() */
>  
> -    req_json = qobject_to_json(req);
> -    trace_handle_qmp_command(mon, qstring_get_str(req_json));
> -    qobject_decref(QOBJECT(req_json));
> +    if (trace_event_get_state(TRACE_HANDLE_QMP_COMMAND)) {
> +        QString *req_json = qobject_to_json(req);
> +        trace_handle_qmp_command(mon, qstring_get_str(req_json));
> +        qobject_decref(QOBJECT(req_json));
> +    }
>  
>      rsp = qmp_dispatch(cur_mon->qmp.commands, req);

Conflicts with Eric's commit 8a4613a, but I can resolve that in my tree.

The commit message is too terse.  "Improve tracing" makes me think of
more informative traces, but that's not the case.  What exactly is
improved here?

Re: [Qemu-devel] [PATCH v2 1/1] monitor: improve tracing in handle_qmp_command

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1046 bytes --]

On 07/28/2017 12:01 PM, Markus Armbruster wrote:
> "Denis V. Lunev" <den@openvz.org> writes:
> 
>> Calculate req_json only if trace_handle_qmp_command enabled.
>>
>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>> CC: Stefan Hajnoczi <stefanha@redhat.com>
>> CC: Lluís Vilanova <vilanova@ac.upc.edu>
>> CC: Dr. David Alan Gilbert <dgilbert@redhat.com>
>> CC: Markus Armbruster <armbru@redhat.com>
>> ---

> Conflicts with Eric's commit 8a4613a, but I can resolve that in my tree.
> 
> The commit message is too terse.  "Improve tracing" makes me think of
> more informative traces, but that's not the case.  What exactly is
> improved here?

Reduce overhead when not tracing.  Without the patch, we are malloc'ing
a QString and spending CPU cycles on converting a QObject to string,
just for the sake of sticking the string in the trace message - if we
aren't tracing, it's wasted effort.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH v2 1/1] monitor: improve tracing in handle_qmp_command

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 07/28/2017 12:01 PM, Markus Armbruster wrote:
>> "Denis V. Lunev" <den@openvz.org> writes:
>> 
>>> Calculate req_json only if trace_handle_qmp_command enabled.
>>>
>>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>>> CC: Stefan Hajnoczi <stefanha@redhat.com>
>>> CC: Lluís Vilanova <vilanova@ac.upc.edu>
>>> CC: Dr. David Alan Gilbert <dgilbert@redhat.com>
>>> CC: Markus Armbruster <armbru@redhat.com>
>>> ---
>
>> Conflicts with Eric's commit 8a4613a, but I can resolve that in my tree.
>> 
>> The commit message is too terse.  "Improve tracing" makes me think of
>> more informative traces, but that's not the case.  What exactly is
>> improved here?
>
> Reduce overhead when not tracing.  Without the patch, we are malloc'ing
> a QString and spending CPU cycles on converting a QObject to string,
> just for the sake of sticking the string in the trace message - if we
> aren't tracing, it's wasted effort.

What about:

    monitor: Reduce handle_qmp_command() tracing overhead

    We are malloc'ing a QString and spending CPU cycles on converting a
    QObject to string, just for the sake of sticking the string in the
    trace message.  Wasted when we aren't tracing.  Avoid that.

Denis?

Re: [Qemu-devel] [PATCH v2 1/1] monitor: improve tracing in handle_qmp_command

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 774 bytes --]

On Tue, Jul 25, 2017 at 05:39:23PM +0300, Denis V. Lunev wrote:
> Calculate req_json only if trace_handle_qmp_command enabled.
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> CC: Lluís Vilanova <vilanova@ac.upc.edu>
> CC: Dr. David Alan Gilbert <dgilbert@redhat.com>
> CC: Markus Armbruster <armbru@redhat.com>
> ---
> Changes from v1:
> - written in the explicit for, as discussed in the mailing list
> 
>  monitor.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)

Replaced commit message & description with the one Markus suggested to
include more details on why this patch is necessary.

Thanks, applied to my tracing tree:
https://github.com/stefanha/qemu/commits/tracing

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]