Re: [Xen-devel] [PATCH] xen/typesafe: Force helpers to be always_inline

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On 10/7/19 10:25 AM, Jan Beulich wrote:
> On 04.10.2019 19:02, George Dunlap wrote:
>> On 10/2/19 9:11 AM, Jan Beulich wrote:
>>> On 01.10.2019 22:59, Andrew Cooper wrote:
>>>> On 01/10/2019 09:38, Jan Beulich wrote:
>>>>> On 30.09.2019 21:16, Andrew Cooper wrote:
>>>>>> Clang in particular has a habit of out-of-lining these and creating 
>>>>>> multiple
>>>>>> local copies of _mfn() and mfn_x(), etc.  Override this behaviour.
>>>>> Is special casing the typesafe helpers then the right approach? The
>>>>> fundamental idea after all is to let the compiler decide. I certainly
>>>>> agree that not inlining such trivial functions despite the inline
>>>>> keyword looks far from optimal, but if there's such a general issue
>>>>> with clang, shouldn't we make "inline" expand to "always_inline"
>>>>> uniformly?
>>>>
>>>> Inline handing is a mess.
>>>>
>>>> We currently define inline to __inline__.  Undoing this results in build
>>>> failures.
>>>>
>>>> Linux currently defines inline to always_inline and they are desperately
>>>> trying to undo this (mis)behaviour.
>>>>
>>>> There are a few uses of always_inline for safety purposes (the
>>>> speculative helpers).  Most uses of always_inline look to be workarounds
>>>> for the size-of-asm bug/(mis)feature.
>>>>
>>>> In an ideal world, we wouldn't need it at all, but I definitely don't
>>>> think that taking the Linux approach is a clever move.  We definitely
>>>> have some static inlines which would better not being inline.
>>>
>>> IOW your suggested approach (at least for the foreseeable future) is to
>>> do what you do here and convert inline to always_inline as we see fit?
>>> If so, we should at least settle on some sufficiently firm criteria by
>>> which such a conversion would be justifiable.
>>>
>>> Seeing that this is primarily to help clang - did you consider
>>> introducing something like clang_inline, expanding to just inline for
>>> gcc, but always_inline for clang? This would at least provide a
>>> sufficiently easy way to undo this if a better clang-side approach can
>>> be found down the road.
>>
>> What would be the point of this?  The only reason always_inline isn't
>> necessary for gcc (if I'm following the argument) is because it so far
>> has always inlined these functions.  If it stopped inlining them, we'd
>> need to change it to always_inline anyway; so why not just say so to
>> begin with?
> 
> The point of this would be to _avoid_ using always_inline as much as
> possible. We really shouldn't fight compiler decisions more than
> absolutely necessary. Hence also my request for sufficiently firm
> criteria when to switch in the first place. Or else would could, as
> mentioned as an option elsewhere, make inline expand to always_inline
> uniformly. (Or course, even always_inline isn't a guarantee for the
> compiler to actually inline a function.)

Every time I try to compose an answer to this paragraph, I end up
writing the paragraph you responded to.  Let me try something else.

"We really shouldn't fight compiler decisions more than absolutely
necessary."

Sure.  But in this particular case, it's been determined that we want to
fight the compiler decision.  The reason for wanting it to be inline
doesn't depend on whether it's clang or gcc; we want it to be inlined
all the time no matter what.  So why go through the effort of inventing
a new thing targeted at clang?

Let's do a cost-benefits analysis of always_inline vs clang_inline.

For each future gcc version, either it will choose to inline this
function with the `inline` key word or not.

1. Use always_inline
 1a. gcc would have done inline anyway.  No cost.
 1b. gcc would not have inlined it.  always_inline takes effect and
there's no cost.
2. Use clang_inline
 2a. gcc would have done inline anyway.  No cost.
 2b. gcc doesn't inline it.  We have random bugs, a discussion, then a
patch to s/clang_inline/always_inline/g;.

IOW, I only see a cost here to 2, and no benefit.

"Hence also my request for sufficiently firm criteria when to switch in
the first place."

Having criteria describing exactly when we want to specify always_inline
would be nice.  But it doesn't change the fact that in this case, we
want it to be always inlined.

"Or else would could, as mentioned as an option elsewhere, make inline
expand to always_inline uniformly."

But you just said we don't want to fight compiler decisions more than
absolutely necessary.

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel