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

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

[Andrew Cooper]

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.

Adjust bool_t to bool for the *_eq() helpers.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: George Dunlap <George.Dunlap@eu.citrix.com>
CC: Ian Jackson <ian.jackson@citrix.com>
CC: Jan Beulich <JBeulich@suse.com>
CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Tim Deegan <tim@xen.org>
CC: Wei Liu <wl@xen.org>
CC: Julien Grall <julien@xen.org>
CC: Juergen Gross <jgross@suse.com>

Spotted while looking at the code generation of evalute_nospec()

Semi-RFC for-4.13.  Nothing (currently un-broken) will break without this, but
it is a step on the way to being able to use Clang and Livepatch in
combination.
---
 xen/include/xen/iommu.h    |  4 ++--
 xen/include/xen/mm.h       | 16 ++++++++--------
 xen/include/xen/typesafe.h |  8 ++++----
 3 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
index 974bd3ffe8..c77b8c1a22 100644
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -42,12 +42,12 @@ TYPE_SAFE(uint64_t, dfn);
 #undef dfn_x
 #endif
 
-static inline dfn_t dfn_add(dfn_t dfn, unsigned long i)
+static always_inline dfn_t dfn_add(dfn_t dfn, unsigned long i)
 {
     return _dfn(dfn_x(dfn) + i);
 }
 
-static inline bool_t dfn_eq(dfn_t x, dfn_t y)
+static always_inline bool dfn_eq(dfn_t x, dfn_t y)
 {
     return dfn_x(x) == dfn_x(y);
 }
diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
index 8d0ddfb60c..5617ecc607 100644
--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -77,22 +77,22 @@ TYPE_SAFE(unsigned long, mfn);
 #undef mfn_x
 #endif
 
-static inline mfn_t mfn_add(mfn_t mfn, unsigned long i)
+static always_inline mfn_t mfn_add(mfn_t mfn, unsigned long i)
 {
     return _mfn(mfn_x(mfn) + i);
 }
 
-static inline mfn_t mfn_max(mfn_t x, mfn_t y)
+static always_inline mfn_t mfn_max(mfn_t x, mfn_t y)
 {
     return _mfn(max(mfn_x(x), mfn_x(y)));
 }
 
-static inline mfn_t mfn_min(mfn_t x, mfn_t y)
+static always_inline mfn_t mfn_min(mfn_t x, mfn_t y)
 {
     return _mfn(min(mfn_x(x), mfn_x(y)));
 }
 
-static inline bool_t mfn_eq(mfn_t x, mfn_t y)
+static always_inline bool mfn_eq(mfn_t x, mfn_t y)
 {
     return mfn_x(x) == mfn_x(y);
 }
@@ -115,22 +115,22 @@ TYPE_SAFE(unsigned long, gfn);
 #undef gfn_x
 #endif
 
-static inline gfn_t gfn_add(gfn_t gfn, unsigned long i)
+static always_inline gfn_t gfn_add(gfn_t gfn, unsigned long i)
 {
     return _gfn(gfn_x(gfn) + i);
 }
 
-static inline gfn_t gfn_max(gfn_t x, gfn_t y)
+static always_inline gfn_t gfn_max(gfn_t x, gfn_t y)
 {
     return _gfn(max(gfn_x(x), gfn_x(y)));
 }
 
-static inline gfn_t gfn_min(gfn_t x, gfn_t y)
+static always_inline gfn_t gfn_min(gfn_t x, gfn_t y)
 {
     return _gfn(min(gfn_x(x), gfn_x(y)));
 }
 
-static inline bool_t gfn_eq(gfn_t x, gfn_t y)
+static always_inline bool gfn_eq(gfn_t x, gfn_t y)
 {
     return gfn_x(x) == gfn_x(y);
 }
diff --git a/xen/include/xen/typesafe.h b/xen/include/xen/typesafe.h
index 7ecd3b4a8d..f242500063 100644
--- a/xen/include/xen/typesafe.h
+++ b/xen/include/xen/typesafe.h
@@ -21,15 +21,15 @@
 
 #define TYPE_SAFE(_type, _name)                                         \
     typedef struct { _type _name; } _name##_t;                          \
-    static inline _name##_t _##_name(_type n) { return (_name##_t) { n }; } \
-    static inline _type _name##_x(_name##_t n) { return n._name; }
+    static always_inline _name##_t _##_name(_type n) { return (_name##_t) { n }; } \
+    static always_inline _type _name##_x(_name##_t n) { return n._name; }
 
 #else
 
 #define TYPE_SAFE(_type, _name)                                         \
     typedef _type _name##_t;                                            \
-    static inline _name##_t _##_name(_type n) { return n; }             \
-    static inline _type _name##_x(_name##_t n) { return n; }
+    static always_inline _name##_t _##_name(_type n) { return n; }      \
+    static always_inline _type _name##_x(_name##_t n) { return n; }
 
 #endif
 
-- 
2.11.0


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

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

[Jürgen Groß]

On 30.09.19 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.
> 
> Adjust bool_t to bool for the *_eq() helpers.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Release-acked-by: Juergen Gross <jgross@suse.com>


Juergen

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

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

[Jan Beulich]

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?

Jan

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

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

[Andrew Cooper]

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.

~Andrew

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

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

[Jan Beulich]

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.

Furthermore, wouldn't the livepatch aspect of this be taken care of
by my plan to prefix filenames (in turn prefixing static symbol names
in our kallsyms) with their (relative) paths? If so, rather than
furthering the mess here, should I see about actually making this
work (addressing a wider range of cases, including gcc compiled code
in a few instances)?

Jan

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

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

[George Dunlap]

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?

 -George

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

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

[Jan Beulich]

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

Jan

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

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

[George Dunlap]

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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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

[Jan Beulich]

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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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

[George Dunlap]

On 10/23/19 8:27 AM, Jan Beulich wrote:
> 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.

But they weren't done for clang's sake; they were done for symbol clash
sake.  If some future version of gcc happened to not inline these, then
we'd get the same problem.

So if you want to take this approach, I'd say make the names something
like, `inline_symbol_clash`.  That tells future people exactly what
needs to be done to switch them back to "bare" inline; and if we never
get around to fixing the symbol clash issue, then it will prevent gcc
from "developing" this issue as well.

-George

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

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

[Jan Beulich]

On 23.10.2019 11:37, George Dunlap wrote:
> On 10/23/19 8:27 AM, Jan Beulich wrote:
>> On 21.10.2019 13:10, George Dunlap wrote:
>>> 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.
> 
> But they weren't done for clang's sake; they were done for symbol clash
> sake.  If some future version of gcc happened to not inline these, then
> we'd get the same problem.
> 
> So if you want to take this approach, I'd say make the names something
> like, `inline_symbol_clash`.  That tells future people exactly what
> needs to be done to switch them back to "bare" inline; and if we never
> get around to fixing the symbol clash issue, then it will prevent gcc
> from "developing" this issue as well.

Oh, yes, good point.

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