Another Obsolete Fix me in trace.h?

DISCUSSION Paolo Bonzini 4 participants 9 messages Nov 24, 2014 – Nov 26, 2014 1 message not in the archive KVM

lore

Share & tools

lore

Paolo Bonzini

author maintainer

2014-11-24 10:12 UTC #1 lore raw

In reply to a message not in the archive

Quoted 9 lines · nick

Greetings Again Gleb and others,
I am assuming in the code I am pasting below the fix me is obsolete now and I can remove it. :) Cheers Nick
TP_printk("%s (0x%x)",
                  __print_symbolic(__entry->exception, kvm_trace_sym_exc),
                   /* FIXME: don't print error_code if not present */
                  __entry->has_error ? __entry->error_code : 0)
);

No, it's not obsolete, the idea is to print only

instead of

%s (0x%x)

if __entry->has_error is false. I don't know the trace API well enough to know if that is possible.

Paolo

Jan Kiszka

2014-11-24 10:40 UTC #2 lore raw

Quoted 13 lines · Paolo Bonzini

Quoted 9 lines · nick
Greetings Again Gleb and others,
I am assuming in the code I am pasting below the fix me is obsolete now and I can remove it. :) Cheers Nick
TP_printk("%s (0x%x)",
                  __print_symbolic(__entry->exception, kvm_trace_sym_exc),
                   /* FIXME: don't print error_code if not present */
                  __entry->has_error ? __entry->error_code : 0)
);
No, it's not obsolete, the idea is to print only

Quoted 8 lines

   %s
instead of
   %s (0x%x)
if __entry->has_error is false. I don't know the trace API well enough to know if that is possible.

Last time I ran across such a scenario, it was not feasible and essentially required separate tracepoints. But maybe Steven knows a trick.

Jan

Signature · 2 lines

Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux

Radim Krčmář

2014-11-24 21:00 UTC #3 lore raw

2014-11-24 11:40+0100, Jan Kiszka:

Quoted 18 lines · Paolo Bonzini

Quoted 17 lines · nick
Quoted 9 lines
Greetings Again Gleb and others,
I am assuming in the code I am pasting below the fix me is obsolete now and I can remove it. :) Cheers Nick
TP_printk("%s (0x%x)",
                  __print_symbolic(__entry->exception, kvm_trace_sym_exc),
                   /* FIXME: don't print error_code if not present */
                  __entry->has_error ? __entry->error_code : 0)
);
No, it's not obsolete, the idea is to print only
   %s
instead of

Quoted 7 lines

Quoted 4 lines
   %s (0x%x)
if __entry->has_error is false. I don't know the trace API well enough to know if that is possible.
Last time I ran across such a scenario, it was not feasible and essentially required separate tracepoints. But maybe Steven knows a trick.

The format string has to be a string literal[1]; we could change it to allow expressions[2], but what we want is almost possible through a direct call to trace_seq_printf()[3].

The raw result would look like

  #define __print(fmt, args...) ({ \
  	const char *buf_start = trace_seq_buffer_ptr(p); \
  	trace_seq_printf(p, fmt, args); \
  	trace_seq_putc(p, '\0'); \
  	buf_start; \
  	})

TP_printk("%s%s", [...],

            __entry->has_error ? __print("(0x%x)", __entry->error_code) : "")

and would be acceptable if something __print-like made it into a ftrace helper[4]. (Userspace won't be able to nicely print it otherwise.)

---
1: #define TP_printk(fmt, args...) fmt "\n", args
2: TP_printk(__entry->has_error ? "%s (0x%x)" : "%s", [...] 3: Already in scsi_dispatch_cmd_start or kvm_mmu_get_page tracepoints. 4: Like __print_hex or print_symbolic.

Steven Rostedt

2014-11-24 21:19 UTC #4 lore raw

Quoted 40 lines · On Mon, 24 Nov 2014 22:00:01 +0100 Radim Krčmář <[email protected]> wrote:

2014-11-24 11:40+0100, Jan Kiszka:
Quoted 18 lines · Paolo Bonzini
Quoted 17 lines · nick
Quoted 9 lines
Greetings Again Gleb and others,
I am assuming in the code I am pasting below the fix me is obsolete now and I can remove it. :) Cheers Nick
TP_printk("%s (0x%x)",
                  __print_symbolic(__entry->exception, kvm_trace_sym_exc),
                   /* FIXME: don't print error_code if not present */
                  __entry->has_error ? __entry->error_code : 0)
);
No, it's not obsolete, the idea is to print only
   %s
instead of
Quoted 7 lines
Quoted 4 lines
   %s (0x%x)
if __entry->has_error is false. I don't know the trace API well enough to know if that is possible.
Last time I ran across such a scenario, it was not feasible and essentially required separate tracepoints. But maybe Steven knows a trick.
The format string has to be a string literal[1]; we could change it to allow expressions[2], but what we want is almost possible through a direct call to trace_seq_printf()[3].
The raw result would look like
  #define __print(fmt, args...) ({ \
  	const char *buf_start = trace_seq_buffer_ptr(p); \
  	trace_seq_printf(p, fmt, args); \
  	trace_seq_putc(p, '\0'); \
  	buf_start; \
  	})

Quoted 5 lines

TP_printk("%s%s", [...],
            __entry->has_error ? __print("(0x%x)", __entry->error_code) : "")
