Re: [RFC patch] spindep: add cross cache lines checking
On Tue, 2012-03-06 at 09:32 +, Arnd Bergmann wrote:
> On Tuesday 06 March 2012, Alex Shi wrote:
> > I have one concern and one questions here:
> > concern: maybe the lock is in a well designed 'packed' struct, and it is
> > safe for cross lines issue. but __alignof__ will return 1;
> >
> > struct abc{
> > raw_spinlock_t lock1;
> > chara;
> > charb;
> > }__attribute__((packed));
> >
> > Since the lock is the first object of struct, usually it is well placed.
>
> No, it's actually not. The structure has an external alignment of 1, so
> if you have an array of these or put it into another struct like
>
> struct xyz {
> char x;
> struct abc a;
> };
>
> then it will be misaligned. Thre is no such thing as a well designed 'packed'
> struct. The only reason to use packing is to describe structures we have no
> control over such as hardware layouts and on-wire formats that have unusal
> alignments, and those will never have a spinlock on them.
Understand. thx. So is the following checking that your wanted?
===
diff --git a/include/linux/rwlock.h b/include/linux/rwlock.h
index bc2994e..64828a3 100644
--- a/include/linux/rwlock.h
+++ b/include/linux/rwlock.h
@@ -21,10 +21,12 @@
do { \
static struct lock_class_key __key; \
\
+ BUILD_BUG_ON(__alignof__(lock) == 1); \
__rwlock_init((lock), #lock, &__key); \
} while (0)
#else
# define rwlock_init(lock) \
+ BUILD_BUG_ON(__alignof__(lock) == 1); \
do { *(lock) = __RW_LOCK_UNLOCKED(lock); } while (0)
#endif
diff --git a/include/linux/spinlock.h b/include/linux/spinlock.h
index 7df6c17..df8a992 100644
--- a/include/linux/spinlock.h
+++ b/include/linux/spinlock.h
@@ -96,11 +96,13 @@
do { \
static struct lock_class_key __key; \
\
+ BUILD_BUG_ON(__alignof__(lock) == 1); \
__raw_spin_lock_init((lock), #lock, &__key);\
} while (0)
#else
# define raw_spin_lock_init(lock) \
+ BUILD_BUG_ON(__alignof__(lock) == 1); \
do { *(lock) = __RAW_SPIN_LOCK_UNLOCKED(lock); } while (0)
#endif
===
Btw,
1, it is alignof bug for default gcc on my fc15 and Ubuntu 11.10 etc?
struct sub {
int raw_lock;
char a;
};
struct foo {
struct sub z;
int slk;
char y;
}__attribute__((packed));
struct foo f1;
__alignof__(f1.z.raw_lock) is 4, but its address actually can align on
one byte.
>
> Arnd
Re: [RFC patch] spindep: add cross cache lines checking
> I think the check should be (__alignof__(lock) < __alignof__(rwlock_t)),
> otherwise it will still pass when you have structure with
> attribute((packed,aligned(2)))
reasonable!
>
>> 1, it is alignof bug for default gcc on my fc15 and Ubuntu 11.10 etc?
>>
>> struct sub {
>> int raw_lock;
>> char a;
>> };
>> struct foo {
>> struct sub z;
>> int slk;
>> char y;
>> }__attribute__((packed));
>>
>> struct foo f1;
>>
>> __alignof__(f1.z.raw_lock) is 4, but its address actually can align on
>> one byte.
>
> That looks like correct behavior, because the alignment of raw_lock inside of
> struct sub is still 4. But it does mean that there can be cases where the
> compile-time check is not sufficient, so we might want the run-time check
> as well, at least under some config option.
what's your opinion of this, Ingo?
Re: [RFC patch] spindep: add cross cache lines checking
On Wed, 2012-03-07 at 14:39 +0100, Ingo Molnar wrote:
> * Alex Shi wrote:
>
> > > I think the check should be (__alignof__(lock) <
> > > __alignof__(rwlock_t)), otherwise it will still pass when
> > > you have structure with attribute((packed,aligned(2)))
> >
> > reasonable!
> >
> > >> 1, it is alignof bug for default gcc on my fc15 and Ubuntu 11.10 etc?
> > >>
> > >> struct sub {
> > >> int raw_lock;
> > >> char a;
> > >> };
> > >> struct foo {
> > >> struct sub z;
> > >> int slk;
> > >> char y;
> > >> }__attribute__((packed));
> > >>
> > >> struct foo f1;
> > >>
> > >> __alignof__(f1.z.raw_lock) is 4, but its address actually can align on
> > >> one byte.
> > >
> > > That looks like correct behavior, because the alignment of
> > > raw_lock inside of struct sub is still 4. But it does mean
> > > that there can be cases where the compile-time check is not
> > > sufficient, so we might want the run-time check as well, at
> > > least under some config option.
> >
> > what's your opinion of this, Ingo?
>
> Dunno. How many real bugs have you found via this patch?
None. Guess stupid code was shot in lkml reviewing. But if the patch in,
it is helpful to block stupid code in developing.
>
> Thanks,
>
> Ingo
Re: [RFC patch] spindep: add cross cache lines checking
> > 1, it is alignof bug for default gcc on my fc15 and Ubuntu 11.10 etc?
> >
> > struct sub {
> > int raw_lock;
> > char a;
> > };
> > struct foo {
> > struct sub z;
> > int slk;
> > char y;
> > }__attribute__((packed));
> >
> > struct foo f1;
> >
> > __alignof__(f1.z.raw_lock) is 4, but its address actually can align on
> > one byte.
>
> That looks like correct behavior, because the alignment of raw_lock inside of
> struct sub is still 4. But it does mean that there can be cases where the
> compile-time check is not sufficient, so we might want the run-time check
> as well, at least under some config option.
According to explanation of gcc, seems it should return 1 when it can be
align on char. And then it's useful for design intend. Any comments from
gcc guys?
http://gcc.gnu.org/onlinedocs/gcc/Alignment.html
The keyword __alignof__ allows you to inquire about how an object is
aligned, or the minimum alignment usually required by a type. Its syntax
is just like sizeof.
Re: [RFC patch] spindep: add cross cache lines checking
On Thu, 2012-03-08 at 08:13 +0100, Ingo Molnar wrote:
> * Alex Shi wrote:
>
> > On Wed, 2012-03-07 at 14:39 +0100, Ingo Molnar wrote:
> > > * Alex Shi wrote:
> > >
> > > > > I think the check should be (__alignof__(lock) <
> > > > > __alignof__(rwlock_t)), otherwise it will still pass when
> > > > > you have structure with attribute((packed,aligned(2)))
> > > >
> > > > reasonable!
> > > >
> > > > >> 1, it is alignof bug for default gcc on my fc15 and Ubuntu 11.10 etc?
> > > > >>
> > > > >> struct sub {
> > > > >> int raw_lock;
> > > > >> char a;
> > > > >> };
> > > > >> struct foo {
> > > > >> struct sub z;
> > > > >> int slk;
> > > > >> char y;
> > > > >> }__attribute__((packed));
> > > > >>
> > > > >> struct foo f1;
> > > > >>
> > > > >> __alignof__(f1.z.raw_lock) is 4, but its address actually can align
> > > > >> on
> > > > >> one byte.
> > > > >
> > > > > That looks like correct behavior, because the alignment of
> > > > > raw_lock inside of struct sub is still 4. But it does mean
> > > > > that there can be cases where the compile-time check is not
> > > > > sufficient, so we might want the run-time check as well, at
> > > > > least under some config option.
> > > >
> > > > what's your opinion of this, Ingo?
> > >
> > > Dunno. How many real bugs have you found via this patch?
> >
> > None. Guess stupid code was shot in lkml reviewing. But if the
> > patch in, it is helpful to block stupid code in developing.
>
> The question is, if in the last 10 years not a single such case
> made it through to today's 15 million lines of kernel code, why
> should we add the check now?
>
> If it was a simple build time check then maybe, but judging by
> the discussion it does not seem so simple, does it?
Oh, It is may better to have, but I don't mind it was slided. Since even
alignof works as our expectation, it also can't cover all problems.
Currently, we heavily depend gcc's behavior.
Anyway, thanks for review!
>
> Thanks,
>
> Ingo
