xen/include/xen/lib.h | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
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
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
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
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
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
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
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
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
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.
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,
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
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
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
© 2016 - 2024 Red Hat, Inc.