Re: system gets stuck in a lock during boot

[Mathieu Desnoyers]

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

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.
> 
> thanks,
> 
> -Jason
> 
> 

-- 
Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68
--
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/