linux-kernel - Re: [PATCH v2] locking/qspinlock: Save previous node & owner CPU into mcs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <7628453e-c418-49bb-be0a-50bc4248b7b9@redhat.com>
Date: Mon, 6 May 2024 11:05:32 -0400
From: Waiman Long <longman@...hat.com>
To: Peter Zijlstra <peterz@...radead.org>
Cc: Ingo Molnar <mingo@...hat.com>, Will Deacon <will@...nel.org>,
 Boqun Feng <boqun.feng@...il.com>, linux-kernel@...r.kernel.org,
 Vernon Lovejoy <vlovejoy@...hat.com>
Subject: Re: [PATCH v2] locking/qspinlock: Save previous node & owner CPU into
 mcs_spinlock

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