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
