Re: [PATCH] xen: Rework WARN_ON() to return whether a warning was triggered

From: "Jürgen Groß" <jgross@suse.com>
To: Jan Beulich <jbeulich@suse.com>,
	Stefano Stabellini <sstabellini@kernel.org>
Cc: Julien Grall <julien@xen.org>,
	bertrand.marquis@arm.com, Rahul.Singh@arm.com,
	Julien Grall <jgrall@amazon.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	George Dunlap <george.dunlap@citrix.com>,
	Ian Jackson <iwj@xenproject.org>, Wei Liu <wl@xen.org>,
	xen-devel@lists.xenproject.org
Subject: Re: [PATCH] xen: Rework WARN_ON() to return whether a warning was triggered
Date: Fri, 18 Dec 2020 09:19:38 +0100	[thread overview]
Message-ID: <142e7b4d-649d-07d0-26cf-185a434a365c@suse.com> (raw)
In-Reply-To: <81ea6132-b8b6-90b9-2c5c-9ca89ee6c0d0@suse.com>

[-- 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 --]