Re: system gets stuck in a lock during boot

[Steven Rostedt]

On Wed, 2009-10-07 at 10:40 -0400, Mathieu Desnoyers wrote:
> * Jason Baron (jbaron@xxxxxxxxxx) wrote:
> > On Tue, Oct 06, 2009 at 10:02:01PM -0400, Steven Rostedt wrote:
> > > > So the problem I'm seeing is an oops on boot caused by the call->system pointer
> > > > deference in event_create_dir(). The 'call' variable is of type 'struct
> > > > ftrace_event_call'. 
> > > > 
> > > > What's going on is that the 'struct ftrace_event_call' is of size 168 bytes
> > > > (sizeof(struct ftrace_event_call)) = 168 = 0xA8. However, in memory the
> > > > structures are 16-byte aligned. Thus, the stride for walking through the
> > > > pointers needs to be 176 (0xB0), but instead its 168 causing the oops.
> > > > 
> > > > I've only seen this issue while using gcc (GCC) 4.5.0 20090916, on a
> > > > vanilla 2.6.31 kernel.
> > > > 
> > > > That said, I'm not sure the compiler is doing the wrong thing here. The
> > > > 'struct ftrace_event_call' contains an embedded 'struct list_head' which
> > > > is 16 bytes. According to the gcc docs, the aligned attribute, 'specifies a
> > > > minimum alignment for the variable or structure field, measured in bytes'.
> > > > Thus, at least according to the docs, gcc can increase the alignment of the
> > > > 'struct ftrace_event_call', from its original specification of 4, to 16. Even
> > > > in the case where we are working corectly the structures are 8-byte aligned.
> > > > 
> > > > Thus, I would reccommend the patch below as a preventive measure. Its
> > > > the minimal patch I've found to resolve this issue. In general, if we
> > > > are going to walk data structures embedded in a special elf section, I
> > > > think the general rules needs to be to set the alignment to the power of
> > > > two which is greater than or equal to the largest item in the structure.
> > > > 
> > > > thanks,
> > > > 
> > > > -Jason
> > > > 
> > > > Signed-off-by: Jason Baron <jbaron@xxxxxxxxxx>
> > > > 
> > > > 
> > > > diff --git a/include/linux/ftrace_event.h b/include/linux/ftrace_event.h
> > > > index a81170d..7182f03 100644
> > > > --- a/include/linux/ftrace_event.h
> > > > +++ b/include/linux/ftrace_event.h
> > > > @@ -124,7 +124,10 @@ struct ftrace_event_call {
> > > >  	atomic_t		profile_count;
> > > >  	int			(*profile_enable)(struct ftrace_event_call *);
> > > >  	void			(*profile_disable)(struct ftrace_event_call *);
> > > > -};
> > > > +} __attribute__((aligned(16)));
> > > > +
> > > > +/* Align to the largest field in the data structure:
> > > > + * sizeof(struct list_head) = 16 */
> > > 
> > > Is this true for i386?
> > > 
> > > I just tried this patch and it seems to work. Can you give it a try.
> > > 
> > > Signed-off-by: Steven Rostedt <rostedt@xxxxxxxxxxx>
> > > 
> > > 
> > > diff --git a/include/linux/ftrace_event.h b/include/linux/ftrace_event.h
> > > index 4ec5e67..044b70d 100644
> > > --- a/include/linux/ftrace_event.h
> > > +++ b/include/linux/ftrace_event.h
> > > @@ -133,7 +133,7 @@ struct ftrace_event_call {
> > >  	atomic_t		profile_count;
> > >  	int			(*profile_enable)(void);
> > >  	void			(*profile_disable)(void);
> > > -};
> > > +} __attribute__((aligned(sizeof(struct list_head))));
> 
> I don't like that.
> 
> Basically, the vmlinux.lds.h linker script must have alignment
> statements before each section, which match the alignment of the section
> structures. Failure to do so would put padding at the beginning of the
> section, which is definitely not working at all. I don't see how we can
> automatically pass sizeof(struct list_head) to a linker script :/

OK, what about __attribute__((aligned((BITS_PER_LONG/8)*2)))

That should also work in the linker script as well.

With the added comment:

/*
 * We must aligned by the largest item in the structure. This happens
 * to be the list_head, which consists of two pointers.
 */

> 
> Mathieu
> 
> > >  
> > >  #define FTRACE_MAX_PROFILE_SIZE	2048
> > >  
> > > diff --git a/include/trace/ftrace.h b/include/trace/ftrace.h
> > > index cc0d966..31e7637 100644
> > > --- a/include/trace/ftrace.h
> > > +++ b/include/trace/ftrace.h
> > > @@ -501,7 +501,6 @@ static void ftrace_profile_disable_##call(void)				\
> > >   * }
> > >   *
> > >   * static struct ftrace_event_call __used
> > > - * __attribute__((__aligned__(4)))
> > >   * __attribute__((section("_ftrace_events"))) event_<call> = {
> > >   *	.name			= "<call>",
> > >   *	.system			= "<system>",
> > > @@ -619,7 +618,6 @@ static int ftrace_raw_init_event_##call(void)				\
> > >  }									\
> > >  									\
> > >  static struct ftrace_event_call __used					\
> > > -__attribute__((__aligned__(4)))						\
> > >  __attribute__((section("_ftrace_events"))) event_##call = {		\
> > >  	.name			= #call,				\
> > >  	.system			= __stringify(TRACE_SYSTEM),		\
> > > 
> > > 
> > 
> > indeed your patch works as well for me, its much cleaner! 
> > 
> > However, I want to make sure this fix is sufficient and is the best way to
> > address this type of issue in general. For example, I know tracepoints are
> > using the aligned attribute in all 3 places -> definition, usage, and linker
> > alignment. (adding Mathieu to 'cc list). Is just the definition 'aligned'
> > sufficient? Also, once we find a method for solving these issues in general,
> > we need to review all users of this kind of technique to make sure they are
> > consistent. I also think your patch above needs to add a comment to say what
> > its doing.

Yes, I forgot to add the comment. One really does belong there.

-- Steve

> > 
> > thanks,
> > 
> > -Jason
> > 
> > 
> 

--
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/