[Xen-devel,v2,1/6] Port WARN_ON_ONCE() from Linux

Message ID 20180524004620.23828-2-sameer.goel@linaro.org
State New
Headers show
Series
  • SMMUv3 driver
Related show

Commit Message

Sameer Goel May 24, 2018, 12:46 a.m.
Port WARN_ON_ONCE macro from Linux.

Signed-off-by: Sameer Goel <sameer.goel@linaro.org>
Acked-by: Julien Grall <julien.grall@linaro.org>
---
 xen/arch/arm/xen.lds.S |  1 +
 xen/arch/x86/xen.lds.S |  1 +
 xen/include/xen/lib.h  | 13 +++++++++++++
 3 files changed, 15 insertions(+)

Comments

Jan Beulich May 24, 2018, 7:53 a.m. | #1
>>> On 24.05.18 at 02:46, <sameer.goel@linaro.org> wrote:
> Port WARN_ON_ONCE macro from Linux.

In such a case you should justify adjustments you've made:

> --- a/xen/include/xen/lib.h
> +++ b/xen/include/xen/lib.h
> @@ -11,6 +11,19 @@
>  #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_ONCE(p)                               \
> +({                                                    \
> +    static bool __section(".data.unlikely") warned;   \

Linux uses .data.once. That or .data.cold would seem better to me than
.data.unlikely.

Jan
Sameer Goel May 24, 2018, 8:23 p.m. | #2
On 05/24/2018 01:53 AM, Jan Beulich wrote:
>>>> On 24.05.18 at 02:46, <sameer.goel@linaro.org> wrote:
>> Port WARN_ON_ONCE macro from Linux.
> In such a case you should justify adjustments you've made:
I can add more details, but have mostly just changed variable names. The 
macro is self explanatory.

Should I just change this to: "Define WARN_ON_ONCE macro to mirror LInux 
functionality"
>
>> --- a/xen/include/xen/lib.h
>> +++ b/xen/include/xen/lib.h
>> @@ -11,6 +11,19 @@
>>   #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_ONCE(p)                               \
>> +({                                                    \
>> +    static bool __section(".data.unlikely") warned;   \
> Linux uses .data.once. That or .data.cold would seem better to me than
> .data.unlikely.
I guess there is not reason to keep this in a specific section. I'll 
just go ahead and remove the section here?

>
> Jan
>
>
Jan Beulich May 25, 2018, 7:07 a.m. | #3
>>> On 24.05.18 at 22:23, <sameer.goel@linaro.org> wrote:

> 
> On 05/24/2018 01:53 AM, Jan Beulich wrote:
>>>>> On 24.05.18 at 02:46, <sameer.goel@linaro.org> wrote:
>>> Port WARN_ON_ONCE macro from Linux.
>> In such a case you should justify adjustments you've made:
> I can add more details, but have mostly just changed variable names. The 
> macro is self explanatory.
> 
> Should I just change this to: "Define WARN_ON_ONCE macro to mirror LInux 
> functionality"

That would seem better to me.

>>> --- a/xen/include/xen/lib.h
>>> +++ b/xen/include/xen/lib.h
>>> @@ -11,6 +11,19 @@
>>>   #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_ONCE(p)                               \
>>> +({                                                    \
>>> +    static bool __section(".data.unlikely") warned;   \
>> Linux uses .data.once. That or .data.cold would seem better to me than
>> .data.unlikely.
> I guess there is not reason to keep this in a specific section. I'll 
> just go ahead and remove the section here?

There certainly is a reason: We don't want such variables to sit in the
middle of an otherwise frequently accessed cache line. Hence the "cold"
part of the suggested alternatives name.

Jan
Sameer Goel June 7, 2018, 1:21 a.m. | #4
On 05/25/2018 01:07 AM, Jan Beulich wrote:
>>>> On 24.05.18 at 22:23, <sameer.goel@linaro.org> wrote:
>>>> --- a/xen/include/xen/lib.h
>>>> +++ b/xen/include/xen/lib.h
>>>> @@ -11,6 +11,19 @@
>>>>    #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_ONCE(p)                               \
>>>> +({                                                    \
>>>> +    static bool __section(".data.unlikely") warned;   \
>>> Linux uses .data.once. That or .data.cold would seem better to me than
>>> .data.unlikely.
>> I guess there is not reason to keep this in a specific section. I'll
>> just go ahead and remove the section here?
> There certainly is a reason: We don't want such variables to sit in the
> middle of an otherwise frequently accessed cache line. Hence the "cold"
> part of the suggested alternatives name.
Till the last release Linux was using .data.unlikely to just keep this 
var in its own data section. I can change the name to cold. In Linux 
this was changed to once to enable a sys node that can reset the value 
of this section and for Xen this is not needed.

Thanks,
Sameer
> Jan
>
>

Patch

diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
index 245a0e0e85..2f211be22f 100644
--- a/xen/arch/arm/xen.lds.S
+++ b/xen/arch/arm/xen.lds.S
@@ -94,6 +94,7 @@  SECTIONS
        __end_schedulers_array = .;
        *(.data.rel)
        *(.data.rel.*)
+       *(.data.unlikely)
        CONSTRUCTORS
   } :text
 
diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index 70afedd31d..11b2a93eb1 100644
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -270,6 +270,7 @@  SECTIONS
        *(.data)
        *(.data.rel)
        *(.data.rel.*)
+       *(.data.unlikely)
        CONSTRUCTORS
   } :text
 
diff --git a/xen/include/xen/lib.h b/xen/include/xen/lib.h
index 1d9771340c..b8442a292e 100644
--- a/xen/include/xen/lib.h
+++ b/xen/include/xen/lib.h
@@ -11,6 +11,19 @@ 
 #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_ONCE(p)                               \
+({                                                    \
+    static bool __section(".data.unlikely") warned;   \
+    bool ret_warn_once = !!(p);                       \
+                                                      \
+    if ( unlikely(ret_warn_once && !warned) )         \
+    {                                                 \
+        warned = true;                                \
+        WARN();                                       \
+    }                                                 \
+    unlikely(ret_warn_once);                          \
+})
+
 #if __GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 6)
 /* Force a compilation error if condition is true */
 #define BUILD_BUG_ON(cond) ({ _Static_assert(!(cond), "!(" #cond ")"); })