pstree: Aligned pstree item allocation

Submitted by Kirill Tkhai on Jan. 28, 2017, 3:56 p.m.

Details

Message ID 148561895682.19279.17651348327492043847.stgit@localhost.localdomain
State Superseded
Series "pstree: Aligned pstree item allocation"
Headers show

Commit Message

Kirill Tkhai Jan. 28, 2017, 3:56 p.m.
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(-)

Patch hide | download patch | download mbox

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;
+	};
 	struct pstree_item	*parent;
 	struct list_head	children;	/* list of my children */
 	struct list_head	sibling;	/* linkage in my parent's children list */
@@ -25,10 +30,6 @@  struct pstree_item {
 	struct pid		*threads;	/* array of threads */
 	CoreEntry		**core;
 	TaskKobjIdsEntry	*ids;
-	union {
-		futex_t		task_st;
-		unsigned long	task_st_le_bits;
-	};
 };
 
 enum {
diff --git a/criu/pstree.c b/criu/pstree.c
index 8eeea947c..bee9c8917 100644
--- a/criu/pstree.c
+++ b/criu/pstree.c
@@ -203,6 +203,7 @@  struct pstree_item *__alloc_pstree_item(bool rst)
 			return NULL;
 	} else {
 		sz = sizeof(*item) + sizeof(struct rst_info);
+		/* Guarantees sizeof(void *) alignment */
 		item = shmalloc(sz);
 		if (!item)
 			return NULL;

Comments

Andrey Vagin Jan. 30, 2017, 8:45 p.m.
On Sat, Jan 28, 2017 at 06:56:21PM +0300, 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.

Why is it not aligned without this patch?
> 
> 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;
> +	};
>  	struct pstree_item	*parent;
>  	struct list_head	children;	/* list of my children */
>  	struct list_head	sibling;	/* linkage in my parent's children list */
> @@ -25,10 +30,6 @@ struct pstree_item {
>  	struct pid		*threads;	/* array of threads */
>  	CoreEntry		**core;
>  	TaskKobjIdsEntry	*ids;
> -	union {
> -		futex_t		task_st;
> -		unsigned long	task_st_le_bits;
> -	};
>  };
>  
>  enum {
> diff --git a/criu/pstree.c b/criu/pstree.c
> index 8eeea947c..bee9c8917 100644
> --- a/criu/pstree.c
> +++ b/criu/pstree.c
> @@ -203,6 +203,7 @@ struct pstree_item *__alloc_pstree_item(bool rst)
>  			return NULL;
>  	} else {
>  		sz = sizeof(*item) + sizeof(struct rst_info);
> +		/* Guarantees sizeof(void *) alignment */
>  		item = shmalloc(sz);
>  		if (!item)
>  			return NULL;
> 
> _______________________________________________
> CRIU mailing list
> CRIU@openvz.org
> https://lists.openvz.org/mailman/listinfo/criu
Kirill Tkhai Jan. 31, 2017, 8:58 a.m.
On 30.01.2017 23:45, Andrei Vagin wrote:
> On Sat, Jan 28, 2017 at 06:56:21PM +0300, 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.
> 
> Why is it not aligned without this patch?

This patch points that in clear way!

>>
>> 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;
>> +	};
>>  	struct pstree_item	*parent;
>>  	struct list_head	children;	/* list of my children */
>>  	struct list_head	sibling;	/* linkage in my parent's children list */
>> @@ -25,10 +30,6 @@ struct pstree_item {
>>  	struct pid		*threads;	/* array of threads */
>>  	CoreEntry		**core;
>>  	TaskKobjIdsEntry	*ids;
>> -	union {
>> -		futex_t		task_st;
>> -		unsigned long	task_st_le_bits;
>> -	};
>>  };
>>  
>>  enum {
>> diff --git a/criu/pstree.c b/criu/pstree.c
>> index 8eeea947c..bee9c8917 100644
>> --- a/criu/pstree.c
>> +++ b/criu/pstree.c
>> @@ -203,6 +203,7 @@ struct pstree_item *__alloc_pstree_item(bool rst)
>>  			return NULL;
>>  	} else {
>>  		sz = sizeof(*item) + sizeof(struct rst_info);
>> +		/* Guarantees sizeof(void *) alignment */
>>  		item = shmalloc(sz);
>>  		if (!item)
>>  			return NULL;
>>
>> _______________________________________________
>> CRIU mailing list
>> CRIU@openvz.org
>> https://lists.openvz.org/mailman/listinfo/criu
Andrey Vagin Feb. 1, 2017, 1:14 a.m.
On Tue, Jan 31, 2017 at 11:58:13AM +0300, Kirill Tkhai wrote:
> On 30.01.2017 23:45, Andrei Vagin wrote:
> > On Sat, Jan 28, 2017 at 06:56:21PM +0300, 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.
> > 
> > Why is it not aligned without this patch?
> 
> This patch points that in clear way!

The C Standard guarantees that all fields in structures aligned, doesn't
it? I don't have objections about this patch, just to know that I don't
skip something.

> 
> >>
> >> 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;
> >> +	};
> >>  	struct pstree_item	*parent;
> >>  	struct list_head	children;	/* list of my children */
> >>  	struct list_head	sibling;	/* linkage in my parent's children list */
> >> @@ -25,10 +30,6 @@ struct pstree_item {
> >>  	struct pid		*threads;	/* array of threads */
> >>  	CoreEntry		**core;
> >>  	TaskKobjIdsEntry	*ids;
> >> -	union {
> >> -		futex_t		task_st;
> >> -		unsigned long	task_st_le_bits;
> >> -	};
> >>  };
> >>  
> >>  enum {
> >> diff --git a/criu/pstree.c b/criu/pstree.c
> >> index 8eeea947c..bee9c8917 100644
> >> --- a/criu/pstree.c
> >> +++ b/criu/pstree.c
> >> @@ -203,6 +203,7 @@ struct pstree_item *__alloc_pstree_item(bool rst)
> >>  			return NULL;
> >>  	} else {
> >>  		sz = sizeof(*item) + sizeof(struct rst_info);
> >> +		/* Guarantees sizeof(void *) alignment */
> >>  		item = shmalloc(sz);
> >>  		if (!item)
> >>  			return NULL;
> >>
> >> _______________________________________________
> >> CRIU mailing list
> >> CRIU@openvz.org
> >> https://lists.openvz.org/mailman/listinfo/criu
Pavel Emelianov Feb. 1, 2017, 2:12 p.m.
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.

>  	struct pstree_item	*parent;
>  	struct list_head	children;	/* list of my children */
>  	struct list_head	sibling;	/* linkage in my parent's children list */
> @@ -25,10 +30,6 @@ struct pstree_item {
>  	struct pid		*threads;	/* array of threads */
>  	CoreEntry		**core;
>  	TaskKobjIdsEntry	*ids;
> -	union {
> -		futex_t		task_st;
> -		unsigned long	task_st_le_bits;
> -	};
>  };
>  
>  enum {
> diff --git a/criu/pstree.c b/criu/pstree.c
> index 8eeea947c..bee9c8917 100644
> --- a/criu/pstree.c
> +++ b/criu/pstree.c
> @@ -203,6 +203,7 @@ struct pstree_item *__alloc_pstree_item(bool rst)
>  			return NULL;
>  	} else {
>  		sz = sizeof(*item) + sizeof(struct rst_info);
> +		/* Guarantees sizeof(void *) alignment */
>  		item = shmalloc(sz);
>  		if (!item)
>  			return NULL;
> 
> .
>
Kirill Tkhai Feb. 1, 2017, 2:17 p.m.
On 01.02.2017 04:14, Andrei Vagin wrote:
> On Tue, Jan 31, 2017 at 11:58:13AM +0300, Kirill Tkhai wrote:
>> On 30.01.2017 23:45, Andrei Vagin wrote:
>>> On Sat, Jan 28, 2017 at 06:56:21PM +0300, 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.
>>>
>>> Why is it not aligned without this patch?
>>
>> This patch points that in clear way!
> 
> The C Standard guarantees that all fields in structures aligned, doesn't
> it? I don't have objections about this patch, just to know that I don't
> skip something.

You sure, and patch does not change something in this way.
But it may be a reminder for a person, who changes allocator.
I'm not insist on this.
 
>>
>>>>
>>>> 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;
>>>> +	};
>>>>  	struct pstree_item	*parent;
>>>>  	struct list_head	children;	/* list of my children */
>>>>  	struct list_head	sibling;	/* linkage in my parent's children list */
>>>> @@ -25,10 +30,6 @@ struct pstree_item {
>>>>  	struct pid		*threads;	/* array of threads */
>>>>  	CoreEntry		**core;
>>>>  	TaskKobjIdsEntry	*ids;
>>>> -	union {
>>>> -		futex_t		task_st;
>>>> -		unsigned long	task_st_le_bits;
>>>> -	};
>>>>  };
>>>>  
>>>>  enum {
>>>> diff --git a/criu/pstree.c b/criu/pstree.c
>>>> index 8eeea947c..bee9c8917 100644
>>>> --- a/criu/pstree.c
>>>> +++ b/criu/pstree.c
>>>> @@ -203,6 +203,7 @@ struct pstree_item *__alloc_pstree_item(bool rst)
>>>>  			return NULL;
>>>>  	} else {
>>>>  		sz = sizeof(*item) + sizeof(struct rst_info);
>>>> +		/* Guarantees sizeof(void *) alignment */
>>>>  		item = shmalloc(sz);
>>>>  		if (!item)
>>>>  			return NULL;
>>>>
>>>> _______________________________________________
>>>> CRIU mailing list
>>>> CRIU@openvz.org
>>>> https://lists.openvz.org/mailman/listinfo/criu