Re: [Xen-devel] [PATCHv1 3/4] spinlock: shrink struct lock_debug

[Jennifer Herbert]

On 18/12/15 16:58, Jan Beulich wrote:

On 18.12.15 at 15:09,  wrote:

--- a/xen/common/spinlock.c
+++ b/xen/common/spinlock.c
@@ -16,7 +16,7 @@ static atomic_t spin_debug __read_mostly = ATOMIC_INIT(0);
  
  static void check_lock(struct lock_debug *debug)

  {
-int irq_safe = !local_irq_is_enabled();
+s16 irq_safe = !local_irq_is_enabled();
  
  if ( unlikely(atomic_read(_debug) <= 0) )

  return;

I can't figure out why this odd looking change is needed.

Jan




This patch shrinks struct lock_debug's irq_safe member.  Since you end 
up with many instances of this embedded in other structures such as 
struct domain, this become a useful saving in memory.  At one point, I 
had an issue with struct domain exceeding 4k, which caused trouble.
Given the change to struct_lock debug,  this local variable irq_safe, is 
compared with it, and used in cmpxchg with it it, and therefore should 
match type.


- Jenny

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCHv1 3/4] spinlock: shrink struct lock_debug

[Jan Beulich]

>>> On 19.01.16 at 13:31,  wrote:
> On 18/12/15 16:58, Jan Beulich wrote:
> On 18.12.15 at 15:09,  wrote:
>>> --- a/xen/common/spinlock.c
>>> +++ b/xen/common/spinlock.c
>>> @@ -16,7 +16,7 @@ static atomic_t spin_debug __read_mostly = ATOMIC_INIT(0);
>>>   
>>>   static void check_lock(struct lock_debug *debug)
>>>   {
>>> -int irq_safe = !local_irq_is_enabled();
>>> +s16 irq_safe = !local_irq_is_enabled();
>>>   
>>>   if ( unlikely(atomic_read(_debug) <= 0) )
>>>   return;
>> I can't figure out why this odd looking change is needed.
> 
> This patch shrinks struct lock_debug's irq_safe member.  Since you end 
> up with many instances of this embedded in other structures such as 
> struct domain, this become a useful saving in memory.  At one point, I 
> had an issue with struct domain exceeding 4k, which caused trouble.
> Given the change to struct_lock debug,  this local variable irq_safe, is 
> compared with it, and used in cmpxchg with it it, and therefore should 
> match type.

I don't think the type of the local variable matters for the cmpxchg();
note how the literal -1 is also not of type s16. Also if what you say
was to be consistent, the type of "seen" would need to be changed
too. Afaict all that can happen from the proposed type change is
worse generated code (which doesn't matter much as it's debug-only
code, but anyway).

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCHv1 3/4] spinlock: shrink struct lock_debug

[David Vrabel]

From: Jennifer Herbert 

Reduce the size of struct lock_debug so increases in other lock
structures don't increase the size of struct domain too much.

Signed-off-by: Jennifer Herbert 
Signed-off-by: David Vrabel 
---
 xen/common/spinlock.c  | 2 +-
 xen/include/xen/spinlock.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/common/spinlock.c b/xen/common/spinlock.c
index 7f89694..c129a88 100644
--- a/xen/common/spinlock.c
+++ b/xen/common/spinlock.c
@@ -16,7 +16,7 @@ static atomic_t spin_debug __read_mostly = ATOMIC_INIT(0);
 
 static void check_lock(struct lock_debug *debug)
 {
-int irq_safe = !local_irq_is_enabled();
+s16 irq_safe = !local_irq_is_enabled();
 
 if ( unlikely(atomic_read(_debug) <= 0) )
 return;
diff --git a/xen/include/xen/spinlock.h b/xen/include/xen/spinlock.h
index fb0438e..5e54407 100644
--- a/xen/include/xen/spinlock.h
+++ b/xen/include/xen/spinlock.h
@@ -6,7 +6,7 @@
 
 #ifndef NDEBUG
 struct lock_debug {
-int irq_safe; /* +1: IRQ-safe; 0: not IRQ-safe; -1: don't know yet */
+s16 irq_safe; /* +1: IRQ-safe; 0: not IRQ-safe; -1: don't know yet */
 };
 #define _LOCK_DEBUG { -1 }
 void spin_debug_enable(void);
-- 
2.1.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCHv1 3/4] spinlock: shrink struct lock_debug

[Jan Beulich]

>>> On 18.12.15 at 15:09,  wrote:

> --- a/xen/common/spinlock.c
> +++ b/xen/common/spinlock.c
> @@ -16,7 +16,7 @@ static atomic_t spin_debug __read_mostly = ATOMIC_INIT(0);
>  
>  static void check_lock(struct lock_debug *debug)
>  {
> -int irq_safe = !local_irq_is_enabled();
> +s16 irq_safe = !local_irq_is_enabled();
>  
>  if ( unlikely(atomic_read(_debug) <= 0) )
>  return;

I can't figure out why this odd looking change is needed. 

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel