Re: [PATCH 3/5] xen/sort: Switch to an extern inline implementation

[Andrew Cooper <amc96@xxxxxxxx>]

On 16/11/2021 00:41, Andrew Cooper wrote:
> On 16/11/2021 00:36, Stefano Stabellini wrote:
>> On Thu, 11 Nov 2021, Julien Grall wrote:
>>> On 11/11/2021 17:57, Andrew Cooper wrote:
>>>> There are exactly 3 callers of sort() in the hypervisor.
>>>>
>>>> Both arm callers pass in NULL for the swap function.  While this might seem
>>>> like an attractive option at first, it causes generic_swap() to be used
>>>> which
>>>> forced a byte-wise copy.  Provide real swap functions which the compiler 
>>>> can
>>>> optimise sensibly.
>>> I understand the theory, but none of the two calls are in hot paths or deal
>>> with large set on Arm. So I am rather hesitant to introduce specialised swap
>>> in each case as it doesn't seem we will gain much from this change.
>> While I like Andrew's optimization, like Julien, I would also rather not
>> have to introduce specialized swap functions any time a sort() is
>> called. Why not keeping around generic_swap as extern gnu_inline in
>> sort.h as well so that the ARM callers can simply:
>>
>>     sort(bootinfo.mem.bank, bootinfo.mem.nr_banks, sizeof(struct membank),
>>          cmp_memory_node, generic_swap);
>>
>> ?
>>
>> That looks like a good option, although it would still result in a
>> small increase in bloat.
> That is basically what Jan suggested.
>
> I'm still unconvinced that you actually want to be doing byte-wise
> swapping, even if it isn't a hotpath.  A custom swap function will
> compile to less code than using generic_swap() like this, and run faster.

ARM Ping.

I really think this is a bad course of action.  If you're going to
insist on it, I want something in writing.

~Andrew