Re: [Xen-devel] [PATCH 1/7] Port WARN_ON_ONCE() from Linux

2018-05-18 Thread Sameer Goel 


On 2/13/2018 2:46 AM, Jan Beulich wrote:
 On 09.02.18 at 11:47,  wrote:
>> On Fri, Feb 09, 2018 at 10:45:25AM +, 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


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 1/7] Port WARN_ON_ONCE() from Linux

2018-02-13 Thread Jan Beulich 
>>> On 09.02.18 at 04:10,  wrote:
> Port WARN_ON_ONCE macro from Linux.
> 
> Signed-off-by: Sameer Goel 
> Acked-by: Julien Grall 
> ---
>  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


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 1/7] Port WARN_ON_ONCE() from Linux

2018-02-13 Thread Jan Beulich 
>>> On 09.02.18 at 11:47,  wrote:
> On Fri, Feb 09, 2018 at 10:45:25AM +, 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

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 1/7] Port WARN_ON_ONCE() from Linux

2018-02-12 Thread Wei Liu 
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.

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 1/7] Port WARN_ON_ONCE() from Linux

2018-02-09 Thread Roger Pau Monné 
On Fri, Feb 09, 2018 at 10:45:25AM +, 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.

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 1/7] Port WARN_ON_ONCE() from Linux

2018-02-09 Thread Julien Grall 

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,

--
Julien Grall

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 1/7] Port WARN_ON_ONCE() from Linux

2018-02-09 Thread Roger Pau Monné 
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.

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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

2018-02-08 Thread Sameer Goel 
Port WARN_ON_ONCE macro from Linux.

Signed-off-by: Sameer Goel 
Acked-by: Julien Grall 
---
 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);\
+\
+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 ")"); })
-- 
2.14.1


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel