* [Path -tip 1/3] Tracing/ftrace: Change the type of the print_line callback @ 2008-09-26 15:25 Frederic Weisbecker 2008-09-28 16:19 ` Pekka Paalanen 0 siblings, 1 reply; 5+ messages in thread From: Frederic Weisbecker @ 2008-09-26 15:25 UTC (permalink / raw) To: mingo; +Cc: rostedt, linux-kernel, pq We need a kind of disambiguation when a print_line callback returns 0. This value can significate both: _ There is not enough space to print all of the entry. Please flush the current seq and retry _ I can't handle this type of entry. Such a confusion can break the pipe. This patch changes the type of this callback for a better information. Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com> --- diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h index b28bf88..73cd7c5 100644 --- a/kernel/trace/trace.h +++ b/kernel/trace/trace.h @@ -180,6 +180,14 @@ struct trace_array { struct trace_array_cpu *data[NR_CPUS]; }; + +/* Return values for print_line callback */ +enum print_line_t { + TRACE_TYPE_PARTIAL_LINE = 0, /* Retry after flushing the seq */ + TRACE_TYPE_HANDLED = 1, + TRACE_TYPE_UNHANDLED = 2 /* Relay to other output functions */ +}; + /* * A specific tracer, represented by methods that operate on a trace array: */ @@ -200,7 +208,7 @@ struct tracer { int (*selftest)(struct tracer *trace, struct trace_array *tr); #endif - int (*print_line)(struct trace_iterator *iter); + enum print_line_t (*print_line)(struct trace_iterator *iter); struct tracer *next; int print_max; }; diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c index 50ac334..ca95ec3 100644 --- a/kernel/trace/trace.c +++ b/kernel/trace/trace.c @@ -1510,7 +1510,7 @@ void trace_seq_print_cont(struct trace_seq *s, struct trace_iterator *iter) trace_seq_putc(s, '\n'); } -static int +static enum print_line_t print_lat_fmt(struct trace_iterator *iter, unsigned int trace_idx, int cpu) { struct trace_seq *s = &iter->seq; @@ -1530,7 +1530,7 @@ print_lat_fmt(struct trace_iterator *iter, unsigned int trace_idx, int cpu) next_entry = entry; if (entry->type == TRACE_CONT) - return 1; + return TRACE_TYPE_HANDLED; rel_usecs = ns2usecs(next_entry->field.t - entry->field.t); abs_usecs = ns2usecs(entry->field.t - iter->tr->time_start); @@ -1601,10 +1601,10 @@ print_lat_fmt(struct trace_iterator *iter, unsigned int trace_idx, int cpu) default: trace_seq_printf(s, "Unknown type %d\n", entry->type); } - return 1; + return TRACE_TYPE_HANDLED; } -static int print_trace_fmt(struct trace_iterator *iter) +static enum print_line_t print_trace_fmt(struct trace_iterator *iter) { struct trace_seq *s = &iter->seq; unsigned long sym_flags = (trace_flags & TRACE_ITER_SYM_MASK); @@ -1621,7 +1621,7 @@ static int print_trace_fmt(struct trace_iterator *iter) entry = iter->ent; if (entry->type == TRACE_CONT) - return 1; + return TRACE_TYPE_HANDLED; field = &entry->field; @@ -1633,24 +1633,24 @@ static int print_trace_fmt(struct trace_iterator *iter) ret = trace_seq_printf(s, "%16s-%-5d ", comm, field->pid); if (!ret) - return 0; + return TRACE_TYPE_PARTIAL_LINE; ret = trace_seq_printf(s, "[%03d] ", iter->cpu); if (!ret) - return 0; + return TRACE_TYPE_PARTIAL_LINE; ret = trace_seq_printf(s, "%5lu.%06lu: ", secs, usec_rem); if (!ret) - return 0; + return TRACE_TYPE_PARTIAL_LINE; switch (entry->type) { case TRACE_FN: ret = seq_print_ip_sym(s, field->fn.ip, sym_flags); if (!ret) - return 0; + return TRACE_TYPE_PARTIAL_LINE; if ((sym_flags & TRACE_ITER_PRINT_PARENT) && field->fn.parent_ip) { ret = trace_seq_printf(s, " <-"); if (!ret) - return 0; + return TRACE_TYPE_PARTIAL_LINE; if (kretprobed(field->fn.parent_ip)) ret = trace_seq_puts(s, KRETPROBE_MSG); else @@ -1658,11 +1658,11 @@ static int print_trace_fmt(struct trace_iterator *iter) field->fn.parent_ip, sym_flags); if (!ret) - return 0; + return TRACE_TYPE_PARTIAL_LINE; } ret = trace_seq_printf(s, "\n"); if (!ret) - return 0; + return TRACE_TYPE_PARTIAL_LINE; break; case TRACE_CTX: case TRACE_WAKE: @@ -1680,7 +1680,7 @@ static int print_trace_fmt(struct trace_iterator *iter) field->ctx.next_prio, T); if (!ret) - return 0; + return TRACE_TYPE_PARTIAL_LINE; break; case TRACE_SPECIAL: ret = trace_seq_printf(s, "# %ld %ld %ld\n", @@ -1688,23 +1688,23 @@ static int print_trace_fmt(struct trace_iterator *iter) field->special.arg2, field->special.arg3); if (!ret) - return 0; + return TRACE_TYPE_PARTIAL_LINE; break; case TRACE_STACK: for (i = 0; i < FTRACE_STACK_ENTRIES; i++) { if (i) { ret = trace_seq_puts(s, " <= "); if (!ret) - return 0; + return TRACE_TYPE_PARTIAL_LINE; } ret = seq_print_ip_sym(s, field->stack.caller[i], sym_flags); if (!ret) - return 0; + return TRACE_TYPE_PARTIAL_LINE; } ret = trace_seq_puts(s, "\n"); if (!ret) - return 0; + return TRACE_TYPE_PARTIAL_LINE; break; case TRACE_PRINT: seq_print_ip_sym(s, field->print.ip, sym_flags); @@ -1713,10 +1713,10 @@ static int print_trace_fmt(struct trace_iterator *iter) trace_seq_print_cont(s, iter); break; } - return 1; + return TRACE_TYPE_HANDLED; } -static int print_raw_fmt(struct trace_iterator *iter) +static enum print_line_t print_raw_fmt(struct trace_iterator *iter) { struct trace_seq *s = &iter->seq; struct trace_entry *entry; @@ -1727,14 +1727,14 @@ static int print_raw_fmt(struct trace_iterator *iter) entry = iter->ent; if (entry->type == TRACE_CONT) - return 1; + return TRACE_TYPE_HANDLED; field = &entry->field; ret = trace_seq_printf(s, "%d %d %llu ", field->pid, iter->cpu, field->t); if (!ret) - return 0; + return TRACE_TYPE_PARTIAL_LINE; switch (entry->type) { case TRACE_FN: @@ -1742,7 +1742,7 @@ static int print_raw_fmt(struct trace_iterator *iter) field->fn.ip, field->fn.parent_ip); if (!ret) - return 0; + return TRACE_TYPE_PARTIAL_LINE; break; case TRACE_CTX: case TRACE_WAKE: @@ -1761,7 +1761,7 @@ static int print_raw_fmt(struct trace_iterator *iter) field->ctx.next_prio, T); if (!ret) - return 0; + return TRACE_TYPE_PARTIAL_LINE; break; case TRACE_SPECIAL: case TRACE_STACK: @@ -1770,7 +1770,7 @@ static int print_raw_fmt(struct trace_iterator *iter) field->special.arg2, field->special.arg3); if (!ret) - return 0; + return TRACE_TYPE_PARTIAL_LINE; break; case TRACE_PRINT: trace_seq_printf(s, "# %lx %s", @@ -1779,7 +1779,7 @@ static int print_raw_fmt(struct trace_iterator *iter) trace_seq_print_cont(s, iter); break; } - return 1; + return TRACE_TYPE_HANDLED; } #define SEQ_PUT_FIELD_RET(s, x) \ @@ -1794,7 +1794,7 @@ do { \ return 0; \ } while (0) -static int print_hex_fmt(struct trace_iterator *iter) +static enum print_line_t print_hex_fmt(struct trace_iterator *iter) { struct trace_seq *s = &iter->seq; unsigned char newline = '\n'; @@ -1805,7 +1805,7 @@ static int print_hex_fmt(struct trace_iterator *iter) entry = iter->ent; if (entry->type == TRACE_CONT) - return 1; + return TRACE_TYPE_HANDLED; field = &entry->field; @@ -1843,10 +1843,10 @@ static int print_hex_fmt(struct trace_iterator *iter) } SEQ_PUT_FIELD_RET(s, newline); - return 1; + return TRACE_TYPE_HANDLED; } -static int print_bin_fmt(struct trace_iterator *iter) +static enum print_line_t print_bin_fmt(struct trace_iterator *iter) { struct trace_seq *s = &iter->seq; struct trace_entry *entry; @@ -1855,7 +1855,7 @@ static int print_bin_fmt(struct trace_iterator *iter) entry = iter->ent; if (entry->type == TRACE_CONT) - return 1; + return TRACE_TYPE_HANDLED; field = &entry->field; @@ -1883,7 +1883,7 @@ static int print_bin_fmt(struct trace_iterator *iter) SEQ_PUT_FIELD_RET(s, field->special.arg3); break; } - return 1; + return TRACE_TYPE_HANDLED; } static int trace_empty(struct trace_iterator *iter) @@ -1904,27 +1904,42 @@ static int trace_empty(struct trace_iterator *iter) static int print_trace_line(struct trace_iterator *iter) { - int ret; + enum print_line_t ret; - if (iter->trace && iter->trace->print_line) - if ((ret = iter->trace->print_line(iter))) + if (iter->trace && iter->trace->print_line) { + ret = iter->trace->print_line(iter); + if (ret == TRACE_TYPE_HANDLED || + ret == TRACE_TYPE_PARTIAL_LINE) return ret; + } - if (trace_flags & TRACE_ITER_BIN) - if ((ret = print_bin_fmt(iter))) + if (trace_flags & TRACE_ITER_BIN) { + ret = print_bin_fmt(iter); + if (ret == TRACE_TYPE_HANDLED || + ret == TRACE_TYPE_PARTIAL_LINE) return ret; + } - if (trace_flags & TRACE_ITER_HEX) - if ((ret = print_hex_fmt(iter))) + if (trace_flags & TRACE_ITER_HEX) { + ret = print_hex_fmt(iter); + if (ret == TRACE_TYPE_HANDLED || + ret == TRACE_TYPE_PARTIAL_LINE) return ret; + } - if (trace_flags & TRACE_ITER_RAW) - if ((ret = print_raw_fmt(iter))) + if (trace_flags & TRACE_ITER_RAW) { + ret = print_raw_fmt(iter); + if (ret == TRACE_TYPE_HANDLED || + ret == TRACE_TYPE_PARTIAL_LINE) return ret; + } - if (iter->iter_flags & TRACE_FILE_LAT_FMT) - if ((ret = print_lat_fmt(iter, iter->idx, iter->cpu))) + if (iter->iter_flags & TRACE_FILE_LAT_FMT) { + ret = print_lat_fmt(iter, iter->idx, iter->cpu); + if (ret == TRACE_TYPE_HANDLED || + ret == TRACE_TYPE_PARTIAL_LINE) return ret; + } return print_trace_fmt(iter); } @@ -2722,11 +2737,11 @@ tracing_read_pipe(struct file *filp, char __user *ubuf, } while (find_next_entry_inc(iter) != NULL) { - int ret; + enum print_line_t ret; int len = iter->seq.len; ret = print_trace_line(iter); - if (!ret) { + if (ret == TRACE_TYPE_PARTIAL_LINE) { /* don't print partial lines */ iter->seq.len = len; break; @@ -2738,6 +2753,10 @@ tracing_read_pipe(struct file *filp, char __user *ubuf, break; } + if (!iter->seq.len) + /* Don't break the pipe */ + trace_seq_puts(&iter->seq, "Warning: empty seq\n"); + for_each_cpu_mask(cpu, mask) { data = iter->tr->data[cpu]; __raw_spin_unlock(&data->lock); ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [Path -tip 1/3] Tracing/ftrace: Change the type of the print_line callback 2008-09-26 15:25 [Path -tip 1/3] Tracing/ftrace: Change the type of the print_line callback Frederic Weisbecker @ 2008-09-28 16:19 ` Pekka Paalanen 2008-09-29 9:11 ` Frédéric Weisbecker 0 siblings, 1 reply; 5+ messages in thread From: Pekka Paalanen @ 2008-09-28 16:19 UTC (permalink / raw) To: Frederic Weisbecker; +Cc: mingo, rostedt, linux-kernel On Fri, 26 Sep 2008 17:25:21 +0200 Frederic Weisbecker <fweisbec@gmail.com> wrote: > We need a kind of disambiguation when a print_line callback returns 0. > This value can significate both: > > _ There is not enough space to print all of the entry. Please flush the current seq and retry > _ I can't handle this type of entry. > > Such a confusion can break the pipe. > > This patch changes the type of this callback for a better information. > > Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com> Frederic, this looks good to me, except the very last hunk. See below for some comments. > --- > diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h > index b28bf88..73cd7c5 100644 > --- a/kernel/trace/trace.h > +++ b/kernel/trace/trace.h > @@ -180,6 +180,14 @@ struct trace_array { > struct trace_array_cpu *data[NR_CPUS]; > }; > > + > +/* Return values for print_line callback */ > +enum print_line_t { > + TRACE_TYPE_PARTIAL_LINE = 0, /* Retry after flushing the seq */ > + TRACE_TYPE_HANDLED = 1, > + TRACE_TYPE_UNHANDLED = 2 /* Relay to other output functions */ > +}; > + I might have chosen names like TRACE_PRINT_{RETRY,DONE,DEFAULT_FORMAT}, but it's your call. I'm not sure which one is more self-explanatory when you see a "return TRACE_...;" or "if (ret == TRACE_...)" > /* > * A specific tracer, represented by methods that operate on a trace array: > */ > @@ -200,7 +208,7 @@ struct tracer { > int (*selftest)(struct tracer *trace, > struct trace_array *tr); > #endif > - int (*print_line)(struct trace_iterator *iter); > + enum print_line_t (*print_line)(struct trace_iterator *iter); > struct tracer *next; > int print_max; > }; > diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c > index 50ac334..ca95ec3 100644 > --- a/kernel/trace/trace.c > +++ b/kernel/trace/trace.c > @@ -1510,7 +1510,7 @@ void trace_seq_print_cont(struct trace_seq *s, struct trace_iterator *iter) > trace_seq_putc(s, '\n'); > } > > -static int > +static enum print_line_t > print_lat_fmt(struct trace_iterator *iter, unsigned int trace_idx, int cpu) > { > struct trace_seq *s = &iter->seq; > @@ -1530,7 +1530,7 @@ print_lat_fmt(struct trace_iterator *iter, unsigned int trace_idx, int cpu) > next_entry = entry; > > if (entry->type == TRACE_CONT) > - return 1; > + return TRACE_TYPE_HANDLED; > > rel_usecs = ns2usecs(next_entry->field.t - entry->field.t); > abs_usecs = ns2usecs(entry->field.t - iter->tr->time_start); > @@ -1601,10 +1601,10 @@ print_lat_fmt(struct trace_iterator *iter, unsigned int trace_idx, int cpu) > default: > trace_seq_printf(s, "Unknown type %d\n", entry->type); > } > - return 1; > + return TRACE_TYPE_HANDLED; > } > > -static int print_trace_fmt(struct trace_iterator *iter) > +static enum print_line_t print_trace_fmt(struct trace_iterator *iter) > { > struct trace_seq *s = &iter->seq; > unsigned long sym_flags = (trace_flags & TRACE_ITER_SYM_MASK); > @@ -1621,7 +1621,7 @@ static int print_trace_fmt(struct trace_iterator *iter) > entry = iter->ent; > > if (entry->type == TRACE_CONT) > - return 1; > + return TRACE_TYPE_HANDLED; > > field = &entry->field; > > @@ -1633,24 +1633,24 @@ static int print_trace_fmt(struct trace_iterator *iter) > > ret = trace_seq_printf(s, "%16s-%-5d ", comm, field->pid); > if (!ret) > - return 0; > + return TRACE_TYPE_PARTIAL_LINE; > ret = trace_seq_printf(s, "[%03d] ", iter->cpu); > if (!ret) > - return 0; > + return TRACE_TYPE_PARTIAL_LINE; > ret = trace_seq_printf(s, "%5lu.%06lu: ", secs, usec_rem); > if (!ret) > - return 0; > + return TRACE_TYPE_PARTIAL_LINE; > > switch (entry->type) { > case TRACE_FN: > ret = seq_print_ip_sym(s, field->fn.ip, sym_flags); > if (!ret) > - return 0; > + return TRACE_TYPE_PARTIAL_LINE; > if ((sym_flags & TRACE_ITER_PRINT_PARENT) && > field->fn.parent_ip) { > ret = trace_seq_printf(s, " <-"); > if (!ret) > - return 0; > + return TRACE_TYPE_PARTIAL_LINE; > if (kretprobed(field->fn.parent_ip)) > ret = trace_seq_puts(s, KRETPROBE_MSG); > else > @@ -1658,11 +1658,11 @@ static int print_trace_fmt(struct trace_iterator *iter) > field->fn.parent_ip, > sym_flags); > if (!ret) > - return 0; > + return TRACE_TYPE_PARTIAL_LINE; > } > ret = trace_seq_printf(s, "\n"); > if (!ret) > - return 0; > + return TRACE_TYPE_PARTIAL_LINE; > break; > case TRACE_CTX: > case TRACE_WAKE: > @@ -1680,7 +1680,7 @@ static int print_trace_fmt(struct trace_iterator *iter) > field->ctx.next_prio, > T); > if (!ret) > - return 0; > + return TRACE_TYPE_PARTIAL_LINE; > break; > case TRACE_SPECIAL: > ret = trace_seq_printf(s, "# %ld %ld %ld\n", > @@ -1688,23 +1688,23 @@ static int print_trace_fmt(struct trace_iterator *iter) > field->special.arg2, > field->special.arg3); > if (!ret) > - return 0; > + return TRACE_TYPE_PARTIAL_LINE; > break; > case TRACE_STACK: > for (i = 0; i < FTRACE_STACK_ENTRIES; i++) { > if (i) { > ret = trace_seq_puts(s, " <= "); > if (!ret) > - return 0; > + return TRACE_TYPE_PARTIAL_LINE; > } > ret = seq_print_ip_sym(s, field->stack.caller[i], > sym_flags); > if (!ret) > - return 0; > + return TRACE_TYPE_PARTIAL_LINE; > } > ret = trace_seq_puts(s, "\n"); > if (!ret) > - return 0; > + return TRACE_TYPE_PARTIAL_LINE; > break; > case TRACE_PRINT: > seq_print_ip_sym(s, field->print.ip, sym_flags); > @@ -1713,10 +1713,10 @@ static int print_trace_fmt(struct trace_iterator *iter) > trace_seq_print_cont(s, iter); > break; > } > - return 1; > + return TRACE_TYPE_HANDLED; > } > > -static int print_raw_fmt(struct trace_iterator *iter) > +static enum print_line_t print_raw_fmt(struct trace_iterator *iter) > { > struct trace_seq *s = &iter->seq; > struct trace_entry *entry; > @@ -1727,14 +1727,14 @@ static int print_raw_fmt(struct trace_iterator *iter) > entry = iter->ent; > > if (entry->type == TRACE_CONT) > - return 1; > + return TRACE_TYPE_HANDLED; > > field = &entry->field; > > ret = trace_seq_printf(s, "%d %d %llu ", > field->pid, iter->cpu, field->t); > if (!ret) > - return 0; > + return TRACE_TYPE_PARTIAL_LINE; > > switch (entry->type) { > case TRACE_FN: > @@ -1742,7 +1742,7 @@ static int print_raw_fmt(struct trace_iterator *iter) > field->fn.ip, > field->fn.parent_ip); > if (!ret) > - return 0; > + return TRACE_TYPE_PARTIAL_LINE; > break; > case TRACE_CTX: > case TRACE_WAKE: > @@ -1761,7 +1761,7 @@ static int print_raw_fmt(struct trace_iterator *iter) > field->ctx.next_prio, > T); > if (!ret) > - return 0; > + return TRACE_TYPE_PARTIAL_LINE; > break; > case TRACE_SPECIAL: > case TRACE_STACK: > @@ -1770,7 +1770,7 @@ static int print_raw_fmt(struct trace_iterator *iter) > field->special.arg2, > field->special.arg3); > if (!ret) > - return 0; > + return TRACE_TYPE_PARTIAL_LINE; > break; > case TRACE_PRINT: > trace_seq_printf(s, "# %lx %s", > @@ -1779,7 +1779,7 @@ static int print_raw_fmt(struct trace_iterator *iter) > trace_seq_print_cont(s, iter); > break; > } > - return 1; > + return TRACE_TYPE_HANDLED; > } > > #define SEQ_PUT_FIELD_RET(s, x) \ > @@ -1794,7 +1794,7 @@ do { \ > return 0; \ > } while (0) > > -static int print_hex_fmt(struct trace_iterator *iter) > +static enum print_line_t print_hex_fmt(struct trace_iterator *iter) > { > struct trace_seq *s = &iter->seq; > unsigned char newline = '\n'; > @@ -1805,7 +1805,7 @@ static int print_hex_fmt(struct trace_iterator *iter) > entry = iter->ent; > > if (entry->type == TRACE_CONT) > - return 1; > + return TRACE_TYPE_HANDLED; > > field = &entry->field; > > @@ -1843,10 +1843,10 @@ static int print_hex_fmt(struct trace_iterator *iter) > } > SEQ_PUT_FIELD_RET(s, newline); > > - return 1; > + return TRACE_TYPE_HANDLED; > } > > -static int print_bin_fmt(struct trace_iterator *iter) > +static enum print_line_t print_bin_fmt(struct trace_iterator *iter) > { > struct trace_seq *s = &iter->seq; > struct trace_entry *entry; > @@ -1855,7 +1855,7 @@ static int print_bin_fmt(struct trace_iterator *iter) > entry = iter->ent; > > if (entry->type == TRACE_CONT) > - return 1; > + return TRACE_TYPE_HANDLED; > > field = &entry->field; > > @@ -1883,7 +1883,7 @@ static int print_bin_fmt(struct trace_iterator *iter) > SEQ_PUT_FIELD_RET(s, field->special.arg3); > break; > } > - return 1; > + return TRACE_TYPE_HANDLED; > } > > static int trace_empty(struct trace_iterator *iter) > @@ -1904,27 +1904,42 @@ static int trace_empty(struct trace_iterator *iter) > > static int print_trace_line(struct trace_iterator *iter) Shouldn't the return type be bool? If it's not, reading this function makes me wonder about the conversion from enum to int, i.e. what is the meaning of the int. > { > - int ret; > + enum print_line_t ret; > > - if (iter->trace && iter->trace->print_line) > - if ((ret = iter->trace->print_line(iter))) > + if (iter->trace && iter->trace->print_line) { > + ret = iter->trace->print_line(iter); > + if (ret == TRACE_TYPE_HANDLED || > + ret == TRACE_TYPE_PARTIAL_LINE) > return ret; It would be shorter to write if (ret != TRACE_TYPE_UNHANDLED) and then one could even return (ret == TRACE_TYPE_HANDLED); > + } > > - if (trace_flags & TRACE_ITER_BIN) > - if ((ret = print_bin_fmt(iter))) > + if (trace_flags & TRACE_ITER_BIN) { > + ret = print_bin_fmt(iter); > + if (ret == TRACE_TYPE_HANDLED || > + ret == TRACE_TYPE_PARTIAL_LINE) > return ret; > + } Do these actually need checking? I don't think the default print functions would ever return TRACE_TYPE_UNHANDLED, could they? And even if they did, do all the different default print functions not handle the same set of entry types? > - if (trace_flags & TRACE_ITER_HEX) > - if ((ret = print_hex_fmt(iter))) > + if (trace_flags & TRACE_ITER_HEX) { > + ret = print_hex_fmt(iter); > + if (ret == TRACE_TYPE_HANDLED || > + ret == TRACE_TYPE_PARTIAL_LINE) > return ret; > + } > > - if (trace_flags & TRACE_ITER_RAW) > - if ((ret = print_raw_fmt(iter))) > + if (trace_flags & TRACE_ITER_RAW) { > + ret = print_raw_fmt(iter); > + if (ret == TRACE_TYPE_HANDLED || > + ret == TRACE_TYPE_PARTIAL_LINE) > return ret; > + } > > - if (iter->iter_flags & TRACE_FILE_LAT_FMT) > - if ((ret = print_lat_fmt(iter, iter->idx, iter->cpu))) > + if (iter->iter_flags & TRACE_FILE_LAT_FMT) { > + ret = print_lat_fmt(iter, iter->idx, iter->cpu); > + if (ret == TRACE_TYPE_HANDLED || > + ret == TRACE_TYPE_PARTIAL_LINE) > return ret; > + } > > return print_trace_fmt(iter); > } > @@ -2722,11 +2737,11 @@ tracing_read_pipe(struct file *filp, char __user *ubuf, > } > > while (find_next_entry_inc(iter) != NULL) { > - int ret; > + enum print_line_t ret; > int len = iter->seq.len; > > ret = print_trace_line(iter); Return value type mismatch. > - if (!ret) { > + if (ret == TRACE_TYPE_PARTIAL_LINE) { > /* don't print partial lines */ > iter->seq.len = len; > break; > @@ -2738,6 +2753,10 @@ tracing_read_pipe(struct file *filp, char __user *ubuf, > break; > } > > + if (!iter->seq.len) > + /* Don't break the pipe */ > + trace_seq_puts(&iter->seq, "Warning: empty seq\n"); > + > for_each_cpu_mask(cpu, mask) { > data = iter->tr->data[cpu]; > __raw_spin_unlock(&data->lock); We have to find a proper way to prevent the pipe from closing early. I'm trying to look into this. I'd like you to leave that last hunk out. Other than that, very good. Cheers. btw. there might be a corner case, when a single line does not fit even into an empty struct trace_seq in tracing_read_pipe(), but I haven't thought of that yet. I'd expect it to hang. -- Pekka Paalanen http://www.iki.fi/pq/ ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Path -tip 1/3] Tracing/ftrace: Change the type of the print_line callback 2008-09-28 16:19 ` Pekka Paalanen @ 2008-09-29 9:11 ` Frédéric Weisbecker 2008-09-29 15:04 ` Pekka Paalanen 0 siblings, 1 reply; 5+ messages in thread From: Frédéric Weisbecker @ 2008-09-29 9:11 UTC (permalink / raw) To: Pekka Paalanen; +Cc: mingo, rostedt, linux-kernel 2008/9/28 Pekka Paalanen <pq@iki.fi>: > I might have chosen names like TRACE_PRINT_{RETRY,DONE,DEFAULT_FORMAT}, > but it's your call. I'm not sure which one is more self-explanatory > when you see a "return TRACE_...;" or "if (ret == TRACE_...)" Hmm yes that's perhaps more explicit. > Shouldn't the return type be bool? > If it's not, reading this function makes me wonder about the conversion > from enum to int, i.e. what is the meaning of the int. Actually it should be enum print_line_t. I forgot to change its type. But we need to check its return value as an enum print_line_t. > It would be shorter to write > if (ret != TRACE_TYPE_UNHANDLED) > and then one could even > return (ret == TRACE_TYPE_HANDLED); Yes, I corrected it in my "week-end patch" :-) > Do these actually need checking? I don't think > the default print functions would ever return > TRACE_TYPE_UNHANDLED, could they? > And even if they did, do all the different default print > functions not handle the same set of entry types? At this moment they don't. But I just wanted to set a security in case of possible future changes in these functions. > We have to find a proper way to prevent the pipe from closing > early. I'm trying to look into this. I'd like you to leave > that last hunk out. Other than that, very good. I made a new patch this week-end and I found a way to prevent from closing the pipe. I will just change a bit my patch in the base of your comments. > btw. there might be a corner case, when a single line does not > fit even into an empty struct trace_seq in tracing_read_pipe(), > but I haven't thought of that yet. I'd expect it to hang. I should look at this possible issue too. I didn't think about it yet. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Path -tip 1/3] Tracing/ftrace: Change the type of the print_line callback 2008-09-29 9:11 ` Frédéric Weisbecker @ 2008-09-29 15:04 ` Pekka Paalanen 2008-09-29 15:56 ` Frédéric Weisbecker 0 siblings, 1 reply; 5+ messages in thread From: Pekka Paalanen @ 2008-09-29 15:04 UTC (permalink / raw) To: Frédéric Weisbecker; +Cc: mingo, rostedt, linux-kernel On Mon, 29 Sep 2008 11:11:15 +0200 "Frédéric Weisbecker" <fweisbec@gmail.com> wrote: > 2008/9/28 Pekka Paalanen <pq@iki.fi>: > [static int print_trace_line(struct trace_iterator *iter)] > > Shouldn't the return type be bool? > > If it's not, reading this function makes me wonder about the conversion > > from enum to int, i.e. what is the meaning of the int. > > Actually it should be enum print_line_t. I forgot to change its type. > But we need to check its return value as an enum print_line_t. Is print_line_t necessary? Does it have to return any other information than "this entry was handled somehow" vs. "this entry must be handled later"? Currently it's just a flag to say "please flush and retry". -- Pekka Paalanen http://www.iki.fi/pq/ ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Path -tip 1/3] Tracing/ftrace: Change the type of the print_line callback 2008-09-29 15:04 ` Pekka Paalanen @ 2008-09-29 15:56 ` Frédéric Weisbecker 0 siblings, 0 replies; 5+ messages in thread From: Frédéric Weisbecker @ 2008-09-29 15:56 UTC (permalink / raw) To: Pekka Paalanen; +Cc: mingo, rostedt, linux-kernel 2008/9/29 Pekka Paalanen <pq@iki.fi>: > On Mon, 29 Sep 2008 11:11:15 +0200 > "Frédéric Weisbecker" <fweisbec@gmail.com> wrote: > >> 2008/9/28 Pekka Paalanen <pq@iki.fi>: >> > > [static int print_trace_line(struct trace_iterator *iter)] > >> > Shouldn't the return type be bool? >> > If it's not, reading this function makes me wonder about the conversion >> > from enum to int, i.e. what is the meaning of the int. >> >> Actually it should be enum print_line_t. I forgot to change its type. >> But we need to check its return value as an enum print_line_t. > > Is print_line_t necessary? Does it have to return any other information than > "this entry was handled somehow" vs. "this entry must be handled later"? > Currently it's just a flag to say "please flush and retry". It would just make the code more readable in the read_pipe function. if (sret == TRACE_TYPE_PARTIAL_LINE) tells us more than if (!sret). And it would avoid some tests to return a boolean in print_trace_line. No? ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2008-09-29 15:57 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2008-09-26 15:25 [Path -tip 1/3] Tracing/ftrace: Change the type of the print_line callback Frederic Weisbecker 2008-09-28 16:19 ` Pekka Paalanen 2008-09-29 9:11 ` Frédéric Weisbecker 2008-09-29 15:04 ` Pekka Paalanen 2008-09-29 15:56 ` Frédéric Weisbecker
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox