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

From: "Jan Beulich" <JBeulich@suse.com>
To: Jennifer Herbert <Jennifer.Herbert@citrix.com>
Cc: xen-devel@lists.xenproject.org,
	David Vrabel <david.vrabel@citrix.com>,
	Ian Campbell <ian.campbell@citrix.com>
Subject: Re: [PATCHv1 3/4] spinlock: shrink struct lock_debug
Date: Tue, 19 Jan 2016 06:04:23 -0700	[thread overview]
Message-ID: <569E426702000078000C882F@prv-mh.provo.novell.com> (raw)
In-Reply-To: <569E2C9D.2040901@citrix.com>

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