From: Jan Beulich <jbeulich@suse.com>
To: Andrew Cooper <andrew.cooper3@citrix.com>, Julien Grall <julien@xen.org>
Cc: George Dunlap <George.Dunlap@eu.citrix.com>,
Ian Jackson <iwj@xenproject.org>, Wei Liu <wl@xen.org>,
Stefano Stabellini <sstabellini@kernel.org>,
"xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>
Subject: Re: [PATCH v2 7/8] lib: move bsearch code
Date: Tue, 24 Nov 2020 10:39:16 +0100 [thread overview]
Message-ID: <1d04fce7-beb4-9434-a528-b1cfdd07084b@suse.com> (raw)
In-Reply-To: <77534dc3-bdd6-f884-99e3-90dc9b02a81f@citrix.com>
On 24.11.2020 01:40, Andrew Cooper wrote:
> On 23/11/2020 22:49, Julien Grall wrote:
>> 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?
>
> Substantial.
>
>>
>> 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
>
> There is a very good reason why this is the classic example used for
> extern inline's in various libc's.
>
> The gains are from the compiler being able to optimise away the function
> pointer(s) entirely. Instead of working on opaque objects, it can see
> the accesses directly, implement compares as straight array reads, (for
> sorting, the swap() call turns into memcpy()) and because it can see all
> the memory accesses, doesn't have to assume that every call to cmp()
> modifies arbitrary data in the array (i.e. doesn't have to reload the
> objects from memory every iteration).
>
> extern inline allows the compiler full flexibility to judge whether
> inlining is a net win, based on optimisation settings and observing what
> the practical memory access pattern would be from not inlining.
>
> extern inline is the appropriate thing to use here, except for the big
> note in the GCC manual saying "always use gnu_inline in this case" which
> appears to be working around a change in the C99 standard which forces
> any non-static inline to emit a body even when its not called, due to
> rules about global symbols.
>
> Therefore, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
Thanks Andrew.
Julien - please clarify whether you're okay with Andrew's response,
or whether you continue to object the conversion to inline.
> On a totally separate point, I wonder if we'd be better off compiling
> with -fgnu89-inline because I can't see any case we're we'd want the C99
> inline semantics anywhere in Xen.
I'm not sure about this, i.e. I wouldn't want to exclude such a
case appearing. I think using attributes is better in general, as
it allows fine grained control.
Jan
next prev parent reply other threads:[~2020-11-24 9:39 UTC|newest]
Thread overview: 35+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-10-23 10:15 [PATCH v2 0/8] xen: beginnings of moving library-like code into an archive Jan Beulich
2020-10-23 10:16 ` [PATCH v2 1/8] lib: split _ctype[] into its own object, under lib/ Jan Beulich
2020-11-18 17:00 ` Julien Grall
2020-10-23 10:17 ` [PATCH v2 2/8] lib: collect library files in an archive Jan Beulich
2020-11-18 17:06 ` Julien Grall
2020-11-19 10:15 ` Jan Beulich
2020-11-18 17:31 ` Julien Grall
2020-11-19 10:44 ` Jan Beulich
2020-10-23 10:17 ` [PATCH v2 3/8] lib: move list sorting code Jan Beulich
2020-11-18 17:38 ` Julien Grall
2020-11-19 10:10 ` Jan Beulich
2020-10-23 10:17 ` [PATCH v2 4/8] lib: move parse_size_and_unit() Jan Beulich
2020-11-18 17:39 ` Julien Grall
2020-11-18 17:57 ` Julien Grall
2020-11-24 0:58 ` Andrew Cooper
2020-11-24 9:30 ` Jan Beulich
2020-10-23 10:18 ` [PATCH v2 5/8] lib: move init_constructors() Jan Beulich
2020-11-18 17:42 ` Julien Grall
2020-10-23 10:18 ` [PATCH v2 6/8] lib: move rbtree code Jan Beulich
2020-11-18 17:46 ` Julien Grall
2020-11-19 10:23 ` Jan Beulich
2020-10-23 10:19 ` [PATCH v2 7/8] lib: move bsearch code Jan Beulich
2020-11-18 18:09 ` Julien Grall
2020-11-19 10:27 ` Jan Beulich
2020-11-23 22:49 ` Julien Grall
2020-11-24 0:40 ` Andrew Cooper
2020-11-24 9:39 ` Jan Beulich [this message]
2020-11-24 16:57 ` Julien Grall
2020-12-07 10:23 ` Jan Beulich
2020-12-09 9:41 ` Julien Grall
2020-12-09 14:27 ` Bertrand Marquis
2020-12-09 14:54 ` Jan Beulich
2020-12-09 15:06 ` Bertrand Marquis
2020-10-23 10:19 ` [PATCH v2 8/8] lib: move sort code Jan Beulich
2020-11-18 18:10 ` Julien Grall
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1d04fce7-beb4-9434-a528-b1cfdd07084b@suse.com \
--to=jbeulich@suse.com \
--cc=George.Dunlap@eu.citrix.com \
--cc=andrew.cooper3@citrix.com \
--cc=iwj@xenproject.org \
--cc=julien@xen.org \
--cc=sstabellini@kernel.org \
--cc=wl@xen.org \
--cc=xen-devel@lists.xenproject.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).