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

[Jan Beulich <jbeulich@xxxxxxxx>]

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