Re: [PATCH v2 7/8] lib: move bsearch code

[Julien Grall <julien@xxxxxxx>]

Hi Jan,

On 19/11/2020 10:27, Jan Beulich wrote:
> On 18.11.2020 19:09, Julien Grall wrote:
> > On 23/10/2020 11:19, Jan Beulich wrote:
> > > --- a/xen/include/xen/compiler.h
> > > +++ b/xen/include/xen/compiler.h
> > > @@ -12,6 +12,7 @@
> > >    
> > >    #define inline        __inline__
> > >    #define always_inline __inline__ __attribute__ ((__always_inline__))
> > > +#define gnu_inline    __inline__ __attribute__ ((__gnu_inline__))
> > bsearch() is only used by Arm and I haven't seen anyone so far
> > complaining about the perf of I/O emulation.
> >
> > Therefore, I am not convinced that there is enough justification to
> > introduce a GNU attribute just for this patch.
> Please settle this with Andrew: He had asked for the function to
> become inline. I don't view making it static inline in the header
> as an option here - if the compiler decides to not inline it, we
> should not end up with multiple instances in different CUs.
That's the cons of static inline... but then why is it suddenly a 
problem with this helper?
> And
> without making it static inline the attribute needs adding; at
> least I'm unaware of an alternative which works with the various
> compiler versions.
The question we have to answer is: What is the gain with this approach?

If it is not quantifiable, then introducing compiler specific attribute 
is not an option.
IIRC, there are only two callers (all in Arm code) of this function. 
Even inlined, I don't believe you would drastically reduce the number of 
instructions compare to a full blown version. To be generous, I would 
say you may save ~20 instructions per copy.
Therefore, so far, the compiler specific attribute doesn't look 
justified to me. As usual, I am happy to be proven wrong.
Cheers,

--
Julien Grall