[2/3] xen: drop preempt_count() for non-debug builds
diff mbox series

Message ID 20190522094549.28397-3-jgross@suse.com
State New, archived
Headers show
Series
  • tune preempt_[dis|en]able()
Related show

Commit Message

Jürgen Groß May 22, 2019, 9:45 a.m. UTC
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(-)

Comments

Andrew Cooper May 22, 2019, 10 a.m. UTC | #1
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.
Jan Beulich May 22, 2019, 10:12 a.m. UTC | #2
>>> 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
Jürgen Groß May 22, 2019, 10:17 a.m. UTC | #3
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
Jürgen Groß May 22, 2019, 10:17 a.m. UTC | #4
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
Jan Beulich May 22, 2019, 10:18 a.m. UTC | #5
>>> 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
Jürgen Groß May 22, 2019, 10:20 a.m. UTC | #6
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
Andrew Cooper May 22, 2019, 10:39 a.m. UTC | #7
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

Patch
diff mbox series

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