Explicitly align the start of the buffer page data array to the
required arch alignment. This is required for architectures that
require 8 byte alignment but do not have a 8 byte local_t.
Signed-off-by: Will Newton <[email protected]>
---
kernel/trace/ring_buffer.c | 4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)
I don't believe that any currently in-tree architecture is affected by
this, but it can be an issue on 32bit architectures that require an
8 byte aligment but only have a 32bit local_t. I think it's potentially
cleaner to make the alignment explicit anyway.
diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
index 0ef7b4b..36d5699 100644
--- a/kernel/trace/ring_buffer.c
+++ b/kernel/trace/ring_buffer.c
@@ -215,6 +215,8 @@ EXPORT_SYMBOL_GPL(tracing_is_on);
# define RB_ARCH_ALIGNMENT 8U
#endif
+#define RB_ALIGN_DATA __aligned(RB_ARCH_ALIGNMENT)
+
/* define RINGBUF_TYPE_DATA for 'case RINGBUF_TYPE_DATA:' */
#define RINGBUF_TYPE_DATA 0 ... RINGBUF_TYPE_DATA_TYPE_LEN_MAX
@@ -363,7 +365,7 @@ EXPORT_SYMBOL_GPL(ring_buffer_event_data);
struct buffer_data_page {
u64 time_stamp; /* page time stamp */
local_t commit; /* write committed index */
- unsigned char data[]; /* data of buffer page */
+ unsigned char data[] RB_ALIGN_DATA; /* data of buffer page */
};
/*
--
1.7.3.4
On Thu, 2011-06-16 at 14:03 +0100, Will Newton wrote:
> Explicitly align the start of the buffer page data array to the
> required arch alignment. This is required for architectures that
> require 8 byte alignment but do not have a 8 byte local_t.
What arch does that? A 32bit arch that forces 8 byte alignment?
>
> Signed-off-by: Will Newton <[email protected]>
> ---
> kernel/trace/ring_buffer.c | 4 +++-
> 1 files changed, 3 insertions(+), 1 deletions(-)
>
> I don't believe that any currently in-tree architecture is affected by
> this, but it can be an issue on 32bit architectures that require an
> 8 byte aligment but only have a 32bit local_t. I think it's potentially
> cleaner to make the alignment explicit anyway.
>
> diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
> index 0ef7b4b..36d5699 100644
> --- a/kernel/trace/ring_buffer.c
> +++ b/kernel/trace/ring_buffer.c
> @@ -215,6 +215,8 @@ EXPORT_SYMBOL_GPL(tracing_is_on);
> # define RB_ARCH_ALIGNMENT 8U
> #endif
>
> +#define RB_ALIGN_DATA __aligned(RB_ARCH_ALIGNMENT)
> +
Note, the code above this is:
#define RB_EVNT_HDR_SIZE (offsetof(struct ring_buffer_event, array))
#define RB_ALIGNMENT 4U
#define RB_MAX_SMALL_DATA (RB_ALIGNMENT * RINGBUF_TYPE_DATA_TYPE_LEN_MAX)
#define RB_EVNT_MIN_SIZE 8U /* two 32bit words */
#if !defined(CONFIG_64BIT) || defined(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS)
# define RB_FORCE_8BYTE_ALIGNMENT 0
# define RB_ARCH_ALIGNMENT RB_ALIGNMENT
#else
# define RB_FORCE_8BYTE_ALIGNMENT 1
# define RB_ARCH_ALIGNMENT 8U
#endif
Which means that when CONFIG_64BIT is not set, RB_ARCH_ALIGNMENT is 4.
This means that this patch is really a nop and doesn't do anything for
your arch.
-- Steve
> /* define RINGBUF_TYPE_DATA for 'case RINGBUF_TYPE_DATA:' */
> #define RINGBUF_TYPE_DATA 0 ... RINGBUF_TYPE_DATA_TYPE_LEN_MAX
>
> @@ -363,7 +365,7 @@ EXPORT_SYMBOL_GPL(ring_buffer_event_data);
> struct buffer_data_page {
> u64 time_stamp; /* page time stamp */
> local_t commit; /* write committed index */
> - unsigned char data[]; /* data of buffer page */
> + unsigned char data[] RB_ALIGN_DATA; /* data of buffer page */
> };
>
> /*
On Thu, Jun 16, 2011 at 2:17 PM, Steven Rostedt <[email protected]> wrote:
> On Thu, 2011-06-16 at 14:03 +0100, Will Newton wrote:
>> Explicitly align the start of the buffer page data array to the
>> required arch alignment. This is required for architectures that
>> require 8 byte alignment but do not have a 8 byte local_t.
>
> What arch does that? A 32bit arch that forces 8 byte alignment?
Yes, it's likely for all 64bit arches will be aligned correctly.
>>
>> Signed-off-by: Will Newton <[email protected]>
>> ---
>> ?kernel/trace/ring_buffer.c | ? ?4 +++-
>> ?1 files changed, 3 insertions(+), 1 deletions(-)
>>
>> I don't believe that any currently in-tree architecture is affected by
>> this, but it can be an issue on 32bit architectures that require an
>> 8 byte aligment but only have a 32bit local_t. I think it's potentially
>> cleaner to make the alignment explicit anyway.
>>
>> diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
>> index 0ef7b4b..36d5699 100644
>> --- a/kernel/trace/ring_buffer.c
>> +++ b/kernel/trace/ring_buffer.c
>> @@ -215,6 +215,8 @@ EXPORT_SYMBOL_GPL(tracing_is_on);
>> ?# define RB_ARCH_ALIGNMENT ? ? ? ? ? 8U
>> ?#endif
>>
>> +#define RB_ALIGN_DATA ? ? ? ? ? ? ? ?__aligned(RB_ARCH_ALIGNMENT)
>> +
>
> Note, the code above this is:
>
> #define RB_EVNT_HDR_SIZE (offsetof(struct ring_buffer_event, array))
> #define RB_ALIGNMENT ? ? ? ? ? ?4U
> #define RB_MAX_SMALL_DATA ? ? ? (RB_ALIGNMENT * RINGBUF_TYPE_DATA_TYPE_LEN_MAX)
> #define RB_EVNT_MIN_SIZE ? ? ? ?8U ? ? ?/* two 32bit words */
>
> #if !defined(CONFIG_64BIT) || defined(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS)
> # define RB_FORCE_8BYTE_ALIGNMENT ? ? ? 0
> # define RB_ARCH_ALIGNMENT ? ? ? ? ? ? ?RB_ALIGNMENT
> #else
> # define RB_FORCE_8BYTE_ALIGNMENT ? ? ? 1
> # define RB_ARCH_ALIGNMENT ? ? ? ? ? ? ?8U
> #endif
>
>
> Which means that when CONFIG_64BIT is not set, RB_ARCH_ALIGNMENT is 4.
> This means that this patch is really a nop and doesn't do anything for
> your arch.
I have modified that conditional in my local tree so the patch is not
a nop there. Obviously it doesn't make any sense to merge that change
yet. I thought this part of the patch was a reasonable cleanup to make
the code a bit more self documenting though.
On Thu, 2011-06-16 at 14:32 +0100, Will Newton wrote:
> On Thu, Jun 16, 2011 at 2:17 PM, Steven Rostedt <[email protected]> wrote:
> > On Thu, 2011-06-16 at 14:03 +0100, Will Newton wrote:
> >> Explicitly align the start of the buffer page data array to the
> >> required arch alignment. This is required for architectures that
> >> require 8 byte alignment but do not have a 8 byte local_t.
> >
> > What arch does that? A 32bit arch that forces 8 byte alignment?
>
> Yes, it's likely for all 64bit arches will be aligned correctly.
What arch is this exactly. Is it because 64bit variables must be aligned
on 8byte boundaries?
>
> >>
> >> Signed-off-by: Will Newton <[email protected]>
> >> ---
> >> kernel/trace/ring_buffer.c | 4 +++-
> >> 1 files changed, 3 insertions(+), 1 deletions(-)
> >>
> >> I don't believe that any currently in-tree architecture is affected by
> >> this, but it can be an issue on 32bit architectures that require an
> >> 8 byte aligment but only have a 32bit local_t. I think it's potentially
> >> cleaner to make the alignment explicit anyway.
> >>
> >> diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
> >> index 0ef7b4b..36d5699 100644
> >> --- a/kernel/trace/ring_buffer.c
> >> +++ b/kernel/trace/ring_buffer.c
> >> @@ -215,6 +215,8 @@ EXPORT_SYMBOL_GPL(tracing_is_on);
> >> # define RB_ARCH_ALIGNMENT 8U
> >> #endif
> >>
> >> +#define RB_ALIGN_DATA __aligned(RB_ARCH_ALIGNMENT)
> >> +
> >
> > Note, the code above this is:
> >
> > #define RB_EVNT_HDR_SIZE (offsetof(struct ring_buffer_event, array))
> > #define RB_ALIGNMENT 4U
> > #define RB_MAX_SMALL_DATA (RB_ALIGNMENT * RINGBUF_TYPE_DATA_TYPE_LEN_MAX)
> > #define RB_EVNT_MIN_SIZE 8U /* two 32bit words */
> >
> > #if !defined(CONFIG_64BIT) || defined(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS)
> > # define RB_FORCE_8BYTE_ALIGNMENT 0
> > # define RB_ARCH_ALIGNMENT RB_ALIGNMENT
> > #else
> > # define RB_FORCE_8BYTE_ALIGNMENT 1
> > # define RB_ARCH_ALIGNMENT 8U
> > #endif
> >
> >
> > Which means that when CONFIG_64BIT is not set, RB_ARCH_ALIGNMENT is 4.
> > This means that this patch is really a nop and doesn't do anything for
> > your arch.
>
> I have modified that conditional in my local tree so the patch is not
> a nop there. Obviously it doesn't make any sense to merge that change
> yet. I thought this part of the patch was a reasonable cleanup to make
> the code a bit more self documenting though.
Unfortunately, if anything it confuses the code more. If I were
reviewing it and saw that we are aligning something that is already
aligned with the same value, I would question that code.
Now if/when your arch is merged, we can add this with the other changes
you have, which will make reviewing the code not as confusing.
-- Steve
On Thu, Jun 16, 2011 at 6:24 PM, Steven Rostedt <[email protected]> wrote:
> On Thu, 2011-06-16 at 14:32 +0100, Will Newton wrote:
>> On Thu, Jun 16, 2011 at 2:17 PM, Steven Rostedt <[email protected]> wrote:
>> > On Thu, 2011-06-16 at 14:03 +0100, Will Newton wrote:
>> >> Explicitly align the start of the buffer page data array to the
>> >> required arch alignment. This is required for architectures that
>> >> require 8 byte alignment but do not have a 8 byte local_t.
>> >
>> > What arch does that? A 32bit arch that forces 8 byte alignment?
>>
>> Yes, it's likely for all 64bit arches will be aligned correctly.
>
> What arch is this exactly. Is it because 64bit variables must be aligned
> on 8byte boundaries?
Yes, it's a 32bit system with 64bit alignment required for 64bit
variables. I'm not sure whether there are other architectures in the
tree that have this characteristic but it's relatively common in cores
with DSP heritage.
http://imgtec.com/meta/meta-technology.asp
>>
>> >>
>> >> Signed-off-by: Will Newton <[email protected]>
>> >> ---
>> >> ?kernel/trace/ring_buffer.c | ? ?4 +++-
>> >> ?1 files changed, 3 insertions(+), 1 deletions(-)
>> >>
>> >> I don't believe that any currently in-tree architecture is affected by
>> >> this, but it can be an issue on 32bit architectures that require an
>> >> 8 byte aligment but only have a 32bit local_t. I think it's potentially
>> >> cleaner to make the alignment explicit anyway.
>> >>
>> >> diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
>> >> index 0ef7b4b..36d5699 100644
>> >> --- a/kernel/trace/ring_buffer.c
>> >> +++ b/kernel/trace/ring_buffer.c
>> >> @@ -215,6 +215,8 @@ EXPORT_SYMBOL_GPL(tracing_is_on);
>> >> ?# define RB_ARCH_ALIGNMENT ? ? ? ? ? 8U
>> >> ?#endif
>> >>
>> >> +#define RB_ALIGN_DATA ? ? ? ? ? ? ? ?__aligned(RB_ARCH_ALIGNMENT)
>> >> +
>> >
>> > Note, the code above this is:
>> >
>> > #define RB_EVNT_HDR_SIZE (offsetof(struct ring_buffer_event, array))
>> > #define RB_ALIGNMENT ? ? ? ? ? ?4U
>> > #define RB_MAX_SMALL_DATA ? ? ? (RB_ALIGNMENT * RINGBUF_TYPE_DATA_TYPE_LEN_MAX)
>> > #define RB_EVNT_MIN_SIZE ? ? ? ?8U ? ? ?/* two 32bit words */
>> >
>> > #if !defined(CONFIG_64BIT) || defined(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS)
>> > # define RB_FORCE_8BYTE_ALIGNMENT ? ? ? 0
>> > # define RB_ARCH_ALIGNMENT ? ? ? ? ? ? ?RB_ALIGNMENT
>> > #else
>> > # define RB_FORCE_8BYTE_ALIGNMENT ? ? ? 1
>> > # define RB_ARCH_ALIGNMENT ? ? ? ? ? ? ?8U
>> > #endif
>> >
>> >
>> > Which means that when CONFIG_64BIT is not set, RB_ARCH_ALIGNMENT is 4.
>> > This means that this patch is really a nop and doesn't do anything for
>> > your arch.
>>
>> I have modified that conditional in my local tree so the patch is not
>> a nop there. Obviously it doesn't make any sense to merge that change
>> yet. I thought this part of the patch was a reasonable cleanup to make
>> the code a bit more self documenting though.
>
> Unfortunately, if anything it confuses the code more. If I were
> reviewing it and saw that we are aligning something that is already
> aligned with the same value, I would question that code.
It does however make the conditional more flexible as you could change
it to #if defined (CONFIG_WHATEVER) and it will still work, rather
than the 64bit assumption leaking throughout the code.
> Now if/when your arch is merged, we can add this with the other changes
> you have, which will make reviewing the code not as confusing.
Sure, no problem.