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

[Julien Grall <julien@xxxxxxx>]

Hi Andrew,

On 17/12/2021 15:56, Andrew Cooper wrote:
> 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.
I agree with all the points you wrote. However, you also have to put 
into perspective how this is used. On Arm, the two callers happen either 
during boot a domain creation. Neither of them have a significant impact 
on the time spent. In fact, I would be surprised if you notice the change.
So unless there are other (good) reasons to suppress generic_swap(), I 
still want to be able to use generic_swap() when the performance doesn't 
matter (like the two Arm callers).
Cheers,

--
Julien Grall