and would be acceptable if something __print-like made it into a ftrace helper[4]. (Userspace won't be able to nicely print it otherwise.)

You mean if we add something like a __print_conditional(cond, fmt, ...);

For this case you would have:

TP_printk("%s%s", [...],

	__print_conditional(__entry->has_error, " (0x%x)", __entry->error_code));

Where __print_conditional() will return "" when "cond" is false, and will return the formatted string otherwise.

That wouldn't be too hard to implement.

-- Steve

Quoted 7 lines

---
1: #define TP_printk(fmt, args...) fmt "\n", args
2: TP_printk(__entry->has_error ? "%s (0x%x)" : "%s", [...] 3: Already in scsi_dispatch_cmd_start or kvm_mmu_get_page tracepoints. 4: Like __print_hex or print_symbolic.

Radim Krčmář

2014-11-24 21:49 UTC #5 lore raw

2014-11-24 16:19-0500, Steven Rostedt:

Quoted 17 lines

Quoted 14 lines · On Mon, 24 Nov 2014 22:00:01 +0100 Radim Krčmář <[email protected]> wrote:
2014-11-24 11:40+0100, Jan Kiszka:
The format string has to be a string literal[1]; we could change it to allow expressions[2], but what we want is almost possible through a direct call to trace_seq_printf()[3].
The raw result would look like
  #define __print(fmt, args...) ({ \
  	const char *buf_start = trace_seq_buffer_ptr(p); \
  	trace_seq_printf(p, fmt, args); \
  	trace_seq_putc(p, '\0'); \
  	buf_start; \
  	})

Quoted 7 lines

Quoted 5 lines
TP_printk("%s%s", [...],
            __entry->has_error ? __print("(0x%x)", __entry->error_code) : "")
and would be acceptable if something __print-like made it into a ftrace helper[4]. (Userspace won't be able to nicely print it otherwise.)
You mean if we add something like a __print_conditional(cond, fmt, ...);

The benefit of _conditional is cleaner code?

(_conditional would be possible as a #define on top of generic print,
the ternary seems to be parsed correctly.)

Quoted 7 lines

For this case you would have:
TP_printk("%s%s", [...],
	__print_conditional(__entry->has_error, " (0x%x)", __entry->error_code));
Where __print_conditional() will return "" when "cond" is false, and will return the formatted string otherwise.

(This might introduce 'const char empty[] = ""'.)

Quoted 1 line

That wouldn't be too hard to implement.

I'll look at the patch tommorrow.

Thanks.

Radim Krčmář

2014-11-26 12:40 UTC #6 lore raw

2014-11-24 22:49+0100, Radim Krčmář:

Quoted 4 lines

2014-11-24 16:19-0500, Steven Rostedt:
Quoted 1 line
That wouldn't be too hard to implement.
I'll look at the patch tommorrow.

The kernel part is trivial.
Most of the code is going to be in tools/lib/traceevent/event-parse.c.

I wasn't sure whether to
1) define new 'enum print_arg_type' for our function and do exactly what other __print* did
2) extend existing PRINT_FUNC with variable arguments and register it as a "global" event handler

as (2) makes more sense to me, but we already had it when some __print* was implemented ... (and I didn't want to dig too deep into the project)

Steven Rostedt

2014-11-26 14:15 UTC #7 lore raw

Quoted 9 lines · On Wed, 26 Nov 2014 13:40:26 +0100 Radim Krčmář <[email protected]> wrote:

2014-11-24 22:49+0100, Radim Krčmář:
Quoted 4 lines
2014-11-24 16:19-0500, Steven Rostedt:
Quoted 1 line
That wouldn't be too hard to implement.
I'll look at the patch tommorrow.
The kernel part is trivial.
Most of the code is going to be in tools/lib/traceevent/event-parse.c.

Quoted 8 lines

I wasn't sure whether to
1) define new 'enum print_arg_type' for our function and do exactly what other __print* did
2) extend existing PRINT_FUNC with variable arguments and register it as a "global" event handler
as (2) makes more sense to me, but we already had it when some __print* was implemented ... (and I didn't want to dig too deep into the project)

Add a new hardcoded instance. The PRINT_FUNC is for plugins that have a unique function for the events they handle. Notice that event-parse does not define any func handlers. If this is to be generic, then lets make it specified as a new enum.

Thanks,

-- Steve

Radim Krčmář

2014-11-26 14:49 UTC #8 lore raw

2014-11-26 09:15-0500, Steven Rostedt:

Quoted 11 lines

Quoted 9 lines · On Wed, 26 Nov 2014 13:40:26 +0100 Radim Krčmář <[email protected]> wrote:
The kernel part is trivial.
Most of the code is going to be in tools/lib/traceevent/event-parse.c.
I wasn't sure whether to
1) define new 'enum print_arg_type' for our function and do exactly what other __print* did
2) extend existing PRINT_FUNC with variable arguments and register it as a "global" event handler

Quoted 7 lines

Quoted 2 lines
as (2) makes more sense to me, but we already had it when some __print* was implemented ... (and I didn't want to dig too deep into the project)
Add a new hardcoded instance. The PRINT_FUNC is for plugins that have a unique function for the events they handle. Notice that event-parse does not define any func handlers. If this is to be generic, then lets make it specified as a new enum.

Ok, thanks.

(It just seemed weird that we have an infrastructure that could allow a
"generic plugin" to cover this functionality.)

Steven Rostedt

2014-11-26 15:23 UTC #9 lore raw

Quoted 2 lines · On Wed, 26 Nov 2014 15:49:34 +0100 Radim Krčmář <[email protected]> wrote:

(It just seemed weird that we have an infrastructure that could allow a
"generic plugin" to cover this functionality.)

Maybe in the future we may do something like that. But for now, think of these as "optimized" ;-)

-- Steve