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