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

[Julien Grall <julien@xen.org>]

Hi Jan,

On 14/04/2021 08:03, Jan Beulich wrote:
> On 13.04.2021 20:19, Julien Grall wrote:
>> On 08/04/2021 13:23, Jan Beulich wrote:
>>> There is a difference in generated code: xzalloc_bytes() forces
>>> SMP_CACHE_BYTES alignment. I think we not only don't need this here, but
>>> actually don't want it.
>>
>> So I think moving to xmalloc_flex_struct() is a pretty good move. But I
>> am actually a bit confused with the argument used.
>>
>> Could you provide some details why you think forcing the array to be
>> aligned to the maximum cache line supported (128 bytes on Arm) is wrong?
> 
> It is not "wrong" in that sense, but if this is intended it shouldn't
> be arranged via use of xmalloc_bytes().

This is not very clear from the commit message (or even the cover 
letter). How about:

"
The current use 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_flex_struct(). So 
if we want to prevent sharing a cache line, it should be part of 
x*alloc_flex_struct().

In this case, we don't need stricter alignment than what the allocator 
does. So the call to xzalloc_bytes() is now replaced with a call 
xzalloc_flex_Struct().
"

Ideally, we want the same sort of the commit message in the other patches.

> As also pointed out in a
> similar discussion on another sub-thread, imo xmalloc_bytes(), being
> type-unsafe, should go away altogether mid-term.

And I will support dropping xmalloc_bytes().

> If individual callers
> have specific alignment requirements (which ought to be the exception),
> they should explicitly request the needed alignment. If architectures
> would prefer all allocations to have certain minimum alignment (e.g.
> to avoid cacheline sharing, which was Andrew's argument) or other
> "arrangement" (alignment by itself may not be that interesting due to
> the bhdr placed ahead of the allocation), it should be the allocator
> itself that provides for this, not individual callers.

And I agree that the allocator should do the alignment if this benefits 
every allocation.

Cheers,

-- 
Julien Grall