Re: [PATCH 6/6] ftrace: take advantage of variable length entries

[Steven Rostedt]

On Wed, 1 Oct 2008, Ingo Molnar wrote:
> 
> * Steven Rostedt <rostedt@xxxxxxxxxxx> wrote:
> 
> > > >  	struct trace_entry *entry = iter->ent;
> > > > -	struct mmiotrace_map *m	= &entry->field.mmiomap;
> > > > +	struct mmiotrace_map *m	= (struct mmiotrace_map *)entry;
> > > 
> > > This is different style than above, missing the struct
> > > trace_mmiotrace_map intermediate step. Looks like a bug,
> > > since struct mmiotrace_map is not the first field in
> > > struct trace_mmiotrace_map.
> > 
> > Crap! yes this is a bug. Thanks for pointing this out.
> 
> hm, there's a significant amount of type casts, see the grep below.

Yeah, I don't like that either.

> 
> The ringbuffer becoming type-opaque has exactly these kinds of dangers, 
> and as i suggested a few days ago, please think about a debug mode that 
> stores the record type in the trace entry and validates it on extraction 
> or something like that.
> 
> That might even be a robustness feature: the tracer should not crash if 
> the trace buffer gets corrupted. ftrace had that invariant before, i 
> think we should try to keep as many aspects of it as possible.

Although the ring buffer is type opaque, that isn't the cause of this 
issue. Ftrace still adds the types as needed. But since we no longer have 
the big "one type for all" union entry, we need to be a bit more careful. 
All types start with a one byte type id (giving us 255 types to use for 
ftrace, zero being saved).

The following patch fixes this bug and addresses the issue.

-- Steve

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