lkml.org 
[lkml]   [2009]   [Oct]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: system gets stuck in a lock during boot
    * Steven Rostedt (rostedt@goodmis.org) wrote:
    > On Wed, 2009-10-07 at 10:40 -0400, Mathieu Desnoyers wrote:
    > > * Jason Baron (jbaron@redhat.com) 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@redhat.com>
    > > > > >
    > > > > >
    > > > > > 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@goodmis.org>
    > > > >
    > > > >
    > > > > 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.
    > */
    >

    Yep, sounds good. Oddly we have to keep these in sync manually. I'd also
    add a comment in the C code to tell whoever want to change the size of
    the structure to also check the linker script.

    Also adding a BUILD_BUG_ON() that checks the structure sizeof() would be
    a nice safety-net (this should probably be added to tracepoints too
    eventually).

    Mathieu

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

    --
    Mathieu Desnoyers
    OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F BA06 3F25 A8FE 3BAE 9A68


    \
     
     \ /
      Last update: 2009-10-07 17:15    [W:3.515 / U:0.144 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site