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

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

Commit Message

Sameer Goel Feb. 9, 2018, 3:10 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

Roger Pau Monné Feb. 9, 2018, 10:29 a.m. | #1
On Thu, Feb 08, 2018 at 08:10:49PM -0700, Sameer Goel wrote:
> diff --git a/xen/include/xen/lib.h b/xen/include/xen/lib.h
> index 1d9771340c..697212a061 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; \
> +    int __ret_warn_once = !!(p);                    \
       ^ bool

> +                                                    \
> +    if ( unlikely(__ret_warn_once && !__warned) )     \
> +    {                                               \
> +        __warned = true;                            \
> +        WARN();                                     \
> +    }                                               \
> +    unlikely(__ret_warn_once);                      \

Does this macro really need to return something? It seems weird to me
to allow usages like: if ( WARN_ON_ONCE...

Nit: could you please align the '\'?

Thanks, Roger.
Julien Grall Feb. 9, 2018, 10:45 a.m. | #2
Hi,

On 02/09/2018 10:29 AM, Roger Pau Monné wrote:
> On Thu, Feb 08, 2018 at 08:10:49PM -0700, Sameer Goel wrote:
>> diff --git a/xen/include/xen/lib.h b/xen/include/xen/lib.h
>> index 1d9771340c..697212a061 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; \
>> +    int __ret_warn_once = !!(p);                    \
>         ^ bool
> 
>> +                                                    \
>> +    if ( unlikely(__ret_warn_once && !__warned) )     \
>> +    {                                               \
>> +        __warned = true;                            \
>> +        WARN();                                     \
>> +    }                                               \
>> +    unlikely(__ret_warn_once);                      \
> 
> Does this macro really need to return something? It seems weird to me
> to allow usages like: if ( WARN_ON_ONCE...

This construct is used in Linux (included in the driver ported):

if (WARN_ON_ONCE(fwspec->iommu_priv)) {
      master = fwspec->iommu_priv;
      smmu = master->smmu;
} else {
....
}

IHMO the makes the code nicer to read over:

WARN_ON_ONCE(...)
if ( ... ) {
} else {
}

Cheers,
Roger Pau Monné Feb. 9, 2018, 10:47 a.m. | #3
On Fri, Feb 09, 2018 at 10:45:25AM +0000, Julien Grall wrote:
> Hi,
> 
> On 02/09/2018 10:29 AM, Roger Pau Monné wrote:
> > On Thu, Feb 08, 2018 at 08:10:49PM -0700, Sameer Goel wrote:
> > > diff --git a/xen/include/xen/lib.h b/xen/include/xen/lib.h
> > > index 1d9771340c..697212a061 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; \
> > > +    int __ret_warn_once = !!(p);                    \
> >         ^ bool
> > 
> > > +                                                    \
> > > +    if ( unlikely(__ret_warn_once && !__warned) )     \
> > > +    {                                               \
> > > +        __warned = true;                            \
> > > +        WARN();                                     \
> > > +    }                                               \
> > > +    unlikely(__ret_warn_once);                      \
> > 
> > Does this macro really need to return something? It seems weird to me
> > to allow usages like: if ( WARN_ON_ONCE...
> 
> This construct is used in Linux (included in the driver ported):
> 
> if (WARN_ON_ONCE(fwspec->iommu_priv)) {
>      master = fwspec->iommu_priv;
>      smmu = master->smmu;
> } else {
> ....
> }
> 
> IHMO the makes the code nicer to read over:

OK, if that's intended I'm fine with it, just wanted to check.

Thanks, Roger.
Wei Liu Feb. 12, 2018, 2:49 p.m. | #4
On Thu, Feb 08, 2018 at 08:10:49PM -0700, Sameer Goel wrote:
>  
> +#define WARN_ON_ONCE(p)                             \
> +({                                                  \
> +    static bool __section(".data.unlikely") __warned; \
> +    int __ret_warn_once = !!(p);                    \
> +                                                    \
> +    if ( unlikely(__ret_warn_once && !__warned) )     \
> +    {                                               \
> +        __warned = true;                            \

Please don't mix bool and int type.

Wei.
Jan Beulich Feb. 13, 2018, 9:46 a.m. | #5
>>> On 09.02.18 at 11:47, <roger.pau@citrix.com> wrote:
> On Fri, Feb 09, 2018 at 10:45:25AM +0000, Julien Grall wrote:
>> Hi,
>> 
>> On 02/09/2018 10:29 AM, Roger Pau Monné wrote:
>> > On Thu, Feb 08, 2018 at 08:10:49PM -0700, Sameer Goel wrote:
>> > > diff --git a/xen/include/xen/lib.h b/xen/include/xen/lib.h
>> > > index 1d9771340c..697212a061 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; \
>> > > +    int __ret_warn_once = !!(p);                    \
>> >         ^ bool
>> > 
>> > > +                                                    \
>> > > +    if ( unlikely(__ret_warn_once && !__warned) )     \
>> > > +    {                                               \
>> > > +        __warned = true;                            \
>> > > +        WARN();                                     \
>> > > +    }                                               \
>> > > +    unlikely(__ret_warn_once);                      \
>> > 
>> > Does this macro really need to return something? It seems weird to me
>> > to allow usages like: if ( WARN_ON_ONCE...
>> 
>> This construct is used in Linux (included in the driver ported):
>> 
>> if (WARN_ON_ONCE(fwspec->iommu_priv)) {
>>      master = fwspec->iommu_priv;
>>      smmu = master->smmu;
>> } else {
>> ....
>> }
>> 
>> IHMO the makes the code nicer to read over:
> 
> OK, if that's intended I'm fine with it, just wanted to check.

But WARN_ON() should then be given the same property, I think.

Jan
Jan Beulich Feb. 13, 2018, 9:48 a.m. | #6
>>> On 09.02.18 at 04:10, <sameer.goel@linaro.org> wrote:
> 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(+)
> 
> diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
> index b0390180b4..4dc7997cf0 100644
> --- a/xen/arch/arm/xen.lds.S
> +++ b/xen/arch/arm/xen.lds.S
> @@ -87,6 +87,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 095298048f..353ca148ca 100644
> --- a/xen/arch/x86/xen.lds.S
> +++ b/xen/arch/x86/xen.lds.S
> @@ -240,6 +240,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..697212a061 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; \
> +    int __ret_warn_once = !!(p);                    \

Please don't introduce new name space violations (read: no
leading underscores here; use trailing one if need be).

Jan
Sameer Goel May 18, 2018, 10:46 p.m. | #7
On 2/13/2018 2:46 AM, Jan Beulich wrote:
>>>> On 09.02.18 at 11:47, <roger.pau@citrix.com> wrote:
>> On Fri, Feb 09, 2018 at 10:45:25AM +0000, Julien Grall wrote:
>>> Hi,
>>>
>>> On 02/09/2018 10:29 AM, Roger Pau Monné wrote:
>>>> On Thu, Feb 08, 2018 at 08:10:49PM -0700, Sameer Goel wrote:
>>>>> diff --git a/xen/include/xen/lib.h b/xen/include/xen/lib.h
>>>>> index 1d9771340c..697212a061 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; \
>>>>> +    int __ret_warn_once = !!(p);                    \
>>>>         ^ bool
>>>>
>>>>> +                                                    \
>>>>> +    if ( unlikely(__ret_warn_once && !__warned) )     \
>>>>> +    {                                               \
>>>>> +        __warned = true;                            \
>>>>> +        WARN();                                     \
>>>>> +    }                                               \
>>>>> +    unlikely(__ret_warn_once);                      \
>>>> Does this macro really need to return something? It seems weird to me
>>>> to allow usages like: if ( WARN_ON_ONCE...
>>> This construct is used in Linux (included in the driver ported):
>>>
>>> if (WARN_ON_ONCE(fwspec->iommu_priv)) {
>>>      master = fwspec->iommu_priv;
>>>      smmu = master->smmu;
>>> } else {
>>> ....
>>> }
>>>
>>> IHMO the makes the code nicer to read over:
>> OK, if that's intended I'm fine with it, just wanted to check.
> But WARN_ON() should then be given the same property, I think.

Not changing any already defined macros in Xen.
>
> Jan
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel

Patch

diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
index b0390180b4..4dc7997cf0 100644
--- a/xen/arch/arm/xen.lds.S
+++ b/xen/arch/arm/xen.lds.S
@@ -87,6 +87,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 095298048f..353ca148ca 100644
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -240,6 +240,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..697212a061 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; \
+    int __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 ")"); })