Message ID | 1450447746-9305-4-git-send-email-david.vrabel@citrix.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
>>> 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
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
>>> 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 --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);