| Message ID | 20190522094549.28397-3-jgross@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | tune preempt_[dis|en]able() | expand |
On 22/05/2019 10:45, Juergen Gross wrote: > preempt_count() and the associated per-cpu variable __preempt_count > are tested in debug build only. So drop them for non-debug builds. > > Signed-off-by: Juergen Gross <jgross@suse.com> I'd be tempted to fold patches 2 and 3 together, because they are both the same change, and it would reduce the churn. Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>, ideally with the two folded into one.
>>> On 22.05.19 at 11:45, <jgross@suse.com> wrote: > @@ -26,9 +28,11 @@ DECLARE_PER_CPU(unsigned int, __preempt_count); > preempt_count()--; \ > } while (0) > > -#ifndef NDEBUG > void ASSERT_NOT_IN_ATOMIC(void); > + > #else > +#define preempt_disable() barrier(); > +#define preempt_enable() barrier(); Stray semicolons (could be dropped while committing if we really want to go this route). Jan
On 22/05/2019 12:00, Andrew Cooper wrote: > On 22/05/2019 10:45, Juergen Gross wrote: >> preempt_count() and the associated per-cpu variable __preempt_count >> are tested in debug build only. So drop them for non-debug builds. >> >> Signed-off-by: Juergen Gross <jgross@suse.com> > > I'd be tempted to fold patches 2 and 3 together, because they are both > the same change, and it would reduce the churn. Fine with me. > Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>, ideally with the > two folded into one. Thanks, Juergen
On 22/05/2019 12:12, Jan Beulich wrote: >>>> On 22.05.19 at 11:45, <jgross@suse.com> wrote: >> @@ -26,9 +28,11 @@ DECLARE_PER_CPU(unsigned int, __preempt_count); >> preempt_count()--; \ >> } while (0) >> >> -#ifndef NDEBUG >> void ASSERT_NOT_IN_ATOMIC(void); >> + >> #else >> +#define preempt_disable() barrier(); >> +#define preempt_enable() barrier(); > > Stray semicolons (could be dropped while committing if we really > want to go this route). Oh, right. Will correct in V2. Juergen
>>> On 22.05.19 at 12:00, <andrew.cooper3@citrix.com> wrote: > On 22/05/2019 10:45, Juergen Gross wrote: >> preempt_count() and the associated per-cpu variable __preempt_count >> are tested in debug build only. So drop them for non-debug builds. >> >> Signed-off-by: Juergen Gross <jgross@suse.com> > > I'd be tempted to fold patches 2 and 3 together, because they are both > the same change, and it would reduce the churn. > > Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>, ideally with the > two folded into one. I'm a little surprised by this: Wasn't it you who generally wanted what ASSERT() expands to (controlled by NDEBUG) be independent of CONFIG_DEBUG, at some point down the road? Aren't you even having ASSERT()s enabled in release builds of XenServer, or am I misremembering? If so patch 3 would move us in the wrong direction. Jan
On 22/05/2019 12:18, Jan Beulich wrote: >>>> On 22.05.19 at 12:00, <andrew.cooper3@citrix.com> wrote: >> On 22/05/2019 10:45, Juergen Gross wrote: >>> preempt_count() and the associated per-cpu variable __preempt_count >>> are tested in debug build only. So drop them for non-debug builds. >>> >>> Signed-off-by: Juergen Gross <jgross@suse.com> >> >> I'd be tempted to fold patches 2 and 3 together, because they are both >> the same change, and it would reduce the churn. >> >> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>, ideally with the >> two folded into one. > > I'm a little surprised by this: Wasn't it you who generally > wanted what ASSERT() expands to (controlled by NDEBUG) > be independent of CONFIG_DEBUG, at some point down > the road? Aren't you even having ASSERT()s enabled in > release builds of XenServer, or am I misremembering? If so > patch 3 would move us in the wrong direction. A possibility to solve that would be the addition of CONFIG_ASSERT defaulting to CONFIG_DEBUG. Juergen
On 22/05/2019 11:18, Jan Beulich wrote: >>>> On 22.05.19 at 12:00, <andrew.cooper3@citrix.com> wrote: >> On 22/05/2019 10:45, Juergen Gross wrote: >>> preempt_count() and the associated per-cpu variable __preempt_count >>> are tested in debug build only. So drop them for non-debug builds. >>> >>> Signed-off-by: Juergen Gross <jgross@suse.com> >> I'd be tempted to fold patches 2 and 3 together, because they are both >> the same change, and it would reduce the churn. >> >> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>, ideally with the >> two folded into one. > I'm a little surprised by this: Wasn't it you who generally > wanted what ASSERT() expands to (controlled by NDEBUG) > be independent of CONFIG_DEBUG, at some point down > the road? In some ideal world yes, but what is rather more important is actually having optimisation control unrelated to NDEBUG. > Aren't you even having ASSERT()s enabled in > release builds of XenServer, or am I misremembering? If so > patch 3 would move us in the wrong direction. We build and ship a debug and a release hypervisor. ~Andrew
diff --git a/xen/common/preempt.c b/xen/common/preempt.c index 20913e20d3..3077c51d52 100644 --- a/xen/common/preempt.c +++ b/xen/common/preempt.c @@ -23,9 +23,9 @@ #include <xen/irq.h> #include <asm/system.h> +#ifndef NDEBUG DEFINE_PER_CPU(unsigned int, __preempt_count); -#ifndef NDEBUG void ASSERT_NOT_IN_ATOMIC(void) { ASSERT(!preempt_count()); diff --git a/xen/include/xen/preempt.h b/xen/include/xen/preempt.h index f715ca09bc..0bf49cc979 100644 --- a/xen/include/xen/preempt.h +++ b/xen/include/xen/preempt.h @@ -12,6 +12,8 @@ #include <xen/types.h> #include <xen/percpu.h> +#ifndef NDEBUG + DECLARE_PER_CPU(unsigned int, __preempt_count); #define preempt_count() (this_cpu(__preempt_count)) @@ -26,9 +28,11 @@ DECLARE_PER_CPU(unsigned int, __preempt_count); preempt_count()--; \ } while (0) -#ifndef NDEBUG void ASSERT_NOT_IN_ATOMIC(void); + #else +#define preempt_disable() barrier(); +#define preempt_enable() barrier(); #define ASSERT_NOT_IN_ATOMIC() ((void)0) #endif
preempt_count() and the associated per-cpu variable __preempt_count are tested in debug build only. So drop them for non-debug builds. Signed-off-by: Juergen Gross <jgross@suse.com> --- xen/common/preempt.c | 2 +- xen/include/xen/preempt.h | 6 +++++- 2 files changed, 6 insertions(+), 2 deletions(-)