[PATCH] trace-cmd: fix kvm_mmu_prepare_zap_page even name and kvm_mmu_get_page event output in kvm plugin

[PATCH] trace-cmd: fix kvm_mmu_prepare_zap_page even name and kvm_mmu_get_page event output in kvm plugin

[Gleb Natapov]

kvm_mmu_zap_page even was renamed to kvm_mmu_prepare_zap_page.
Print out created field for kvm_mmu_get_page event.

Signed-off-by: Gleb Natapov <gleb@redhat.com>
diff --git a/plugin_kvm.c b/plugin_kvm.c
index 55812ef..adc5694 100644
--- a/plugin_kvm.c
+++ b/plugin_kvm.c
@@ -382,7 +382,7 @@ static int kvm_mmu_print_role(struct trace_seq *s, struct pevent_record *record,
 	} else
 		trace_seq_printf(s, "WORD: %08x", role.word);
 
-	pevent_print_num_field(s, " root %u",  event,
+	pevent_print_num_field(s, " root %u ",  event,
 			       "root_count", record, 1);
 
 	if (pevent_get_field_val(s, event, "unsync", record, &val, 1) < 0)
@@ -397,6 +397,11 @@ static int kvm_mmu_get_page_handler(struct trace_seq *s, struct pevent_record *r
 {
 	unsigned long long val;
 
+	if (pevent_get_field_val(s, event, "created", record, &val, 1) < 0)
+		return -1;
+
+	trace_seq_printf(s, "%s ", val ? "new" : "existing");
+
 	if (pevent_get_field_val(s, event, "gfn", record, &val, 1) < 0)
 		return -1;
 
@@ -430,7 +435,7 @@ int PEVENT_PLUGIN_LOADER(struct pevent *pevent)
 	pevent_register_event_handler(pevent, -1, "kvmmmu", "kvm_mmu_unsync_page",
 				      kvm_mmu_print_role, NULL);
 
-	pevent_register_event_handler(pevent, -1, "kvmmmu", "kvm_mmu_zap_page",
+	pevent_register_event_handler(pevent, -1, "kvmmmu", "kvm_mmu_prepare_zap_page",
 				      kvm_mmu_print_role, NULL);
 
 	return 0;
--
			Gleb.

Re: [PATCH] trace-cmd: fix kvm_mmu_prepare_zap_page even name and kvm_mmu_get_page event output in kvm plugin

[Steven Rostedt]

On Tue, 2012-12-25 at 13:46 +0200, Gleb Natapov wrote:
> kvm_mmu_zap_page even was renamed to kvm_mmu_prepare_zap_page.
> Print out created field for kvm_mmu_get_page event.

trace-cmd needs to be backward compatible with older kernels. If older
kernels used kvm_mmu_zap_page, then please add a check for that and have
the plugin cope with either one.

Thanks,

-- Steve

> 
> Signed-off-by: Gleb Natapov <gleb@redhat.com>
> diff --git a/plugin_kvm.c b/plugin_kvm.c
> index 55812ef..adc5694 100644
> --- a/plugin_kvm.c
> +++ b/plugin_kvm.c
> @@ -382,7 +382,7 @@ static int kvm_mmu_print_role(struct trace_seq *s, struct pevent_record *record,
>  	} else
>  		trace_seq_printf(s, "WORD: %08x", role.word);
>  
> -	pevent_print_num_field(s, " root %u",  event,
> +	pevent_print_num_field(s, " root %u ",  event,
>  			       "root_count", record, 1);
>  
>  	if (pevent_get_field_val(s, event, "unsync", record, &val, 1) < 0)
> @@ -397,6 +397,11 @@ static int kvm_mmu_get_page_handler(struct trace_seq *s, struct pevent_record *r
>  {
>  	unsigned long long val;
>  
> +	if (pevent_get_field_val(s, event, "created", record, &val, 1) < 0)
> +		return -1;
> +
> +	trace_seq_printf(s, "%s ", val ? "new" : "existing");
> +
>  	if (pevent_get_field_val(s, event, "gfn", record, &val, 1) < 0)
>  		return -1;
>  
> @@ -430,7 +435,7 @@ int PEVENT_PLUGIN_LOADER(struct pevent *pevent)
>  	pevent_register_event_handler(pevent, -1, "kvmmmu", "kvm_mmu_unsync_page",
>  				      kvm_mmu_print_role, NULL);
>  
> -	pevent_register_event_handler(pevent, -1, "kvmmmu", "kvm_mmu_zap_page",
> +	pevent_register_event_handler(pevent, -1, "kvmmmu", "kvm_mmu_prepare_zap_page",
>  				      kvm_mmu_print_role, NULL);
>  
>  	return 0;
> --
> 			Gleb.

Re: [PATCH] trace-cmd: fix kvm_mmu_prepare_zap_page even name and kvm_mmu_get_page event output in kvm plugin

[Gleb Natapov]

On Wed, Dec 26, 2012 at 08:58:30AM -0500, Steven Rostedt wrote:
> On Tue, 2012-12-25 at 13:46 +0200, Gleb Natapov wrote:
> > kvm_mmu_zap_page even was renamed to kvm_mmu_prepare_zap_page.
> > Print out created field for kvm_mmu_get_page event.
> 
> trace-cmd needs to be backward compatible with older kernels. If older
> kernels used kvm_mmu_zap_page, then please add a check for that and have
> the plugin cope with either one.
> 
Something like this?

        if (pevent_find_event_by_name(pevent, "kvmmmu", "kvm_mmu_prepare_zap_page"))
                pevent_register_event_handler(pevent, -1, "kvmmmu",
                                "kvm_mmu_prepare_zap_page", kvm_mmu_print_role,
                                NULL);
        else
                pevent_register_event_handler(pevent, -1, "kvmmmu",
                                "kvm_mmu_zap_page", kvm_mmu_print_role, NULL);

Also when trace-cmd encounters an event it does not recognize it prints
mysterious message:

trace-cmd: No such file or directory
  bad op token {

Is this a bug?

> Thanks,
> 
> -- Steve
> 
> > 
> > Signed-off-by: Gleb Natapov <gleb@redhat.com>
> > diff --git a/plugin_kvm.c b/plugin_kvm.c
> > index 55812ef..adc5694 100644
> > --- a/plugin_kvm.c
> > +++ b/plugin_kvm.c
> > @@ -382,7 +382,7 @@ static int kvm_mmu_print_role(struct trace_seq *s, struct pevent_record *record,
> >  	} else
> >  		trace_seq_printf(s, "WORD: %08x", role.word);
> >  
> > -	pevent_print_num_field(s, " root %u",  event,
> > +	pevent_print_num_field(s, " root %u ",  event,
> >  			       "root_count", record, 1);
> >  
> >  	if (pevent_get_field_val(s, event, "unsync", record, &val, 1) < 0)
> > @@ -397,6 +397,11 @@ static int kvm_mmu_get_page_handler(struct trace_seq *s, struct pevent_record *r
> >  {
> >  	unsigned long long val;
> >  
> > +	if (pevent_get_field_val(s, event, "created", record, &val, 1) < 0)
> > +		return -1;
> > +
> > +	trace_seq_printf(s, "%s ", val ? "new" : "existing");
> > +
> >  	if (pevent_get_field_val(s, event, "gfn", record, &val, 1) < 0)
> >  		return -1;
> >  
> > @@ -430,7 +435,7 @@ int PEVENT_PLUGIN_LOADER(struct pevent *pevent)
> >  	pevent_register_event_handler(pevent, -1, "kvmmmu", "kvm_mmu_unsync_page",
> >  				      kvm_mmu_print_role, NULL);
> >  
> > -	pevent_register_event_handler(pevent, -1, "kvmmmu", "kvm_mmu_zap_page",
> > +	pevent_register_event_handler(pevent, -1, "kvmmmu", "kvm_mmu_prepare_zap_page",
> >  				      kvm_mmu_print_role, NULL);
> >  
> >  	return 0;
> > --
> > 			Gleb.
> 

--
			Gleb.

Re: [PATCH] trace-cmd: fix kvm_mmu_prepare_zap_page even name and kvm_mmu_get_page event output in kvm plugin

[Steven Rostedt]

On Wed, 2012-12-26 at 16:13 +0200, Gleb Natapov wrote:
> On Wed, Dec 26, 2012 at 08:58:30AM -0500, Steven Rostedt wrote:
> > On Tue, 2012-12-25 at 13:46 +0200, Gleb Natapov wrote:
> > > kvm_mmu_zap_page even was renamed to kvm_mmu_prepare_zap_page.
> > > Print out created field for kvm_mmu_get_page event.
> > 
> > trace-cmd needs to be backward compatible with older kernels. If older
> > kernels used kvm_mmu_zap_page, then please add a check for that and have
> > the plugin cope with either one.
> > 
> Something like this?
> 
>         if (pevent_find_event_by_name(pevent, "kvmmmu", "kvm_mmu_prepare_zap_page"))
>                 pevent_register_event_handler(pevent, -1, "kvmmmu",
>                                 "kvm_mmu_prepare_zap_page", kvm_mmu_print_role,
>                                 NULL);
>         else
>                 pevent_register_event_handler(pevent, -1, "kvmmmu",
>                                 "kvm_mmu_zap_page", kvm_mmu_print_role, NULL);

Sure, if it works.

> 
> Also when trace-cmd encounters an event it does not recognize it prints
> mysterious message:
> 
> trace-cmd: No such file or directory
>   bad op token {
> 
> Is this a bug?
> 

Yes and no ;-)

I need to get rid of the "perror" part, for errors that don't set errno.
That's been on my todo list for a long time. Maybe when I come back to
work next week I'll fix that.

The 'bad op token {' happens when it tries to parse an event and it
comes across something that it doesn't recognize. In this case a '{'.

The kvm events are notorious with having extremely complex "print_fmt"
fields in their event format files. When no event handler is registered
for an event, trace-cmd uses the print_fmt to figure out how to print
it. This was never meant to be too complex of a parser. For example,
looking at my kvm_mmu_prepare_zap_page, we have:

print fmt: "%s", ({ const char *ret = p->buffer + p->len; static const char *access_str[] = 
{ "---", "--x", "w--", "w-x", "-u-", "-ux", "wu-", "wux" }; union kvm_mmu_page_role role;
 role.word = REC->role; trace_seq_printf(p, "sp gfn %llx %u%s q%u%s %s%s" " %snxe root %u
 %s%c", REC->gfn, role.level, role.cr4_pae ? " pae" : "", role.quadrant, role.direct ? " 
direct" : "", access_str[role.access], role.invalid ? " invalid" : "", role.nxe ? "" : "!",
 REC->root_count, REC->unsync ? "unsync" : "sync", 0); ret; })

trace-cmd has no idea on how to parse "({ const char *ret ..." in fact
it dies on that first "{" because trace-cmd is not a full C parser.

This was why the plugins were created in the first place. To handle
various events that have too complex print_fmts for trace-cmd to
understand.

Most events are simple print_fmts and do not need plugins. Like
sock_rcvqueue_full:

print fmt: "rmem_alloc=%d truesize=%u sk_rcvbuf=%d", REC->rmem_alloc,
REC->truesize, REC->sk_rcvbuf

trace-cmd has no problem parsing events like that.

-- Steve

Re: [PATCH] trace-cmd: fix kvm_mmu_prepare_zap_page even name and kvm_mmu_get_page event output in kvm plugin

[Gleb Natapov]

On Wed, Dec 26, 2012 at 09:22:32AM -0500, Steven Rostedt wrote:
> On Wed, 2012-12-26 at 16:13 +0200, Gleb Natapov wrote:
> > On Wed, Dec 26, 2012 at 08:58:30AM -0500, Steven Rostedt wrote:
> > > On Tue, 2012-12-25 at 13:46 +0200, Gleb Natapov wrote:
> > > > kvm_mmu_zap_page even was renamed to kvm_mmu_prepare_zap_page.
> > > > Print out created field for kvm_mmu_get_page event.
> > > 
> > > trace-cmd needs to be backward compatible with older kernels. If older
> > > kernels used kvm_mmu_zap_page, then please add a check for that and have
> > > the plugin cope with either one.
> > > 
> > Something like this?
> > 
> >         if (pevent_find_event_by_name(pevent, "kvmmmu", "kvm_mmu_prepare_zap_page"))
> >                 pevent_register_event_handler(pevent, -1, "kvmmmu",
> >                                 "kvm_mmu_prepare_zap_page", kvm_mmu_print_role,
> >                                 NULL);
> >         else
> >                 pevent_register_event_handler(pevent, -1, "kvmmmu",
> >                                 "kvm_mmu_zap_page", kvm_mmu_print_role, NULL);
> 
> Sure, if it works.
> 
Well, it isn't and if it were it would have prevented tarce-cmd running
on new kernel from reading traces generated on older kernels. I will
register both.

> > 
> > Also when trace-cmd encounters an event it does not recognize it prints
> > mysterious message:
> > 
> > trace-cmd: No such file or directory
> >   bad op token {
> > 
> > Is this a bug?
> > 
> 
> Yes and no ;-)
> 
> I need to get rid of the "perror" part, for errors that don't set errno.
> That's been on my todo list for a long time. Maybe when I come back to
> work next week I'll fix that.
> 
> The 'bad op token {' happens when it tries to parse an event and it
> comes across something that it doesn't recognize. In this case a '{'.
> 
> The kvm events are notorious with having extremely complex "print_fmt"
> fields in their event format files. When no event handler is registered
> for an event, trace-cmd uses the print_fmt to figure out how to print
> it. This was never meant to be too complex of a parser. For example,
> looking at my kvm_mmu_prepare_zap_page, we have:
> 
> print fmt: "%s", ({ const char *ret = p->buffer + p->len; static const char *access_str[] = 
> { "---", "--x", "w--", "w-x", "-u-", "-ux", "wu-", "wux" }; union kvm_mmu_page_role role;
>  role.word = REC->role; trace_seq_printf(p, "sp gfn %llx %u%s q%u%s %s%s" " %snxe root %u
>  %s%c", REC->gfn, role.level, role.cr4_pae ? " pae" : "", role.quadrant, role.direct ? " 
> direct" : "", access_str[role.access], role.invalid ? " invalid" : "", role.nxe ? "" : "!",
>  REC->root_count, REC->unsync ? "unsync" : "sync", 0); ret; })
> 
> trace-cmd has no idea on how to parse "({ const char *ret ..." in fact
> it dies on that first "{" because trace-cmd is not a full C parser.
> 
> This was why the plugins were created in the first place. To handle
> various events that have too complex print_fmts for trace-cmd to
> understand.
> 
> Most events are simple print_fmts and do not need plugins. Like
> sock_rcvqueue_full:
> 
> print fmt: "rmem_alloc=%d truesize=%u sk_rcvbuf=%d", REC->rmem_alloc,
> REC->truesize, REC->sk_rcvbuf
> 
> trace-cmd has no problem parsing events like that.
> 
Got it.

--
			Gleb.