Re: [PATCH 2/3] xen: drop preempt_count() for non-debug builds

Re: [PATCH 2/3] xen: drop preempt_count() for non-debug builds

[Juergen Gross]

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


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

Re: [Xen-devel] [PATCH 2/3] xen: drop preempt_count() for non-debug builds

[Juergen Gross]

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


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

Re: [PATCH 2/3] xen: drop preempt_count() for non-debug builds

[Andrew Cooper]

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

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

Re: [PATCH 2/3] xen: drop preempt_count() for non-debug builds

[Jan Beulich]

>>> 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



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

Re: [PATCH 2/3] xen: drop preempt_count() for non-debug builds

[Juergen Gross]

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


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

Re: [PATCH 2/3] xen: drop preempt_count() for non-debug builds

[Juergen Gross]

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

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

Re: [PATCH 2/3] xen: drop preempt_count() for non-debug builds

[Jan Beulich]

>>> 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



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

Re: [PATCH 2/3] xen: drop preempt_count() for non-debug builds

[Andrew Cooper]

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.

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

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

[Juergen Gross]

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(-)

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
 
-- 
2.16.4


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