[v2] assorted replacement of x[mz]alloc_bytes()

[PATCH v2 8/8] Arm/optee: don't open-code xzalloc_flex_struct()

Posted by Jan Beulich 4 years, 9 months ago

The current use of xzalloc_bytes() in optee is nearly an open-coded
version  of xzalloc_flex_struct(), which was introduced after the driver
was merged.

The main difference is xzalloc_bytes() will also force the allocation to
be SMP_CACHE_BYTES aligned and therefore avoid sharing the cache line.
While sharing the cache line can have an impact on the performance, this
is also true for most of the other users of x*alloc(), x*alloc_array(),
and x*alloc_flex_struct(). So if we want to prevent sharing cache lines,
arranging for this should be done in the allocator itself.

In this case, we don't need stricter alignment than what the allocator 
provides. Hence replace the call to xzalloc_bytes() with one of
xzalloc_flex_struct().

Signed-off-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: Julien Grall <julien@xen.org>
---
v2: Use commit message very close to what was suggested by Julien.

--- a/xen/arch/arm/tee/optee.c
+++ b/xen/arch/arm/tee/optee.c
@@ -529,8 +529,7 @@ static struct optee_shm_buf *allocate_op
     while ( unlikely(old != atomic_cmpxchg(&ctx->optee_shm_buf_pages,
                                            old, new)) );
 
-    optee_shm_buf = xzalloc_bytes(sizeof(struct optee_shm_buf) +
-                                  pages_cnt * sizeof(struct page *));
+    optee_shm_buf = xzalloc_flex_struct(struct optee_shm_buf, pages, pages_cnt);
     if ( !optee_shm_buf )
     {
         err_code = -ENOMEM;

Ping: [PATCH v2 8/8] Arm/optee: don't open-code xzalloc_flex_struct()

Posted by Jan Beulich 4 years, 9 months ago

Volodymyr,

On 21.04.2021 16:59, Jan Beulich wrote:
> The current use of xzalloc_bytes() in optee is nearly an open-coded
> version  of xzalloc_flex_struct(), which was introduced after the driver
> was merged.
> 
> The main difference is xzalloc_bytes() will also force the allocation to
> be SMP_CACHE_BYTES aligned and therefore avoid sharing the cache line.
> While sharing the cache line can have an impact on the performance, this
> is also true for most of the other users of x*alloc(), x*alloc_array(),
> and x*alloc_flex_struct(). So if we want to prevent sharing cache lines,
> arranging for this should be done in the allocator itself.
> 
> In this case, we don't need stricter alignment than what the allocator 
> provides. Hence replace the call to xzalloc_bytes() with one of
> xzalloc_flex_struct().
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> Signed-off-by: Julien Grall <julien@xen.org>
> ---
> v2: Use commit message very close to what was suggested by Julien.

I realize I forgot to Cc you on the v2 submission, but I didn't hear
back on v1 from you either. May I ask for an ack or otherwise?

Thanks, Jan

> --- a/xen/arch/arm/tee/optee.c
> +++ b/xen/arch/arm/tee/optee.c
> @@ -529,8 +529,7 @@ static struct optee_shm_buf *allocate_op
>      while ( unlikely(old != atomic_cmpxchg(&ctx->optee_shm_buf_pages,
>                                             old, new)) );
>  
> -    optee_shm_buf = xzalloc_bytes(sizeof(struct optee_shm_buf) +
> -                                  pages_cnt * sizeof(struct page *));
> +    optee_shm_buf = xzalloc_flex_struct(struct optee_shm_buf, pages, pages_cnt);
>      if ( !optee_shm_buf )
>      {
>          err_code = -ENOMEM;
> 
>

Re: Ping: [PATCH v2 8/8] Arm/optee: don't open-code xzalloc_flex_struct()

Posted by Volodymyr Babchuk 4 years, 9 months ago

Hi Jan,

Jan Beulich writes:

> On 21.04.2021 16:59, Jan Beulich wrote:
>> The current use of xzalloc_bytes() in optee is nearly an open-coded
>> version  of xzalloc_flex_struct(), which was introduced after the driver
>> was merged.
>> 
>> The main difference is xzalloc_bytes() will also force the allocation to
>> be SMP_CACHE_BYTES aligned and therefore avoid sharing the cache line.
>> While sharing the cache line can have an impact on the performance, this
>> is also true for most of the other users of x*alloc(), x*alloc_array(),
>> and x*alloc_flex_struct(). So if we want to prevent sharing cache lines,
>> arranging for this should be done in the allocator itself.
>> 
>> In this case, we don't need stricter alignment than what the allocator 
>> provides. Hence replace the call to xzalloc_bytes() with one of
>> xzalloc_flex_struct().
>> 
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> Signed-off-by: Julien Grall <julien@xen.org>
>> ---
>> v2: Use commit message very close to what was suggested by Julien.
>
> I realize I forgot to Cc you on the v2 submission, but I didn't hear
> back on v1 from you either. May I ask for an ack or otherwise?

You already had discussion on v1, so I just wanted to see how it will
conclude before stepping in.

I'm fine with this change:

Acked-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>

>
>> --- a/xen/arch/arm/tee/optee.c
>> +++ b/xen/arch/arm/tee/optee.c
>> @@ -529,8 +529,7 @@ static struct optee_shm_buf *allocate_op
>>      while ( unlikely(old != atomic_cmpxchg(&ctx->optee_shm_buf_pages,
>>                                             old, new)) );
>>  
>> -    optee_shm_buf = xzalloc_bytes(sizeof(struct optee_shm_buf) +
>> -                                  pages_cnt * sizeof(struct page *));
>> +    optee_shm_buf = xzalloc_flex_struct(struct optee_shm_buf, pages, pages_cnt);
>>      if ( !optee_shm_buf )
>>      {
>>          err_code = -ENOMEM;
>> 
>> 


-- 
Volodymyr Babchuk at EPAM