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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 21.10.2019 13:10, George Dunlap wrote:
> 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.

No, I don't think that's the case. Code generation should still be
left to the compiler. It's a side effect of not inlining that we
want to address here - symbol name collisions getting in the way
of live patch generation. I've already indicated I'd be happy to
deal with the root of the problem instead, i.e. avoiding the name
collisions.

>  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.

The benefit of 2 would be the easier way of identifying what was
changed just for clang's sake, perhaps with the simple goal of
reverting the whole lot without having to fish out all the individual
commits that may accumulate over time.

> "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.

Right - hence the "Or else" starting he sentence.

Jan

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