| Message ID | 9f896c31-9c3a-a3a9-a994-6b818b403c9c@virtuozzo.com |
|---|---|
| State | Superseded |
| Series | "pstree: Aligned pstree item allocation" |
| Headers | show |
diff --git a/criu/pstree.c b/criu/pstree.c index 6daf4069a..6d980c461 100644 --- a/criu/pstree.c +++ b/criu/pstree.c @@ -221,6 +221,7 @@ struct pstree_item *__alloc_pstree_item(bool rst) item->born_sid = -1; futex_init(&item->task_st); item->pid->item = item; + BUG_ON(item & 3); /* Futex should be aligned */ return item; }
On 02/01/2017 05:15 PM, Kirill Tkhai wrote: > On 01.02.2017 17:12, Pavel Emelyanov wrote: >> On 01/28/2017 06:56 PM, Kirill Tkhai wrote: >>> According to man futex(2): >>> >>> "On all platforms, futexes are four-byte integers >>> that must be aligned on a four-byte boundary". >>> >>> So, allocate them aligned. >>> >>> Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com> >>> --- >>> criu/include/pstree.h | 9 +++++---- >>> criu/pstree.c | 1 + >>> 2 files changed, 6 insertions(+), 4 deletions(-) >>> >>> diff --git a/criu/include/pstree.h b/criu/include/pstree.h >>> index b4dc6b635..ee46a14db 100644 >>> --- a/criu/include/pstree.h >>> +++ b/criu/include/pstree.h >>> @@ -12,6 +12,11 @@ >>> */ >>> #define INIT_PID (1) >>> struct pstree_item { >>> + union { >>> + /* Must be 4 bytes aligned */ >>> + futex_t task_st; >>> + unsigned long task_st_le_bits; >>> + }; >> >> Ugh... Can't we make this more kernel-style? With the gcc attribute. > > It's a reminder, that the futex must be aligned. The note against > unaligned *pstree_item* allocation. Maybe, for the time, when someone > decides to change pstree_item allocation. > > Hm, maybe it'd be better to add something like this instead of the patch? No, let's pick the __aligned() macro from kernel and mark futex-es with it. > diff --git a/criu/pstree.c b/criu/pstree.c > index 6daf4069a..6d980c461 100644 > --- a/criu/pstree.c > +++ b/criu/pstree.c > @@ -221,6 +221,7 @@ struct pstree_item *__alloc_pstree_item(bool rst) > item->born_sid = -1; > futex_init(&item->task_st); > item->pid->item = item; > + BUG_ON(item & 3); /* Futex should be aligned */ > > return item; > } > . >
2017-02-01 17:23 GMT+03:00 Pavel Emelyanov <xemul@virtuozzo.com>: > On 02/01/2017 05:15 PM, Kirill Tkhai wrote: >> On 01.02.2017 17:12, Pavel Emelyanov wrote: >>> On 01/28/2017 06:56 PM, Kirill Tkhai wrote: >>>> According to man futex(2): >>>> >>>> "On all platforms, futexes are four-byte integers >>>> that must be aligned on a four-byte boundary". >>>> >>>> So, allocate them aligned. >>>> >>>> Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com> >>>> --- >>>> criu/include/pstree.h | 9 +++++---- >>>> criu/pstree.c | 1 + >>>> 2 files changed, 6 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/criu/include/pstree.h b/criu/include/pstree.h >>>> index b4dc6b635..ee46a14db 100644 >>>> --- a/criu/include/pstree.h >>>> +++ b/criu/include/pstree.h >>>> @@ -12,6 +12,11 @@ >>>> */ >>>> #define INIT_PID (1) >>>> struct pstree_item { >>>> + union { >>>> + /* Must be 4 bytes aligned */ >>>> + futex_t task_st; >>>> + unsigned long task_st_le_bits; >>>> + }; >>> >>> Ugh... Can't we make this more kernel-style? With the gcc attribute. >> >> It's a reminder, that the futex must be aligned. The note against >> unaligned *pstree_item* allocation. Maybe, for the time, when someone >> decides to change pstree_item allocation. >> >> Hm, maybe it'd be better to add something like this instead of the patch? > > No, let's pick the __aligned() macro from kernel and mark futex-es with it. Hmm, I think the idea with BUG_ON() is not that bad: AFAIU, the problem is not in the structure's member alignment, but in the alignment of the whole structure after dynamic allocation. So, BUG_ON() clearly stands to guard it at least in __alloc_pstree_item(). > >> diff --git a/criu/pstree.c b/criu/pstree.c >> index 6daf4069a..6d980c461 100644 >> --- a/criu/pstree.c >> +++ b/criu/pstree.c >> @@ -221,6 +221,7 @@ struct pstree_item *__alloc_pstree_item(bool rst) >> item->born_sid = -1; >> futex_init(&item->task_st); >> item->pid->item = item; >> + BUG_ON(item & 3); /* Futex should be aligned */ >> >> return item; >> } >> . >>
On 02/01/2017 05:27 PM, Dmitry Safonov wrote: > 2017-02-01 17:23 GMT+03:00 Pavel Emelyanov <xemul@virtuozzo.com>: >> On 02/01/2017 05:15 PM, Kirill Tkhai wrote: >>> On 01.02.2017 17:12, Pavel Emelyanov wrote: >>>> On 01/28/2017 06:56 PM, Kirill Tkhai wrote: >>>>> According to man futex(2): >>>>> >>>>> "On all platforms, futexes are four-byte integers >>>>> that must be aligned on a four-byte boundary". >>>>> >>>>> So, allocate them aligned. >>>>> >>>>> Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com> >>>>> --- >>>>> criu/include/pstree.h | 9 +++++---- >>>>> criu/pstree.c | 1 + >>>>> 2 files changed, 6 insertions(+), 4 deletions(-) >>>>> >>>>> diff --git a/criu/include/pstree.h b/criu/include/pstree.h >>>>> index b4dc6b635..ee46a14db 100644 >>>>> --- a/criu/include/pstree.h >>>>> +++ b/criu/include/pstree.h >>>>> @@ -12,6 +12,11 @@ >>>>> */ >>>>> #define INIT_PID (1) >>>>> struct pstree_item { >>>>> + union { >>>>> + /* Must be 4 bytes aligned */ >>>>> + futex_t task_st; >>>>> + unsigned long task_st_le_bits; >>>>> + }; >>>> >>>> Ugh... Can't we make this more kernel-style? With the gcc attribute. >>> >>> It's a reminder, that the futex must be aligned. The note against >>> unaligned *pstree_item* allocation. Maybe, for the time, when someone >>> decides to change pstree_item allocation. >>> >>> Hm, maybe it'd be better to add something like this instead of the patch? >> >> No, let's pick the __aligned() macro from kernel and mark futex-es with it. > > Hmm, I think the idea with BUG_ON() is not that bad: > AFAIU, the problem is not in the structure's member alignment, but > in the alignment of the whole structure after dynamic allocation. > So, BUG_ON() clearly stands to guard it at least in __alloc_pstree_item(). This is typically solved by introducing the MEMALLOC_ALIGN flag and manual alignment of the return pointer from allocators. -- Pavel