lkml.org 
[lkml]   [2008]   [Aug]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] ftrace: printk formatting infrastructure

On Thu, 31 Jul 2008, Andrew Morton wrote:

>
> Seems like it would be useful.
>
> Do we have any evidence that reandom developers are using ftrace yet?
> And any feedback from them?

It's still new. I've had a lot of interest in this at OLS from various
developers. One person asked me about this feature during my tutorial.

One thing on my todo list is to back port ftrace to previous kernels. I
had a few requests to do just that, since a lot of driver developers and
embedded programmers are using older kernels.

>
> >
> > ...
> >
> > +static void
> > +trace_seq_print_cont(struct trace_seq *s, struct trace_iterator *iter)
> > +{
> > + struct trace_array *tr = iter->tr;
> > + struct trace_array_cpu *data = tr->data[iter->cpu];
> > + struct trace_entry *ent;
> > + struct trace_cont *cont;
> > +
> > + ent = trace_entry_idx(tr, data, iter, iter->cpu);
> > + if (!ent || ent->type != TRACE_CONT) {
> > + trace_seq_putc(s, '\n');
> > + return;
> > + }
> > +
> > + do {
> > + cont = (struct trace_cont *)ent;
>
> What you have here is umm, a variant record, or its in-memory equiv
> which probably has a name.

Yeah I know, I didn't like that.

>
> > ...
> >
> > +/*
> > + * When an item needs more than one entry to fill a buffer
> > + * it can use this structure.
> > + */
> > +struct trace_cont {
> > + char type;
> > + char buf[];
> > +};
>
> Would it not be cleaner and clearer to do this:
>
> struct trace_entry {
> char type;
> union {
> struct {
> char cpu;
> char flags;
> char preempt_count;
> int pid;
> cycle_t t;
> union {
> struct ftrace_entry fn;
> struct ctx_switch_entry ctx;
> struct special_entry special;
> struct stack_entry stack;
> struct mmiotrace_rw mmiorw;
> struct mmiotrace_map mmiomap;
> };
> };
> char buf[0];
> <other things here>
> }
> };
>
> So the first byte (`type') indicates which of the members of that union
> are actually contained in the payload.
>
> That way it's all typesafe and avoids casting.
>
>
> Of course, rather than using the anonymous union/struct trickery it
> would be much nicer and clearer to do it this way:
>
> struct trace_field {
> char cpu;
> char flags;
> char preempt_count;
> int pid;
> cycle_t t;
> union {
> struct ftrace_entry fn;
> struct ctx_switch_entry ctx;
> struct special_entry special;
> struct stack_entry stack;
> struct mmiotrace_rw mmiorw;
> struct mmiotrace_map mmiomap;
> };
> };
>
> struct trace_cont_field {
> char buf[];
> };
>
> ...
>
> struct trace_entry {
> char type;
> union {
> struct trace_field trace_field;
> struct trace_cont_field trace_cont_field;
> ...
> };
> };
>
> however that would require a large (but very simple) edit to the
> existing code.

Hehe, that was actually the reason I didn't do it this way. I didn't want
this patch to "blow up" in size.

>
> In the long run, the larger patch would be better though.
>

I agree. I'll rewrite the patch and to hell with the diff size ;-)

Hmm, Perhaps I'll rewrite this in two patches. One with the struct change,
and the other with the meat of the change.

-- Steve



\
 
 \ /
  Last update: 2008-08-01 14:15    [W:0.046 / U:0.068 seconds]
©2003-2011 Jasper Spaans