On Thu, 31 Jul 2008, Andrew Morton wrote:

quoted text
> 
> 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.

quoted text
> 
> >
> > ...
> >
> > +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.

quoted text
> 
> > ...
> >
> > +/*
> > + * 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.

quoted text
> 
> 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

--
unsubscribe notice
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/