pstree: Aligned pstree item allocation

Submitted by Kirill Tkhai on Feb. 1, 2017, 2:15 p.m.

Details

Message ID 9f896c31-9c3a-a3a9-a994-6b818b403c9c@virtuozzo.com
State Superseded
Series "pstree: Aligned pstree item allocation"
Headers show

Commit Message

Kirill Tkhai Feb. 1, 2017, 2:15 p.m.
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?

Patch hide | download patch | download mbox

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

Comments

Pavel Emelianov Feb. 1, 2017, 2:23 p.m.
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;
>  }
> .
>
Dmitry Safonov Feb. 1, 2017, 2:27 p.m.
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;
>>  }
>> .
>>
Pavel Emelianov Feb. 1, 2017, 3:38 p.m.
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