Re: [PATCH v4 07/12] xen/spinlock: add explicit non-recursive locking functions

From: Juergen Gross <jgross@suse.com>
To: Julien Grall <julien@xen.org>, xen-devel@lists.xenproject.org
Cc: "Stefano Stabellini" <sstabellini@kernel.org>,
	"Bertrand Marquis" <bertrand.marquis@arm.com>,
	"Michal Orzel" <michal.orzel@amd.com>,
	"Volodymyr Babchuk" <Volodymyr_Babchuk@epam.com>,
	"Andrew Cooper" <andrew.cooper3@citrix.com>,
	"George Dunlap" <george.dunlap@citrix.com>,
	"Jan Beulich" <jbeulich@suse.com>, "Wei Liu" <wl@xen.org>,
	"Roger Pau Monné" <roger.pau@citrix.com>,
	"Tamas K Lengyel" <tamas@tklengyel.com>,
	"Lukasz Hawrylko" <lukasz@hawrylko.pl>,
	"Daniel P. Smith" <dpsmith@apertussolutions.com>,
	"Mateusz Mówka" <mateusz.mowka@intel.com>
Subject: Re: [PATCH v4 07/12] xen/spinlock: add explicit non-recursive locking functions
Date: Wed, 13 Dec 2023 07:17:17 +0100	[thread overview]
Message-ID: <f619efa1-da81-446b-968b-f81a2d18a7e5@suse.com> (raw)
In-Reply-To: <c7a969af-274b-4e92-b083-56059cbc98d6@xen.org>

[-- Attachment #1.1.1: Type: text/plain, Size: 2485 bytes --]

On 12.12.23 19:49, Julien Grall wrote:
> Hi Juergen,
> 
> On 12/12/2023 09:47, Juergen Gross wrote:
>> -#define spin_lock_init_prof(s, l) __spin_lock_init_prof(s, l, spinlock_t)
>> -#define rspin_lock_init_prof(s, l) __spin_lock_init_prof(s, l, rspinlock_t)
>> +#define spin_lock_init_prof(s, l)                                             \
>> +    __spin_lock_init_prof(s, l, lock, spinlock_t, 0)
>> +#define rspin_lock_init_prof(s, l)                                            \
>> +    __spin_lock_init_prof(s, l, rlock, rspinlock_t, 1)
>>   void _lock_profile_register_struct(
>>       int32_t type, struct lock_profile_qhead *qhead, int32_t idx);
>> @@ -174,6 +179,7 @@ struct lock_profile_qhead { };
>>   #endif
>> +
> 
> Spurious change?

Indeed, will remove it.

> 
>>   typedef union {
>>       uint32_t head_tail;
>>       struct {
>> @@ -261,4 +267,12 @@ void rspin_unlock_irqrestore(rspinlock_t *lock, unsigned 
>> long flags);
>>   /* Ensure a lock is quiescent between two critical operations. */
>>   #define spin_barrier(l)               _spin_barrier(l)
>> +#define nrspin_trylock(l)    spin_trylock(l)
>> +#define nrspin_lock(l)       spin_lock(l)
>> +#define nrspin_unlock(l)     spin_unlock(l)
>> +#define nrspin_lock_irq(l)   spin_lock_irq(l)
>> +#define nrspin_unlock_irq(l) spin_unlock_irq(l)
>> +#define nrspin_lock_irqsave(l, f)      spin_lock_irqsave(l, f)
>> +#define nrspin_unlock_irqrestore(l, f) spin_unlock_irqrestore(l, f)
> 
> There is a comment describing [r]spinlock but not this new variant. Can you add 
> one?

Okay.

> That said, I know this is existing code, but I have to admit this is a bit 
> unclear why we are allowing an recursive spinlock to be non-recursive. To me it 
> sounds like we are making the typesafe not very safe because it doesn't 
> guarantee that we are not mixing the call nrspin_* with rspin_*.

This is the current API.

If you have locked with nrspin_*, any rspin_* attempt on the same lock will
spin until rspin_unlock (nrspin_* will not set recurse_cpu, but take the
lock).

If you have locked with rspin_*, any nrspin_* attempt on the same lock will
spin, too.

So I don't see any major problem regarding accidental misuse, which wouldn't
be visible by deadlocks (there is no silent misbehavior).

Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]