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