Re: [PATCH v3] EFI/runtime: switch to xv[mz]alloc_array()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 14.08.2025 08:46, Jan Beulich wrote:
> On 14.08.2025 02:29, Daniel P. Smith wrote:
>> On 8/12/25 02:12, Jan Beulich wrote:
>>> On 12.08.2025 02:19, Daniel P. Smith wrote:
>>>> On 7/23/25 09:39, Jan Beulich wrote:
>>>>> Use the more "modern" form, thus doing away with effectively open-coding
>>>>> xmalloc_array() at the same time. While there is a difference in
>>>>> generated code, as xmalloc_bytes() forces SMP_CACHE_BYTES alignment, if
>>>>> code really cared about such higher than default alignment, it should
>>>>> request so explicitly.
>>>>
>>>> While I don't object to the change itself, I think this description is a
>>>> bit over simplification of the change. If the allocation is under
>>>> PAGE_SIZE, then they are equivalent, but if it is over the page size
>>>> there are a few more differences than just cache alignment. It
>>>> completely changes the underlying allocator. I personally also find it a
>>>> bit of a stretch to call xmalloc_bytes(size) an open coded version of
>>>> xmalloc_array(char, size).
>>>
>>> My take is that xmalloc_bytes() should never have existed. Hence why I
>>> didn't add xzmalloc_bytes() when introducing that family of interfaces.
>>
>> Right, which would be a valid argument for replacing it with 
>> xmalloc_array(). Though, I would note that there is an xzalloc_bytes(). 
>> My concern was that you stated there was an open coding, which had me 
>> expecting there was a line of the form, xmanlloc_bytes(count * 
>> size_of_something bigger), being replaced by 
>> xvmalloc_arryay(something_bigger, count).
> 
> Both fir this and ...
> 
>> IMHO, while the C spec does specify char as 1 byte and thus 
>> interchangeable, I would agree that from a contextual perspective, 
>> xmalloc_array() is the more appropriate call. The use of the allocation 
>> is a character array and not a chunk of bytes for an arbitrary buffer.
> 
> ... for this: Hence my wording using "effectively".
> 
>>>> With a stronger description of the change,
>>>
>>> So what exactly do you mean by "stronger"? I can add that in the unlikely
>>> event that one of the allocations is (near) PAGE_SIZE or larger, we now
>>> wouldn't require contiguous memory anymore. Yet based on your comment at
>>> the top I'm not quite sure if that's what you're after and/or enough to
>>> satisfy your request.
>>
>> The phrasing stronger was meant to be more clear on the change/effect, 
>> specifically that the underlying allocator is being changed when the 
>> allocation is greater than a PAGE_SIZE. Not necessarily a long 
>> explanation, just the fact that the allocation will be coming from the 
>> dom heap allocator as opposed to the xen heap allocator. There are 
>> implications to changing the allocater, e.g.,  at a minimum the 
>> allocation order and nonphysical vs. physically contiguous effects. 
>> Having it noted in the commit makes it more obvious what this change is 
>> actually doing. Which may not be obvious when seeing the simple line 
>> changes occurring in the diff. Later, if there is an unexpected 
>> consequence caused by this change, a stronger commit will be helpful 
>> with the bisection investigations.
> 
> First: I don't think each and every such change (there are going to be many)
> should re-explain the switch to the xvmalloc() family of functions. This is
> already stated clearly at the top of xvmalloc.h: Over time, the entire code
> base is meant to be switched.
> 
> Beyond that, to achieve the stronger wording you're after, would it perhaps
> suffice to have the first sentence say "..., thus also doing away ..."?
> Otherwise, may I ask that you please make a more concrete suggestion?

Ping. I'd like to get this in, yet I still don't know how exactly to satisfy
your request for a stronger description.

Jan