* [PATCH] xen: Rework WARN_ON() to return whether a warning was triggered
@ 2020-12-15 11:26 Julien Grall
2020-12-15 11:31 ` Jürgen Groß
2020-12-15 11:46 ` Jan Beulich
0 siblings, 2 replies; 13+ messages in thread
From: Julien Grall @ 2020-12-15 11:26 UTC (permalink / raw)
To: xen-devel
Cc: bertrand.marquis, Rahul.Singh, Julien Grall, Andrew Cooper,
George Dunlap, Ian Jackson, Jan Beulich, Julien Grall,
Stefano Stabellini, Wei Liu
From: Julien Grall <jgrall@amazon.com>
So far, our implementation of WARN_ON() cannot be used in the following
situation:
if ( WARN_ON() )
...
This is because the WARN_ON() doesn't return whether a warning. Such
construction can be handy to have if you have to print more information
and now the stack track.
Rework the WARN_ON() implementation to return whether a warning was
triggered. The idea was borrowed from Linux.
Signed-off-by: Julien Grall <jgrall@amazon.com>
---
This will be used in the SMMUv3 driver (see [1]).
---
xen/include/xen/lib.h | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/xen/include/xen/lib.h b/xen/include/xen/lib.h
index a9679c913d5c..d10c68aa3c07 100644
--- a/xen/include/xen/lib.h
+++ b/xen/include/xen/lib.h
@@ -23,7 +23,13 @@
#include <asm/bug.h>
#define BUG_ON(p) do { if (unlikely(p)) BUG(); } while (0)
-#define WARN_ON(p) do { if (unlikely(p)) WARN(); } while (0)
+#define WARN_ON(p) ({ \
+ bool __ret_warn_on = (p); \
+ \
+ if ( unlikely(__ret_warn_on) ) \
+ WARN(); \
+ unlikely(__ret_warn_on); \
+})
/* All clang versions supported by Xen have _Static_assert. */
#if defined(__clang__) || \
--
2.17.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] xen: Rework WARN_ON() to return whether a warning was triggered
2020-12-15 11:26 [PATCH] xen: Rework WARN_ON() to return whether a warning was triggered Julien Grall
@ 2020-12-15 11:31 ` Jürgen Groß
2020-12-15 13:11 ` Julien Grall
2020-12-15 11:46 ` Jan Beulich
1 sibling, 1 reply; 13+ messages in thread
From: Jürgen Groß @ 2020-12-15 11:31 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: bertrand.marquis, Rahul.Singh, Julien Grall, Andrew Cooper,
George Dunlap, Ian Jackson, Jan Beulich, Stefano Stabellini,
Wei Liu
[-- Attachment #1.1.1: Type: text/plain, Size: 665 bytes --]
On 15.12.20 12:26, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
>
> So far, our implementation of WARN_ON() cannot be used in the following
> situation:
>
> if ( WARN_ON() )
> ...
>
> This is because the WARN_ON() doesn't return whether a warning. Such
... warning has been triggered.
> construction can be handy to have if you have to print more information
> and now the stack track.
Sorry, I'm not able to parse that sentence.
>
> Rework the WARN_ON() implementation to return whether a warning was
> triggered. The idea was borrowed from Linux.
>
> Signed-off-by: Julien Grall <jgrall@amazon.com>
Juergen
[-- Attachment #1.1.2: OpenPGP_0xB0DE9DD628BF132F.asc --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] xen: Rework WARN_ON() to return whether a warning was triggered
2020-12-15 11:26 [PATCH] xen: Rework WARN_ON() to return whether a warning was triggered Julien Grall
2020-12-15 11:31 ` Jürgen Groß
@ 2020-12-15 11:46 ` Jan Beulich
2020-12-15 13:19 ` Julien Grall
1 sibling, 1 reply; 13+ messages in thread
From: Jan Beulich @ 2020-12-15 11:46 UTC (permalink / raw)
To: Julien Grall
Cc: bertrand.marquis, Rahul.Singh, Julien Grall, Andrew Cooper,
George Dunlap, Ian Jackson, Stefano Stabellini, Wei Liu,
xen-devel
On 15.12.2020 12:26, Julien Grall wrote:
> --- a/xen/include/xen/lib.h
> +++ b/xen/include/xen/lib.h
> @@ -23,7 +23,13 @@
> #include <asm/bug.h>
>
> #define BUG_ON(p) do { if (unlikely(p)) BUG(); } while (0)
> -#define WARN_ON(p) do { if (unlikely(p)) WARN(); } while (0)
> +#define WARN_ON(p) ({ \
> + bool __ret_warn_on = (p); \
Please can you avoid leading underscores here?
> + \
> + if ( unlikely(__ret_warn_on) ) \
> + WARN(); \
> + unlikely(__ret_warn_on); \
> +})
Is this latter unlikely() having any effect? So far I thought it
would need to be immediately inside a control construct or be an
operand to && or ||.
Jan
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] xen: Rework WARN_ON() to return whether a warning was triggered
2020-12-15 11:31 ` Jürgen Groß
@ 2020-12-15 13:11 ` Julien Grall
2020-12-15 16:20 ` Jürgen Groß
2020-12-17 17:58 ` Bertrand Marquis
0 siblings, 2 replies; 13+ messages in thread
From: Julien Grall @ 2020-12-15 13:11 UTC (permalink / raw)
To: Jürgen Groß, xen-devel
Cc: bertrand.marquis, Rahul.Singh, Julien Grall, Andrew Cooper,
George Dunlap, Ian Jackson, Jan Beulich, Stefano Stabellini,
Wei Liu
Hi Juergen,
On 15/12/2020 11:31, Jürgen Groß wrote:
> On 15.12.20 12:26, Julien Grall wrote:
>> From: Julien Grall <jgrall@amazon.com>
>>
>> So far, our implementation of WARN_ON() cannot be used in the following
>> situation:
>>
>> if ( WARN_ON() )
>> ...
>>
>> This is because the WARN_ON() doesn't return whether a warning. Such
>
> ... warning has been triggered.
I will add it.
>
>> construction can be handy to have if you have to print more information
>> and now the stack track.
>
> Sorry, I'm not able to parse that sentence.
Urgh :/. How about the following commit message:
"So far, our implementation of WARN_ON() cannot be used in the following
situation:
if ( WARN_ON() )
...
This is because WARN_ON() doesn't return whether a warning has been
triggered. Such construciton can be handy if you want to print more
information and also dump the stack trace.
Therefore, rework the WARN_ON() implementation to return whether a
warning was triggered. The idea was borrowed from Linux".
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] xen: Rework WARN_ON() to return whether a warning was triggered
2020-12-15 11:46 ` Jan Beulich
@ 2020-12-15 13:19 ` Julien Grall
2020-12-15 14:01 ` Jan Beulich
0 siblings, 1 reply; 13+ messages in thread
From: Julien Grall @ 2020-12-15 13:19 UTC (permalink / raw)
To: Jan Beulich
Cc: bertrand.marquis, Rahul.Singh, Julien Grall, Andrew Cooper,
George Dunlap, Ian Jackson, Stefano Stabellini, Wei Liu,
xen-devel
Hi,
On 15/12/2020 11:46, Jan Beulich wrote:
> On 15.12.2020 12:26, Julien Grall wrote:
>> --- a/xen/include/xen/lib.h
>> +++ b/xen/include/xen/lib.h
>> @@ -23,7 +23,13 @@
>> #include <asm/bug.h>
>>
>> #define BUG_ON(p) do { if (unlikely(p)) BUG(); } while (0)
>> -#define WARN_ON(p) do { if (unlikely(p)) WARN(); } while (0)
>> +#define WARN_ON(p) ({ \
>> + bool __ret_warn_on = (p); \
>
> Please can you avoid leading underscores here?
I can.
>
>> + \
>> + if ( unlikely(__ret_warn_on) ) \
>> + WARN(); \
>> + unlikely(__ret_warn_on); \
>> +})
>
> Is this latter unlikely() having any effect? So far I thought it
> would need to be immediately inside a control construct or be an
> operand to && or ||.
The unlikely() is directly taken from the Linux implementation.
My guess is the compiler is still able to use the information for the
branch prediction in the case of:
if ( WARN_ON(...) )
Cheers,
> Jan
>
--
Julien Grall
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] xen: Rework WARN_ON() to return whether a warning was triggered
2020-12-15 13:19 ` Julien Grall
@ 2020-12-15 14:01 ` Jan Beulich
2020-12-17 23:54 ` Stefano Stabellini
0 siblings, 1 reply; 13+ messages in thread
From: Jan Beulich @ 2020-12-15 14:01 UTC (permalink / raw)
To: Julien Grall
Cc: bertrand.marquis, Rahul.Singh, Julien Grall, Andrew Cooper,
George Dunlap, Ian Jackson, Stefano Stabellini, Wei Liu,
xen-devel
On 15.12.2020 14:19, Julien Grall wrote:
> On 15/12/2020 11:46, Jan Beulich wrote:
>> On 15.12.2020 12:26, Julien Grall wrote:
>>> --- a/xen/include/xen/lib.h
>>> +++ b/xen/include/xen/lib.h
>>> @@ -23,7 +23,13 @@
>>> #include <asm/bug.h>
>>>
>>> #define BUG_ON(p) do { if (unlikely(p)) BUG(); } while (0)
>>> -#define WARN_ON(p) do { if (unlikely(p)) WARN(); } while (0)
>>> +#define WARN_ON(p) ({ \
>>> + bool __ret_warn_on = (p); \
>>
>> Please can you avoid leading underscores here?
>
> I can.
>
>>
>>> + \
>>> + if ( unlikely(__ret_warn_on) ) \
>>> + WARN(); \
>>> + unlikely(__ret_warn_on); \
>>> +})
>>
>> Is this latter unlikely() having any effect? So far I thought it
>> would need to be immediately inside a control construct or be an
>> operand to && or ||.
>
> The unlikely() is directly taken from the Linux implementation.
>
> My guess is the compiler is still able to use the information for the
> branch prediction in the case of:
>
> if ( WARN_ON(...) )
Maybe. Or maybe not. I don't suppose the Linux commit introducing
it clarifies this?
Jan
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] xen: Rework WARN_ON() to return whether a warning was triggered
2020-12-15 13:11 ` Julien Grall
@ 2020-12-15 16:20 ` Jürgen Groß
2020-12-17 17:58 ` Bertrand Marquis
1 sibling, 0 replies; 13+ messages in thread
From: Jürgen Groß @ 2020-12-15 16:20 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: bertrand.marquis, Rahul.Singh, Julien Grall, Andrew Cooper,
George Dunlap, Ian Jackson, Jan Beulich, Stefano Stabellini,
Wei Liu
[-- Attachment #1.1.1: Type: text/plain, Size: 1262 bytes --]
On 15.12.20 14:11, Julien Grall wrote:
> Hi Juergen,
>
> On 15/12/2020 11:31, Jürgen Groß wrote:
>> On 15.12.20 12:26, Julien Grall wrote:
>>> From: Julien Grall <jgrall@amazon.com>
>>>
>>> So far, our implementation of WARN_ON() cannot be used in the following
>>> situation:
>>>
>>> if ( WARN_ON() )
>>> ...
>>>
>>> This is because the WARN_ON() doesn't return whether a warning. Such
>>
>> ... warning has been triggered.
>
> I will add it.
>
>>
>>> construction can be handy to have if you have to print more information
>>> and now the stack track.
>>
>> Sorry, I'm not able to parse that sentence.
>
> Urgh :/. How about the following commit message:
>
> "So far, our implementation of WARN_ON() cannot be used in the following
> situation:
>
> if ( WARN_ON() )
> ...
>
> This is because WARN_ON() doesn't return whether a warning has been
> triggered. Such construciton can be handy if you want to print more
> information and also dump the stack trace.
>
> Therefore, rework the WARN_ON() implementation to return whether a
> warning was triggered. The idea was borrowed from Linux".
Better :-)
With that you can add my:
Reviewed-by: Juergen Gross <jgross@suse.com>
Juergen
[-- Attachment #1.1.2: OpenPGP_0xB0DE9DD628BF132F.asc --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] xen: Rework WARN_ON() to return whether a warning was triggered
2020-12-15 13:11 ` Julien Grall
2020-12-15 16:20 ` Jürgen Groß
@ 2020-12-17 17:58 ` Bertrand Marquis
1 sibling, 0 replies; 13+ messages in thread
From: Bertrand Marquis @ 2020-12-17 17:58 UTC (permalink / raw)
To: Julien Grall
Cc: Jürgen Groß,
xen-devel, Rahul Singh, Julien Grall, Andrew Cooper,
George Dunlap, Ian Jackson, Jan Beulich, Stefano Stabellini,
Wei Liu
Hi Julien,
> On 15 Dec 2020, at 13:11, Julien Grall <julien@xen.org> wrote:
>
> Hi Juergen,
>
> On 15/12/2020 11:31, Jürgen Groß wrote:
>> On 15.12.20 12:26, Julien Grall wrote:
>>> From: Julien Grall <jgrall@amazon.com>
>>>
>>> So far, our implementation of WARN_ON() cannot be used in the following
>>> situation:
>>>
>>> if ( WARN_ON() )
>>> ...
>>>
>>> This is because the WARN_ON() doesn't return whether a warning. Such
>> ... warning has been triggered.
>
> I will add it.
>
>>> construction can be handy to have if you have to print more information
>>> and now the stack track.
>> Sorry, I'm not able to parse that sentence.
>
> Urgh :/. How about the following commit message:
>
> "So far, our implementation of WARN_ON() cannot be used in the following situation:
>
> if ( WARN_ON() )
> ...
>
> This is because WARN_ON() doesn't return whether a warning has been triggered. Such construciton can be handy if you want to print more information and also dump the stack trace.
>
> Therefore, rework the WARN_ON() implementation to return whether a warning was triggered. The idea was borrowed from Linux".
With that.
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
And thanks a lot for this :-)
Cheers
Bertrand
>
> Cheers,
>
> --
> Julien Grall
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] xen: Rework WARN_ON() to return whether a warning was triggered
2020-12-15 14:01 ` Jan Beulich
@ 2020-12-17 23:54 ` Stefano Stabellini
2020-12-18 0:29 ` Julien Grall
2020-12-18 7:54 ` Jan Beulich
0 siblings, 2 replies; 13+ messages in thread
From: Stefano Stabellini @ 2020-12-17 23:54 UTC (permalink / raw)
To: Jan Beulich
Cc: Julien Grall, bertrand.marquis, Rahul.Singh, Julien Grall,
Andrew Cooper, George Dunlap, Ian Jackson, Stefano Stabellini,
Wei Liu, xen-devel
On Tue, 15 Dec 2020, Jan Beulich wrote:
> On 15.12.2020 14:19, Julien Grall wrote:
> > On 15/12/2020 11:46, Jan Beulich wrote:
> >> On 15.12.2020 12:26, Julien Grall wrote:
> >>> --- a/xen/include/xen/lib.h
> >>> +++ b/xen/include/xen/lib.h
> >>> @@ -23,7 +23,13 @@
> >>> #include <asm/bug.h>
> >>>
> >>> #define BUG_ON(p) do { if (unlikely(p)) BUG(); } while (0)
> >>> -#define WARN_ON(p) do { if (unlikely(p)) WARN(); } while (0)
> >>> +#define WARN_ON(p) ({ \
> >>> + bool __ret_warn_on = (p); \
> >>
> >> Please can you avoid leading underscores here?
> >
> > I can.
> >
> >>
> >>> + \
> >>> + if ( unlikely(__ret_warn_on) ) \
> >>> + WARN(); \
> >>> + unlikely(__ret_warn_on); \
> >>> +})
> >>
> >> Is this latter unlikely() having any effect? So far I thought it
> >> would need to be immediately inside a control construct or be an
> >> operand to && or ||.
> >
> > The unlikely() is directly taken from the Linux implementation.
> >
> > My guess is the compiler is still able to use the information for the
> > branch prediction in the case of:
> >
> > if ( WARN_ON(...) )
>
> Maybe. Or maybe not. I don't suppose the Linux commit introducing
> it clarifies this?
I did a bit of digging but it looks like the unlikely has been there
forever. I'd just keep it as is.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] xen: Rework WARN_ON() to return whether a warning was triggered
2020-12-17 23:54 ` Stefano Stabellini
@ 2020-12-18 0:29 ` Julien Grall
2020-12-18 7:54 ` Jan Beulich
1 sibling, 0 replies; 13+ messages in thread
From: Julien Grall @ 2020-12-18 0:29 UTC (permalink / raw)
To: Stefano Stabellini
Cc: Jan Beulich, Bertrand Marquis, Rahul Singh, Julien Grall,
Andrew Cooper, George Dunlap, Ian Jackson, Wei Liu, xen-devel
On Thu, 17 Dec 2020 at 23:54, Stefano Stabellini <sstabellini@kernel.org> wrote:
>
> On Tue, 15 Dec 2020, Jan Beulich wrote:
> > On 15.12.2020 14:19, Julien Grall wrote:
> > > On 15/12/2020 11:46, Jan Beulich wrote:
> > >> On 15.12.2020 12:26, Julien Grall wrote:
> > >>> --- a/xen/include/xen/lib.h
> > >>> +++ b/xen/include/xen/lib.h
> > >>> @@ -23,7 +23,13 @@
> > >>> #include <asm/bug.h>
> > >>>
> > >>> #define BUG_ON(p) do { if (unlikely(p)) BUG(); } while (0)
> > >>> -#define WARN_ON(p) do { if (unlikely(p)) WARN(); } while (0)
> > >>> +#define WARN_ON(p) ({ \
> > >>> + bool __ret_warn_on = (p); \
> > >>
> > >> Please can you avoid leading underscores here?
> > >
> > > I can.
> > >
> > >>
> > >>> + \
> > >>> + if ( unlikely(__ret_warn_on) ) \
> > >>> + WARN(); \
> > >>> + unlikely(__ret_warn_on); \
> > >>> +})
> > >>
> > >> Is this latter unlikely() having any effect? So far I thought it
> > >> would need to be immediately inside a control construct or be an
> > >> operand to && or ||.
> > >
> > > The unlikely() is directly taken from the Linux implementation.
> > >
> > > My guess is the compiler is still able to use the information for the
> > > branch prediction in the case of:
> > >
> > > if ( WARN_ON(...) )
> >
> > Maybe. Or maybe not. I don't suppose the Linux commit introducing
> > it clarifies this?
>
> I did a bit of digging but it looks like the unlikely has been there
> forever. I'd just keep it as is.
Thanks! I was planning to answer earlier on with some data but got
preempted with some higher priority work.
The Linux commit message is not very helpful. I will do some testing
so I can convince Jan that compilers can be clever and make use of it.
Cheers,
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] xen: Rework WARN_ON() to return whether a warning was triggered
2020-12-17 23:54 ` Stefano Stabellini
2020-12-18 0:29 ` Julien Grall
@ 2020-12-18 7:54 ` Jan Beulich
2020-12-18 8:19 ` Jürgen Groß
1 sibling, 1 reply; 13+ messages in thread
From: Jan Beulich @ 2020-12-18 7:54 UTC (permalink / raw)
To: Stefano Stabellini
Cc: Julien Grall, bertrand.marquis, Rahul.Singh, Julien Grall,
Andrew Cooper, George Dunlap, Ian Jackson, Wei Liu, xen-devel
On 18.12.2020 00:54, Stefano Stabellini wrote:
> On Tue, 15 Dec 2020, Jan Beulich wrote:
>> On 15.12.2020 14:19, Julien Grall wrote:
>>> On 15/12/2020 11:46, Jan Beulich wrote:
>>>> On 15.12.2020 12:26, Julien Grall wrote:
>>>>> --- a/xen/include/xen/lib.h
>>>>> +++ b/xen/include/xen/lib.h
>>>>> @@ -23,7 +23,13 @@
>>>>> #include <asm/bug.h>
>>>>>
>>>>> #define BUG_ON(p) do { if (unlikely(p)) BUG(); } while (0)
>>>>> -#define WARN_ON(p) do { if (unlikely(p)) WARN(); } while (0)
>>>>> +#define WARN_ON(p) ({ \
>>>>> + bool __ret_warn_on = (p); \
>>>>
>>>> Please can you avoid leading underscores here?
>>>
>>> I can.
>>>
>>>>
>>>>> + \
>>>>> + if ( unlikely(__ret_warn_on) ) \
>>>>> + WARN(); \
>>>>> + unlikely(__ret_warn_on); \
>>>>> +})
>>>>
>>>> Is this latter unlikely() having any effect? So far I thought it
>>>> would need to be immediately inside a control construct or be an
>>>> operand to && or ||.
>>>
>>> The unlikely() is directly taken from the Linux implementation.
>>>
>>> My guess is the compiler is still able to use the information for the
>>> branch prediction in the case of:
>>>
>>> if ( WARN_ON(...) )
>>
>> Maybe. Or maybe not. I don't suppose the Linux commit introducing
>> it clarifies this?
>
> I did a bit of digging but it looks like the unlikely has been there
> forever. I'd just keep it as is.
I'm afraid I don't view this as a reason to inherit code unchanged.
If it was introduced with a clear indication that compilers can
recognize it despite the somewhat unusual placement, then fine. But
likely() / unlikely() quite often get put in more or less blindly -
see the not uncommon unlikely(a && b) style of uses, which don't
typically have the intended effect and would instead need to be
unlikely(a) && unlikely(b) [assuming each condition alone is indeed
deemed unlikely], unless compilers have learned to guess/infer
what's meant between when I last looked at this and now.
Jan
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] xen: Rework WARN_ON() to return whether a warning was triggered
2020-12-18 7:54 ` Jan Beulich
@ 2020-12-18 8:19 ` Jürgen Groß
2020-12-18 8:31 ` Jan Beulich
0 siblings, 1 reply; 13+ messages in thread
From: Jürgen Groß @ 2020-12-18 8:19 UTC (permalink / raw)
To: Jan Beulich, Stefano Stabellini
Cc: Julien Grall, bertrand.marquis, Rahul.Singh, Julien Grall,
Andrew Cooper, George Dunlap, Ian Jackson, Wei Liu, xen-devel
[-- Attachment #1.1.1: Type: text/plain, Size: 3001 bytes --]
On 18.12.20 08:54, Jan Beulich wrote:
> On 18.12.2020 00:54, Stefano Stabellini wrote:
>> On Tue, 15 Dec 2020, Jan Beulich wrote:
>>> On 15.12.2020 14:19, Julien Grall wrote:
>>>> On 15/12/2020 11:46, Jan Beulich wrote:
>>>>> On 15.12.2020 12:26, Julien Grall wrote:
>>>>>> --- a/xen/include/xen/lib.h
>>>>>> +++ b/xen/include/xen/lib.h
>>>>>> @@ -23,7 +23,13 @@
>>>>>> #include <asm/bug.h>
>>>>>>
>>>>>> #define BUG_ON(p) do { if (unlikely(p)) BUG(); } while (0)
>>>>>> -#define WARN_ON(p) do { if (unlikely(p)) WARN(); } while (0)
>>>>>> +#define WARN_ON(p) ({ \
>>>>>> + bool __ret_warn_on = (p); \
>>>>>
>>>>> Please can you avoid leading underscores here?
>>>>
>>>> I can.
>>>>
>>>>>
>>>>>> + \
>>>>>> + if ( unlikely(__ret_warn_on) ) \
>>>>>> + WARN(); \
>>>>>> + unlikely(__ret_warn_on); \
>>>>>> +})
>>>>>
>>>>> Is this latter unlikely() having any effect? So far I thought it
>>>>> would need to be immediately inside a control construct or be an
>>>>> operand to && or ||.
>>>>
>>>> The unlikely() is directly taken from the Linux implementation.
>>>>
>>>> My guess is the compiler is still able to use the information for the
>>>> branch prediction in the case of:
>>>>
>>>> if ( WARN_ON(...) )
>>>
>>> Maybe. Or maybe not. I don't suppose the Linux commit introducing
>>> it clarifies this?
>>
>> I did a bit of digging but it looks like the unlikely has been there
>> forever. I'd just keep it as is.
>
> I'm afraid I don't view this as a reason to inherit code unchanged.
> If it was introduced with a clear indication that compilers can
> recognize it despite the somewhat unusual placement, then fine. But
> likely() / unlikely() quite often get put in more or less blindly -
> see the not uncommon unlikely(a && b) style of uses, which don't
> typically have the intended effect and would instead need to be
> unlikely(a) && unlikely(b) [assuming each condition alone is indeed
> deemed unlikely], unless compilers have learned to guess/infer
> what's meant between when I last looked at this and now.
I have made a little experiment and found that the unlikely() at the
end of a macro is having effect.
The disassembly of
#define unlikely(x) __builtin_expect(!!(x), 0)
#define foo() ({ \
int i = !time(NULL); \
unlikely(i); })
#include "stdio.h"
#include "time.h"
int main() {
if (foo())
puts("a");
return 0;
}
is the same as that of:
#define unlikely(x) __builtin_expect(!!(x), 0)
#include "stdio.h"
#include "time.h"
int main() {
int i = !time(NULL);
if (unlikely(i))
puts("a");
return 0;
}
while that of:
#include "stdio.h"
#include "time.h"
int main() {
int i = !time(NULL);
if (i)
puts("a");
return 0;
}
is different.
Juergen
[-- Attachment #1.1.2: OpenPGP_0xB0DE9DD628BF132F.asc --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] xen: Rework WARN_ON() to return whether a warning was triggered
2020-12-18 8:19 ` Jürgen Groß
@ 2020-12-18 8:31 ` Jan Beulich
0 siblings, 0 replies; 13+ messages in thread
From: Jan Beulich @ 2020-12-18 8:31 UTC (permalink / raw)
To: Jürgen Groß, Stefano Stabellini
Cc: Julien Grall, bertrand.marquis, Rahul.Singh, Julien Grall,
Andrew Cooper, George Dunlap, Ian Jackson, Wei Liu, xen-devel
On 18.12.2020 09:19, Jürgen Groß wrote:
> On 18.12.20 08:54, Jan Beulich wrote:
>> On 18.12.2020 00:54, Stefano Stabellini wrote:
>>> On Tue, 15 Dec 2020, Jan Beulich wrote:
>>>> On 15.12.2020 14:19, Julien Grall wrote:
>>>>> On 15/12/2020 11:46, Jan Beulich wrote:
>>>>>> On 15.12.2020 12:26, Julien Grall wrote:
>>>>>>> --- a/xen/include/xen/lib.h
>>>>>>> +++ b/xen/include/xen/lib.h
>>>>>>> @@ -23,7 +23,13 @@
>>>>>>> #include <asm/bug.h>
>>>>>>>
>>>>>>> #define BUG_ON(p) do { if (unlikely(p)) BUG(); } while (0)
>>>>>>> -#define WARN_ON(p) do { if (unlikely(p)) WARN(); } while (0)
>>>>>>> +#define WARN_ON(p) ({ \
>>>>>>> + bool __ret_warn_on = (p); \
>>>>>>
>>>>>> Please can you avoid leading underscores here?
>>>>>
>>>>> I can.
>>>>>
>>>>>>
>>>>>>> + \
>>>>>>> + if ( unlikely(__ret_warn_on) ) \
>>>>>>> + WARN(); \
>>>>>>> + unlikely(__ret_warn_on); \
>>>>>>> +})
>>>>>>
>>>>>> Is this latter unlikely() having any effect? So far I thought it
>>>>>> would need to be immediately inside a control construct or be an
>>>>>> operand to && or ||.
>>>>>
>>>>> The unlikely() is directly taken from the Linux implementation.
>>>>>
>>>>> My guess is the compiler is still able to use the information for the
>>>>> branch prediction in the case of:
>>>>>
>>>>> if ( WARN_ON(...) )
>>>>
>>>> Maybe. Or maybe not. I don't suppose the Linux commit introducing
>>>> it clarifies this?
>>>
>>> I did a bit of digging but it looks like the unlikely has been there
>>> forever. I'd just keep it as is.
>>
>> I'm afraid I don't view this as a reason to inherit code unchanged.
>> If it was introduced with a clear indication that compilers can
>> recognize it despite the somewhat unusual placement, then fine. But
>> likely() / unlikely() quite often get put in more or less blindly -
>> see the not uncommon unlikely(a && b) style of uses, which don't
>> typically have the intended effect and would instead need to be
>> unlikely(a) && unlikely(b) [assuming each condition alone is indeed
>> deemed unlikely], unless compilers have learned to guess/infer
>> what's meant between when I last looked at this and now.
>
> I have made a little experiment and found that the unlikely() at the
> end of a macro is having effect.
Okay, thanks - then my concern vanishes.
Jan
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2020-12-18 8:31 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-15 11:26 [PATCH] xen: Rework WARN_ON() to return whether a warning was triggered Julien Grall
2020-12-15 11:31 ` Jürgen Groß
2020-12-15 13:11 ` Julien Grall
2020-12-15 16:20 ` Jürgen Groß
2020-12-17 17:58 ` Bertrand Marquis
2020-12-15 11:46 ` Jan Beulich
2020-12-15 13:19 ` Julien Grall
2020-12-15 14:01 ` Jan Beulich
2020-12-17 23:54 ` Stefano Stabellini
2020-12-18 0:29 ` Julien Grall
2020-12-18 7:54 ` Jan Beulich
2020-12-18 8:19 ` Jürgen Groß
2020-12-18 8:31 ` Jan Beulich
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).