diff mbox

[PATCHv1,3/4] spinlock: shrink struct lock_debug

Message ID 1450447746-9305-4-git-send-email-david.vrabel@citrix.com (mailing list archive)
State New, archived
Headers show

Commit Message

David Vrabel Dec. 18, 2015, 2:09 p.m. UTC 
From: Jennifer Herbert <jennifer.herbert@citrix.com>

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 <jennifer.herbert@citrix.com>
Signed-off-by: David Vrabel <david.vrabel@citrix.com>
---
 xen/common/spinlock.c      | 2 +-
 xen/include/xen/spinlock.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

Comments

Jan Beulich Dec. 18, 2015, 4:58 p.m. UTC | #1 
>>> On 18.12.15 at 15:09, <david.vrabel@citrix.com> 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(&spin_debug) <= 0) )
>          return;

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

Jan
Jennifer Herbert Jan. 19, 2016, 12:31 p.m. UTC | #2 
On 18/12/15 16:58, Jan Beulich wrote:
>>>> On 18.12.15 at 15:09, <david.vrabel@citrix.com> 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(&spin_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
Jan Beulich Jan. 19, 2016, 1:04 p.m. UTC | #3 
>>> On 19.01.16 at 13:31, <Jennifer.Herbert@citrix.com> wrote:
> On 18/12/15 16:58, Jan Beulich wrote:
>>>>> On 18.12.15 at 15:09, <david.vrabel@citrix.com> 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(&spin_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
diff mbox

Patch

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(&spin_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);