| Message ID | 20180209031055.21926-2-sameer.goel@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | SMMUv3 driver | expand |
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.
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,
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.
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.
>>> 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
>>> 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
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
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 ")"); })