Received-SPF: pass (google.com: domain of linux-kernel+bounces-170041-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <7628453e-c418-49bb-be0a-50bc4248b7b9@redhat.com>
Date: Mon, 6 May 2024 11:05:32 -0400
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2] locking/qspinlock: Save previous node & owner CPU into
 mcs_spinlock
To: Peter Zijlstra <peterz@infradead.org>
Cc: Ingo Molnar <mingo@redhat.com>, Will Deacon <will@kernel.org>,
 Boqun Feng <boqun.feng@gmail.com>, linux-kernel@vger.kernel.org,
 Vernon Lovejoy <vlovejoy@redhat.com>
References: <20240504024106.654319-1-longman@redhat.com>
 <20240506103637.GM40213@noisy.programming.kicks-ass.net>
Content-Language: en-US
From: Waiman Long <longman@redhat.com>
In-Reply-To: <20240506103637.GM40213@noisy.programming.kicks-ass.net>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

On 5/6/24 06:36, Peter Zijlstra wrote:
> On Fri, May 03, 2024 at 10:41:06PM -0400, Waiman Long wrote:
>
> Not much of a fan of this thing, but I suppose it won't hurt too much ...
I purposely doesn't touch the fast path and add code only to slow path 
to make sure there shouldn't be any noticeable performance impact.
>> diff --git a/kernel/locking/mcs_spinlock.h b/kernel/locking/mcs_spinlock.h
>> index 85251d8771d9..cbe6f07dff2e 100644
>> --- a/kernel/locking/mcs_spinlock.h
>> +++ b/kernel/locking/mcs_spinlock.h
>> @@ -13,12 +13,19 @@
>>   #ifndef __LINUX_MCS_SPINLOCK_H
>>   #define __LINUX_MCS_SPINLOCK_H
>>   
>> +/*
>> + * Save an encoded version of the current MCS lock owner CPU to the
>> + * mcs_spinlock structure of the next lock owner.
>> + */
>> +#define MCS_LOCKED	(smp_processor_id() + 1)
>> +
>>   #include <asm/mcs_spinlock.h>
>>   
>>   struct mcs_spinlock {
>>   	struct mcs_spinlock *next;
>> -	int locked; /* 1 if lock acquired */
>> -	int count;  /* nesting count, see qspinlock.c */
>> +	int locked;	 /* non-zero if lock acquired */
>> +	short count;	 /* nesting count, see qspinlock.c */
>> +	short prev_node; /* encoded previous node value */
> Strictly speaking, count shouldn't ever go much higher than 4, so I
> suppose a byte should be sufficient. That would then leave 24bit for the
> prev thing, but you'll get to use bitfields or some other horrible
> thing.
Using bit field will be more expensive. So I am deferring that until the 
time it becomes necessary.
>>   };
>>   
>>   #ifndef arch_mcs_spin_lock_contended
>> @@ -42,7 +49,7 @@ do {									\
>>    * unlocking.
>>    */
>>   #define arch_mcs_spin_unlock_contended(l)				\
>> -	smp_store_release((l), 1)
>> +	smp_store_release((l), MCS_LOCKED)
> This leaves ARM up a creek I suppose... Also, there's but a single
> MCS_LOCKED user.

I am aware that ARM has its ownarch_mcs_spin_unlock_contended() macro 
defined. That is why I define the MCS_LOCKED macro before including 
asm/mcs_spinlock.h. I am planning to send an arm patch to change 1 to 
MCS_LOCKED if this patch is merged to close the gap. None other arches 
have arch_mcs_spin_unlock_contended() macro defined.

Cheers, Longman