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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 15.04.2021 12:26, Julien Grall wrote:
> 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().
> "

Well, okay, I've inserted a slightly edited version of this into
the patch. But ...

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

... I disagree here: In particular because of the recurring
pattern, I dislike repeated overly verbose descriptions. I could
perhaps see them as being not seen as overly verbose when looking
at every commit on its own (like would happen when someone tries
to do some archaeology later on), but in the context of such a
series this is potentially harmful: If almost a dozen patches
have almost the same sufficiently verbose description, possible
differences may not be paid attention to.

Plus, granted possibly controversial as well, I'm afraid I'm not
happy to (repeatedly) state obvious facts. The abuse (in almost
all cases) of x[mz]alloc_bytes() is, afaict, in no way attributed
to the resulting higher alignment, but rather in the desire to
get away easily without needing to think about using a type-safe
flavor. This said I will admit that prior to the introduction of
x[mz]alloc_flex_struct() I can see how alternatives could quickly
have got quite ugly. And for the few (if any) users which
actually care about this higher alignment, it should have been
noted at the time of introducing the use that the specific need
for certain alignment shouldn't be implied by using this function,
but rather be made explicit. This is even more so as it's not
written down anywhere that x[mz]alloc_bytes() guarantees a certain
alignment (i.e. if the plan wasn't anyway to phase out its use, it
should have been permissible to change the alignment it requests
from the underlying implementation without first auditing all
users).

Jan