diff mbox series

xen: Rework WARN_ON() to return whether a warning was triggered

Message ID 20201215112610.1986-1-julien@xen.org (mailing list archive)
State Superseded
Headers show
Series xen: Rework WARN_ON() to return whether a warning was triggered | expand

Commit Message

Julien Grall Dec. 15, 2020, 11:26 a.m. UTC
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(-)

Comments

Jürgen Groß Dec. 15, 2020, 11:31 a.m. UTC | #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
Jan Beulich Dec. 15, 2020, 11:46 a.m. UTC | #2
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
Julien Grall Dec. 15, 2020, 1:11 p.m. UTC | #3
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 Dec. 15, 2020, 1:19 p.m. UTC | #4
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
>
Jan Beulich Dec. 15, 2020, 2:01 p.m. UTC | #5
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
Jürgen Groß Dec. 15, 2020, 4:20 p.m. UTC | #6
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
Bertrand Marquis Dec. 17, 2020, 5:58 p.m. UTC | #7
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
Stefano Stabellini Dec. 17, 2020, 11:54 p.m. UTC | #8
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.
Julien Grall Dec. 18, 2020, 12:29 a.m. UTC | #9
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,
Jan Beulich Dec. 18, 2020, 7:54 a.m. UTC | #10
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
Jürgen Groß Dec. 18, 2020, 8:19 a.m. UTC | #11
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
Jan Beulich Dec. 18, 2020, 8:31 a.m. UTC | #12
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
diff mbox series

Patch

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__) || \