* [PATCH v3 01/13] xen/spinlock: fix coding style issues
2023-11-20 11:38 [PATCH v3 00/13] xen/spinlock: make recursive spinlocks a dedicated type Juergen Gross
@ 2023-11-20 11:38 ` Juergen Gross
2023-11-20 11:38 ` [PATCH v3 02/13] xen/spinlock: reduce lock profile ifdefs Juergen Gross
` (11 subsequent siblings)
12 siblings, 0 replies; 23+ messages in thread
From: Juergen Gross @ 2023-11-20 11:38 UTC (permalink / raw)
To: xen-devel
Cc: javi.merino, Juergen Gross, Andrew Cooper, George Dunlap,
Jan Beulich, Julien Grall, Stefano Stabellini, Wei Liu
Fix some coding style issues in spinlock coding:
- Instead of u64 etc. use the preferred uint64_t etc. types
- Remove some trailing spaces
- Add missing spaces in "if" statements
- Add some missing blank lines
While modifying the line, add const for the observed_head() parameter.
Signed-off-by: Juergen Gross <jgross@suse.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
---
V2:
- new patch
V3:
- add const for the observed_head() parameter (Jan Beulich)
---
xen/common/spinlock.c | 20 +++++++++++---------
xen/include/xen/spinlock.h | 16 ++++++++--------
2 files changed, 19 insertions(+), 17 deletions(-)
diff --git a/xen/common/spinlock.c b/xen/common/spinlock.c
index 7f453234a9..d7194e518c 100644
--- a/xen/common/spinlock.c
+++ b/xen/common/spinlock.c
@@ -100,7 +100,7 @@ void check_lock(union lock_debug *debug, bool try)
* every lock must be consistently observed else we can deadlock in
* IRQ-context rendezvous functions (a rendezvous which gets every CPU
* into IRQ context before any CPU is released from the rendezvous).
- *
+ *
* If we can mix IRQ-disabled and IRQ-enabled callers, the following can
* happen:
* * Lock is held by CPU A, with IRQs enabled
@@ -108,7 +108,7 @@ void check_lock(union lock_debug *debug, bool try)
* * Rendezvous starts -- CPU A takes interrupt and enters rendezbous spin
* * DEADLOCK -- CPU B will never enter rendezvous, CPU A will never exit
* the rendezvous, and will hence never release the lock.
- *
+ *
* To guard against this subtle bug we latch the IRQ safety of every
* spinlock in the system, on first use.
*
@@ -151,12 +151,12 @@ static void check_barrier(union lock_debug *debug)
/*
* For a barrier, we have a relaxed IRQ-safety-consistency check.
- *
+ *
* It is always safe to spin at the barrier with IRQs enabled -- that does
* not prevent us from entering an IRQ-context rendezvous, and nor are
* we preventing anyone else from doing so (since we do not actually
* acquire the lock during a barrier operation).
- *
+ *
* However, if we spin on an IRQ-unsafe lock with IRQs disabled then that
* is clearly wrong, for the same reason outlined in check_lock() above.
*/
@@ -262,7 +262,7 @@ void spin_debug_disable(void)
#ifdef CONFIG_DEBUG_LOCK_PROFILE
#define LOCK_PROFILE_REL \
- if (lock->profile) \
+ if ( lock->profile ) \
{ \
lock->profile->time_hold += NOW() - lock->profile->time_locked; \
lock->profile->lock_cnt++; \
@@ -270,10 +270,10 @@ void spin_debug_disable(void)
#define LOCK_PROFILE_VAR s_time_t block = 0
#define LOCK_PROFILE_BLOCK block = block ? : NOW();
#define LOCK_PROFILE_GOT \
- if (lock->profile) \
+ if ( lock->profile ) \
{ \
lock->profile->time_locked = NOW(); \
- if (block) \
+ if ( block ) \
{ \
lock->profile->time_block += lock->profile->time_locked - block; \
lock->profile->block_cnt++; \
@@ -298,7 +298,7 @@ static always_inline spinlock_tickets_t observe_lock(spinlock_tickets_t *t)
return v;
}
-static always_inline u16 observe_head(spinlock_tickets_t *t)
+static always_inline uint16_t observe_head(const spinlock_tickets_t *t)
{
smp_rmb();
return read_atomic(&t->head);
@@ -412,7 +412,7 @@ int _spin_trylock(spinlock_t *lock)
*/
got_lock(&lock->debug);
#ifdef CONFIG_DEBUG_LOCK_PROFILE
- if (lock->profile)
+ if ( lock->profile )
lock->profile->time_locked = NOW();
#endif
return 1;
@@ -616,6 +616,7 @@ int spinlock_profile_control(struct xen_sysctl_lockprof_op *pc)
case XEN_SYSCTL_LOCKPROF_reset:
spinlock_profile_reset('\0');
break;
+
case XEN_SYSCTL_LOCKPROF_query:
pc->nr_elem = 0;
par.rc = 0;
@@ -624,6 +625,7 @@ int spinlock_profile_control(struct xen_sysctl_lockprof_op *pc)
pc->time = NOW() - lock_profile_start;
rc = par.rc;
break;
+
default:
rc = -EINVAL;
break;
diff --git a/xen/include/xen/spinlock.h b/xen/include/xen/spinlock.h
index 16d933ae7e..02a062a5b0 100644
--- a/xen/include/xen/spinlock.h
+++ b/xen/include/xen/spinlock.h
@@ -81,8 +81,8 @@ struct lock_profile {
struct lock_profile *next; /* forward link */
const char *name; /* lock name */
struct spinlock *lock; /* the lock itself */
- u64 lock_cnt; /* # of complete locking ops */
- u64 block_cnt; /* # of complete wait for lock */
+ uint64_t lock_cnt; /* # of complete locking ops */
+ uint64_t block_cnt; /* # of complete wait for lock */
s_time_t time_hold; /* cumulated lock time */
s_time_t time_block; /* cumulated wait time */
s_time_t time_locked; /* system time of last locking */
@@ -122,7 +122,7 @@ struct lock_profile_qhead {
prof->lock = &(s)->l; \
prof->next = (s)->profile_head.elem_q; \
(s)->profile_head.elem_q = prof; \
- } while(0)
+ } while( 0 )
void _lock_profile_register_struct(
int32_t type, struct lock_profile_qhead *qhead, int32_t idx);
@@ -153,10 +153,10 @@ struct lock_profile_qhead { };
#endif
typedef union {
- u32 head_tail;
+ uint32_t head_tail;
struct {
- u16 head;
- u16 tail;
+ uint16_t head;
+ uint16_t tail;
};
} spinlock_tickets_t;
@@ -164,10 +164,10 @@ typedef union {
typedef struct spinlock {
spinlock_tickets_t tickets;
- u16 recurse_cpu:SPINLOCK_CPU_BITS;
+ uint16_t recurse_cpu:SPINLOCK_CPU_BITS;
#define SPINLOCK_NO_CPU ((1u << SPINLOCK_CPU_BITS) - 1)
#define SPINLOCK_RECURSE_BITS (16 - SPINLOCK_CPU_BITS)
- u16 recurse_cnt:SPINLOCK_RECURSE_BITS;
+ uint16_t recurse_cnt:SPINLOCK_RECURSE_BITS;
#define SPINLOCK_MAX_RECURSE ((1u << SPINLOCK_RECURSE_BITS) - 1)
union lock_debug debug;
#ifdef CONFIG_DEBUG_LOCK_PROFILE
--
2.35.3
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH v3 02/13] xen/spinlock: reduce lock profile ifdefs
2023-11-20 11:38 [PATCH v3 00/13] xen/spinlock: make recursive spinlocks a dedicated type Juergen Gross
2023-11-20 11:38 ` [PATCH v3 01/13] xen/spinlock: fix coding style issues Juergen Gross
@ 2023-11-20 11:38 ` Juergen Gross
2023-11-24 17:59 ` Alejandro Vallejo
2023-11-20 11:38 ` [PATCH v3 03/13] xen/spinlock: make spinlock initializers more readable Juergen Gross
` (10 subsequent siblings)
12 siblings, 1 reply; 23+ messages in thread
From: Juergen Gross @ 2023-11-20 11:38 UTC (permalink / raw)
To: xen-devel
Cc: javi.merino, Juergen Gross, Andrew Cooper, George Dunlap,
Jan Beulich, Julien Grall, Stefano Stabellini, Wei Liu
With some small adjustments to the LOCK_PROFILE_* macros some #ifdefs
can be dropped from spinlock.c.
Signed-off-by: Juergen Gross <jgross@suse.com>
---
V2:
- new patch
V3:
- add variable name to macros parameter (Jan Beulich)
---
xen/common/spinlock.c | 49 +++++++++++++++++++------------------------
1 file changed, 21 insertions(+), 28 deletions(-)
diff --git a/xen/common/spinlock.c b/xen/common/spinlock.c
index d7194e518c..ce18fbdd8a 100644
--- a/xen/common/spinlock.c
+++ b/xen/common/spinlock.c
@@ -267,25 +267,28 @@ void spin_debug_disable(void)
lock->profile->time_hold += NOW() - lock->profile->time_locked; \
lock->profile->lock_cnt++; \
}
-#define LOCK_PROFILE_VAR s_time_t block = 0
-#define LOCK_PROFILE_BLOCK block = block ? : NOW();
-#define LOCK_PROFILE_GOT \
+#define LOCK_PROFILE_VAR(var, val) s_time_t var = (val)
+#define LOCK_PROFILE_BLOCK(var ) var = var ? : NOW()
+#define LOCK_PROFILE_BLKACC(tst, val) \
+ if ( tst ) \
+ { \
+ lock->profile->time_block += lock->profile->time_locked - (val); \
+ lock->profile->block_cnt++; \
+ }
+#define LOCK_PROFILE_GOT(val) \
if ( lock->profile ) \
{ \
lock->profile->time_locked = NOW(); \
- if ( block ) \
- { \
- lock->profile->time_block += lock->profile->time_locked - block; \
- lock->profile->block_cnt++; \
- } \
+ LOCK_PROFILE_BLKACC(val, val); \
}
#else
#define LOCK_PROFILE_REL
-#define LOCK_PROFILE_VAR
-#define LOCK_PROFILE_BLOCK
-#define LOCK_PROFILE_GOT
+#define LOCK_PROFILE_VAR(var, val)
+#define LOCK_PROFILE_BLOCK(var)
+#define LOCK_PROFILE_BLKACC(tst, val)
+#define LOCK_PROFILE_GOT(val)
#endif
@@ -308,7 +311,7 @@ static void always_inline spin_lock_common(spinlock_t *lock,
void (*cb)(void *), void *data)
{
spinlock_tickets_t tickets = SPINLOCK_TICKET_INC;
- LOCK_PROFILE_VAR;
+ LOCK_PROFILE_VAR(block, 0);
check_lock(&lock->debug, false);
preempt_disable();
@@ -316,14 +319,14 @@ static void always_inline spin_lock_common(spinlock_t *lock,
tickets.head_tail);
while ( tickets.tail != observe_head(&lock->tickets) )
{
- LOCK_PROFILE_BLOCK;
+ LOCK_PROFILE_BLOCK(block);
if ( cb )
cb(data);
arch_lock_relax();
}
arch_lock_acquire_barrier();
got_lock(&lock->debug);
- LOCK_PROFILE_GOT;
+ LOCK_PROFILE_GOT(block);
}
void _spin_lock(spinlock_t *lock)
@@ -411,19 +414,15 @@ int _spin_trylock(spinlock_t *lock)
* arch_lock_acquire_barrier().
*/
got_lock(&lock->debug);
-#ifdef CONFIG_DEBUG_LOCK_PROFILE
- if ( lock->profile )
- lock->profile->time_locked = NOW();
-#endif
+ LOCK_PROFILE_GOT(0);
+
return 1;
}
void _spin_barrier(spinlock_t *lock)
{
spinlock_tickets_t sample;
-#ifdef CONFIG_DEBUG_LOCK_PROFILE
- s_time_t block = NOW();
-#endif
+ LOCK_PROFILE_VAR(block, NOW());
check_barrier(&lock->debug);
smp_mb();
@@ -432,13 +431,7 @@ void _spin_barrier(spinlock_t *lock)
{
while ( observe_head(&lock->tickets) == sample.head )
arch_lock_relax();
-#ifdef CONFIG_DEBUG_LOCK_PROFILE
- if ( lock->profile )
- {
- lock->profile->time_block += NOW() - block;
- lock->profile->block_cnt++;
- }
-#endif
+ LOCK_PROFILE_BLKACC(lock->profile, block);
}
smp_mb();
}
--
2.35.3
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH v3 02/13] xen/spinlock: reduce lock profile ifdefs
2023-11-20 11:38 ` [PATCH v3 02/13] xen/spinlock: reduce lock profile ifdefs Juergen Gross
@ 2023-11-24 17:59 ` Alejandro Vallejo
2023-11-24 18:12 ` Alejandro Vallejo
2023-11-28 13:07 ` Juergen Gross
0 siblings, 2 replies; 23+ messages in thread
From: Alejandro Vallejo @ 2023-11-24 17:59 UTC (permalink / raw)
To: Juergen Gross, xen-devel
Cc: javi.merino, Andrew Cooper, George Dunlap, Jan Beulich,
Julien Grall, Stefano Stabellini, Wei Liu
Hi,
On 20/11/2023 11:38, Juergen Gross wrote:> With some small adjustments to the LOCK_PROFILE_* macros some #ifdefs
> can be dropped from spinlock.c.
>
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
> V2:
> - new patch
> V3:
> - add variable name to macros parameter (Jan Beulich)
> ---
> xen/common/spinlock.c | 49 +++++++++++++++++++------------------------
> 1 file changed, 21 insertions(+), 28 deletions(-)
>
> diff --git a/xen/common/spinlock.c b/xen/common/spinlock.c
> index d7194e518c..ce18fbdd8a 100644
> --- a/xen/common/spinlock.c
> +++ b/xen/common/spinlock.c
> @@ -267,25 +267,28 @@ void spin_debug_disable(void)
> lock->profile->time_hold += NOW() - lock->profile->time_locked; \
> lock->profile->lock_cnt++; \
> }
> -#define LOCK_PROFILE_VAR s_time_t block = 0
> -#define LOCK_PROFILE_BLOCK block = block ? : NOW();
> -#define LOCK_PROFILE_GOT \
> +#define LOCK_PROFILE_VAR(var, val) s_time_t var = (val)
> +#define LOCK_PROFILE_BLOCK(var ) var = var ? : NOW()nit: spaces before the closing parenthesis
And that's as far as I can complain on your changes. As for things that were
already present...
> +#define LOCK_PROFILE_BLKACC(tst, val) \
> + if ( tst ) \
> + { \
> + lock->profile->time_block += lock->profile->time_locked - (val); \
> + lock->profile->block_cnt++; \
> + }> +#define LOCK_PROFILE_GOT(val) \
> if ( lock->profile ) \
> { \
> lock->profile->time_locked = NOW(); \
> - if ( block ) \
> - { \
> - lock->profile->time_block += lock->profile->time_locked - block; \
> - lock->profile->block_cnt++; \
> - } \
> + LOCK_PROFILE_BLKACC(val, val); \
> }... these 2 probably want `lock` to be the first argument so they don't rely on
the variable "lock" to be in context, and...
>
> #else
>
> #define LOCK_PROFILE_REL
> -#define LOCK_PROFILE_VAR
> -#define LOCK_PROFILE_BLOCK
> -#define LOCK_PROFILE_GOT
> +#define LOCK_PROFILE_VAR(var, val)
> +#define LOCK_PROFILE_BLOCK(var)
> +#define LOCK_PROFILE_BLKACC(tst, val)
> +#define LOCK_PROFILE_GOT(val)... I'd feel better if these where actually statements rather than fully empty. i.e:
(void)0, or something like that. Then they would behave well in conditionals without
braces.
>
> #endif
>
> @@ -308,7 +311,7 @@ static void always_inline spin_lock_common(spinlock_t *lock,
> void (*cb)(void *), void *data)
> {
> spinlock_tickets_t tickets = SPINLOCK_TICKET_INC;
> - LOCK_PROFILE_VAR;
> + LOCK_PROFILE_VAR(block, 0);
>
> check_lock(&lock->debug, false);
> preempt_disable();
> @@ -316,14 +319,14 @@ static void always_inline spin_lock_common(spinlock_t *lock,
> tickets.head_tail);
> while ( tickets.tail != observe_head(&lock->tickets) )
> {
> - LOCK_PROFILE_BLOCK;
> + LOCK_PROFILE_BLOCK(block);
> if ( cb )
> cb(data);
> arch_lock_relax();
> }
> arch_lock_acquire_barrier();
> got_lock(&lock->debug);
> - LOCK_PROFILE_GOT;
> + LOCK_PROFILE_GOT(block);
> }
>
> void _spin_lock(spinlock_t *lock)
> @@ -411,19 +414,15 @@ int _spin_trylock(spinlock_t *lock)
> * arch_lock_acquire_barrier().
> */
> got_lock(&lock->debug);
> -#ifdef CONFIG_DEBUG_LOCK_PROFILE
> - if ( lock->profile )
> - lock->profile->time_locked = NOW();
> -#endif
> + LOCK_PROFILE_GOT(0);
> +
> return 1;
> }
>
> void _spin_barrier(spinlock_t *lock)
> {
> spinlock_tickets_t sample;
> -#ifdef CONFIG_DEBUG_LOCK_PROFILE
> - s_time_t block = NOW();
> -#endif
> + LOCK_PROFILE_VAR(block, NOW());
>
> check_barrier(&lock->debug);
> smp_mb();
> @@ -432,13 +431,7 @@ void _spin_barrier(spinlock_t *lock)
> {
> while ( observe_head(&lock->tickets) == sample.head )
> arch_lock_relax();
> -#ifdef CONFIG_DEBUG_LOCK_PROFILE
> - if ( lock->profile )
> - {
> - lock->profile->time_block += NOW() - block;
> - lock->profile->block_cnt++;
> - }
> -#endif
> + LOCK_PROFILE_BLKACC(lock->profile, block);
> }
> smp_mb();
> }
Besides the first nit, the others were already present and
the usage of the macros is very localised, so take it or
leave it. It's fairly inconsequential. Otherwise LGTM.
With the 1st nit sorted
Reviewed-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
Cheers,
Alejandro
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH v3 02/13] xen/spinlock: reduce lock profile ifdefs
2023-11-24 17:59 ` Alejandro Vallejo
@ 2023-11-24 18:12 ` Alejandro Vallejo
2023-11-28 13:07 ` Juergen Gross
1 sibling, 0 replies; 23+ messages in thread
From: Alejandro Vallejo @ 2023-11-24 18:12 UTC (permalink / raw)
To: Juergen Gross, xen-devel
Cc: javi.merino, Andrew Cooper, George Dunlap, Jan Beulich,
Julien Grall, Stefano Stabellini, Wei Liu
On 24/11/2023 17:59, Alejandro Vallejo wrote:
> Hi,
>
> On 20/11/2023 11:38, Juergen Gross wrote:> With some small adjustments
> to the LOCK_PROFILE_* macros some #ifdefs
>> can be dropped from spinlock.c.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>> V2:
>> - new patch
>> V3:
>> - add variable name to macros parameter (Jan Beulich)
>> ---
>> xen/common/spinlock.c | 49 +++++++++++++++++++------------------------
>> 1 file changed, 21 insertions(+), 28 deletions(-)
>>
>> diff --git a/xen/common/spinlock.c b/xen/common/spinlock.c
>> index d7194e518c..ce18fbdd8a 100644
>> --- a/xen/common/spinlock.c
>> +++ b/xen/common/spinlock.c
>> @@ -267,25 +267,28 @@ void spin_debug_disable(void)
>> lock->profile->time_hold += NOW() -
>> lock->profile->time_locked; \
>>
>> lock->profile->lock_cnt++; \
>> }
>> -#define LOCK_PROFILE_VAR s_time_t block = 0
>> -#define LOCK_PROFILE_BLOCK block = block ? : NOW();
>> -#define
>> LOCK_PROFILE_GOT \
>> +#define LOCK_PROFILE_VAR(var, val) s_time_t var = (val)
>> +#define LOCK_PROFILE_BLOCK(var ) var = var ? : NOW()nit: spaces
>> before the closing parenthesis
Ugh, I'm changing email clients and formatting seems have gone haywire.
The first line of each comment is inlined with the quote they refer to.
Cheers,
Alejandro
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 02/13] xen/spinlock: reduce lock profile ifdefs
2023-11-24 17:59 ` Alejandro Vallejo
2023-11-24 18:12 ` Alejandro Vallejo
@ 2023-11-28 13:07 ` Juergen Gross
1 sibling, 0 replies; 23+ messages in thread
From: Juergen Gross @ 2023-11-28 13:07 UTC (permalink / raw)
To: Alejandro Vallejo, xen-devel
Cc: javi.merino, Andrew Cooper, George Dunlap, Jan Beulich,
Julien Grall, Stefano Stabellini, Wei Liu
[-- Attachment #1.1.1: Type: text/plain, Size: 1457 bytes --]
On 24.11.23 18:59, Alejandro Vallejo wrote:
> Hi,
>
> On 20/11/2023 11:38, Juergen Gross wrote:> With some small adjustments to the
> LOCK_PROFILE_* macros some #ifdefs
>> can be dropped from spinlock.c.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>> V2:
>> - new patch
>> V3:
>> - add variable name to macros parameter (Jan Beulich)
>> ---
>> xen/common/spinlock.c | 49 +++++++++++++++++++------------------------
>> 1 file changed, 21 insertions(+), 28 deletions(-)
>>
>> diff --git a/xen/common/spinlock.c b/xen/common/spinlock.c
>> index d7194e518c..ce18fbdd8a 100644
>> --- a/xen/common/spinlock.c
>> +++ b/xen/common/spinlock.c
>> @@ -267,25 +267,28 @@ void spin_debug_disable(void)
>> lock->profile->time_hold += NOW() - lock->profile->time_locked; \
>> lock->profile->lock_cnt++; \
>> }
>> -#define LOCK_PROFILE_VAR s_time_t block = 0
>> -#define LOCK_PROFILE_BLOCK block = block ? : NOW();
>> -#define LOCK_PROFILE_GOT \
>> +#define LOCK_PROFILE_VAR(var, val) s_time_t var = (val)
>> +#define LOCK_PROFILE_BLOCK(var ) var = var ? : NOW()
> nit: spaces before the closing parenthesis
Yeah, seen that later.
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 --]
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v3 03/13] xen/spinlock: make spinlock initializers more readable
2023-11-20 11:38 [PATCH v3 00/13] xen/spinlock: make recursive spinlocks a dedicated type Juergen Gross
2023-11-20 11:38 ` [PATCH v3 01/13] xen/spinlock: fix coding style issues Juergen Gross
2023-11-20 11:38 ` [PATCH v3 02/13] xen/spinlock: reduce lock profile ifdefs Juergen Gross
@ 2023-11-20 11:38 ` Juergen Gross
2023-11-20 11:38 ` [PATCH v3 04/13] xen/spinlock: introduce new type for recursive spinlocks Juergen Gross
` (9 subsequent siblings)
12 siblings, 0 replies; 23+ messages in thread
From: Juergen Gross @ 2023-11-20 11:38 UTC (permalink / raw)
To: xen-devel
Cc: javi.merino, Juergen Gross, Andrew Cooper, George Dunlap,
Jan Beulich, Julien Grall, Stefano Stabellini, Wei Liu
Use named member initializers instead of positional ones for the macros
used to initialize structures.
Signed-off-by: Juergen Gross <jgross@suse.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
---
V2:
- new patch
---
xen/include/xen/spinlock.h | 15 +++++++++++----
1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/xen/include/xen/spinlock.h b/xen/include/xen/spinlock.h
index 02a062a5b0..bbe1472571 100644
--- a/xen/include/xen/spinlock.h
+++ b/xen/include/xen/spinlock.h
@@ -21,7 +21,7 @@ union lock_debug {
bool unseen:1;
};
};
-#define _LOCK_DEBUG { LOCK_DEBUG_INITVAL }
+#define _LOCK_DEBUG { .val = LOCK_DEBUG_INITVAL }
void check_lock(union lock_debug *debug, bool try);
void lock_enter(const union lock_debug *debug);
void lock_exit(const union lock_debug *debug);
@@ -94,12 +94,16 @@ struct lock_profile_qhead {
int32_t idx; /* index for printout */
};
-#define _LOCK_PROFILE(name) { NULL, #name, &name, 0, 0, 0, 0, 0 }
+#define _LOCK_PROFILE(lockname) { .name = #lockname, .lock = &lockname, }
#define _LOCK_PROFILE_PTR(name) \
static struct lock_profile * const __lock_profile_##name \
__used_section(".lockprofile.data") = \
&__lock_profile_data_##name
-#define _SPIN_LOCK_UNLOCKED(x) { { 0 }, SPINLOCK_NO_CPU, 0, _LOCK_DEBUG, x }
+#define _SPIN_LOCK_UNLOCKED(x) { \
+ .recurse_cpu = SPINLOCK_NO_CPU, \
+ .debug =_LOCK_DEBUG, \
+ .profile = x, \
+}
#define SPIN_LOCK_UNLOCKED _SPIN_LOCK_UNLOCKED(NULL)
#define DEFINE_SPINLOCK(l) \
spinlock_t l = _SPIN_LOCK_UNLOCKED(NULL); \
@@ -142,7 +146,10 @@ extern void cf_check spinlock_profile_reset(unsigned char key);
struct lock_profile_qhead { };
-#define SPIN_LOCK_UNLOCKED { { 0 }, SPINLOCK_NO_CPU, 0, _LOCK_DEBUG }
+#define SPIN_LOCK_UNLOCKED { \
+ .recurse_cpu = SPINLOCK_NO_CPU, \
+ .debug =_LOCK_DEBUG, \
+}
#define DEFINE_SPINLOCK(l) spinlock_t l = SPIN_LOCK_UNLOCKED
#define spin_lock_init_prof(s, l) spin_lock_init(&((s)->l))
--
2.35.3
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH v3 04/13] xen/spinlock: introduce new type for recursive spinlocks
2023-11-20 11:38 [PATCH v3 00/13] xen/spinlock: make recursive spinlocks a dedicated type Juergen Gross
` (2 preceding siblings ...)
2023-11-20 11:38 ` [PATCH v3 03/13] xen/spinlock: make spinlock initializers more readable Juergen Gross
@ 2023-11-20 11:38 ` Juergen Gross
2023-11-24 18:59 ` Alejandro Vallejo
2023-11-20 11:38 ` [PATCH v3 05/13] xen/spinlock: rename recursive lock functions Juergen Gross
` (8 subsequent siblings)
12 siblings, 1 reply; 23+ messages in thread
From: Juergen Gross @ 2023-11-20 11:38 UTC (permalink / raw)
To: xen-devel
Cc: javi.merino, Juergen Gross, Jan Beulich, Andrew Cooper,
Roger Pau Monné, Wei Liu, George Dunlap, Julien Grall,
Stefano Stabellini, Paul Durrant
Introduce a new type "rspinlock_t" to be used for recursive spinlocks.
For now it is only an alias of spinlock_t, so both types can still be
used for recursive spinlocks. This will be changed later, though.
Switch all recursive spinlocks to the new type.
Define the initializer helpers and use them where appropriate.
Signed-off-by: Juergen Gross <jgross@suse.com>
---
V2:
- carved out from V1 patch
---
xen/arch/x86/include/asm/mm.h | 2 +-
xen/arch/x86/mm/mm-locks.h | 2 +-
xen/common/domain.c | 4 ++--
xen/common/ioreq.c | 2 +-
xen/drivers/char/console.c | 4 ++--
xen/drivers/passthrough/pci.c | 2 +-
xen/include/xen/sched.h | 6 +++---
xen/include/xen/spinlock.h | 19 +++++++++++++++----
8 files changed, 26 insertions(+), 15 deletions(-)
diff --git a/xen/arch/x86/include/asm/mm.h b/xen/arch/x86/include/asm/mm.h
index 05dfe35502..8a6e0c283f 100644
--- a/xen/arch/x86/include/asm/mm.h
+++ b/xen/arch/x86/include/asm/mm.h
@@ -596,7 +596,7 @@ unsigned long domain_get_maximum_gpfn(struct domain *d);
/* Definition of an mm lock: spinlock with extra fields for debugging */
typedef struct mm_lock {
- spinlock_t lock;
+ rspinlock_t lock;
int unlock_level;
int locker; /* processor which holds the lock */
const char *locker_function; /* func that took it */
diff --git a/xen/arch/x86/mm/mm-locks.h b/xen/arch/x86/mm/mm-locks.h
index 00b1bc402d..b05cad1752 100644
--- a/xen/arch/x86/mm/mm-locks.h
+++ b/xen/arch/x86/mm/mm-locks.h
@@ -20,7 +20,7 @@ DECLARE_PERCPU_RWLOCK_GLOBAL(p2m_percpu_rwlock);
static inline void mm_lock_init(mm_lock_t *l)
{
- spin_lock_init(&l->lock);
+ rspin_lock_init(&l->lock);
l->locker = -1;
l->locker_function = "nobody";
l->unlock_level = 0;
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 8f9ab01c0c..604f70ff5a 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -627,8 +627,8 @@ struct domain *domain_create(domid_t domid,
atomic_set(&d->refcnt, 1);
RCU_READ_LOCK_INIT(&d->rcu_lock);
- spin_lock_init_prof(d, domain_lock);
- spin_lock_init_prof(d, page_alloc_lock);
+ rspin_lock_init_prof(d, domain_lock);
+ rspin_lock_init_prof(d, page_alloc_lock);
spin_lock_init(&d->hypercall_deadlock_mutex);
INIT_PAGE_LIST_HEAD(&d->page_list);
INIT_PAGE_LIST_HEAD(&d->extra_page_list);
diff --git a/xen/common/ioreq.c b/xen/common/ioreq.c
index 62b907f4c4..652c18a9b5 100644
--- a/xen/common/ioreq.c
+++ b/xen/common/ioreq.c
@@ -1331,7 +1331,7 @@ unsigned int ioreq_broadcast(ioreq_t *p, bool buffered)
void ioreq_domain_init(struct domain *d)
{
- spin_lock_init(&d->ioreq_server.lock);
+ rspin_lock_init(&d->ioreq_server.lock);
arch_ioreq_domain_init(d);
}
diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
index 4824d4a91d..8b161488f6 100644
--- a/xen/drivers/char/console.c
+++ b/xen/drivers/char/console.c
@@ -120,7 +120,7 @@ static int __read_mostly sercon_handle = -1;
int8_t __read_mostly opt_console_xen; /* console=xen */
#endif
-static DEFINE_SPINLOCK(console_lock);
+static DEFINE_RSPINLOCK(console_lock);
/*
* To control the amount of printing, thresholds are added.
@@ -1178,7 +1178,7 @@ void console_force_unlock(void)
{
watchdog_disable();
spin_debug_disable();
- spin_lock_init(&console_lock);
+ rspin_lock_init(&console_lock);
serial_force_unlock(sercon_handle);
console_locks_busted = 1;
console_start_sync();
diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index 04d00c7c37..61be34e75f 100644
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -50,7 +50,7 @@ struct pci_seg {
} bus2bridge[MAX_BUSES];
};
-static spinlock_t _pcidevs_lock = SPIN_LOCK_UNLOCKED;
+static DEFINE_RSPINLOCK(_pcidevs_lock);
void pcidevs_lock(void)
{
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 3609ef88c4..c6604aef78 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -376,9 +376,9 @@ struct domain
rcu_read_lock_t rcu_lock;
- spinlock_t domain_lock;
+ rspinlock_t domain_lock;
- spinlock_t page_alloc_lock; /* protects all the following fields */
+ rspinlock_t page_alloc_lock; /* protects all the following fields */
struct page_list_head page_list; /* linked list */
struct page_list_head extra_page_list; /* linked list (size extra_pages) */
struct page_list_head xenpage_list; /* linked list (size xenheap_pages) */
@@ -597,7 +597,7 @@ struct domain
#ifdef CONFIG_IOREQ_SERVER
/* Lock protects all other values in the sub-struct */
struct {
- spinlock_t lock;
+ rspinlock_t lock;
struct ioreq_server *server[MAX_NR_IOREQ_SERVERS];
} ioreq_server;
#endif
diff --git a/xen/include/xen/spinlock.h b/xen/include/xen/spinlock.h
index bbe1472571..19561d5e61 100644
--- a/xen/include/xen/spinlock.h
+++ b/xen/include/xen/spinlock.h
@@ -45,7 +45,7 @@ union lock_debug { };
lock profiling on:
Global locks which should be subject to profiling must be declared via
- DEFINE_SPINLOCK.
+ DEFINE_[R]SPINLOCK.
For locks in structures further measures are necessary:
- the structure definition must include a profile_head with exactly this
@@ -56,7 +56,7 @@ union lock_debug { };
- the single locks which are subject to profiling have to be initialized
via
- spin_lock_init_prof(ptr, lock);
+ [r]spin_lock_init_prof(ptr, lock);
with ptr being the main structure pointer and lock the spinlock field
@@ -109,12 +109,16 @@ struct lock_profile_qhead {
spinlock_t l = _SPIN_LOCK_UNLOCKED(NULL); \
static struct lock_profile __lock_profile_data_##l = _LOCK_PROFILE(l); \
_LOCK_PROFILE_PTR(l)
+#define DEFINE_RSPINLOCK(l) \
+ rspinlock_t l = _SPIN_LOCK_UNLOCKED(NULL); \
+ static struct lock_profile __lock_profile_data_##l = _LOCK_PROFILE(l); \
+ _LOCK_PROFILE_PTR(l)
-#define spin_lock_init_prof(s, l) \
+#define __spin_lock_init_prof(s, l, locktype) \
do { \
struct lock_profile *prof; \
prof = xzalloc(struct lock_profile); \
- (s)->l = (spinlock_t)_SPIN_LOCK_UNLOCKED(prof); \
+ (s)->l = (locktype)_SPIN_LOCK_UNLOCKED(prof); \
if ( !prof ) \
{ \
printk(XENLOG_WARNING \
@@ -128,6 +132,9 @@ struct lock_profile_qhead {
(s)->profile_head.elem_q = prof; \
} while( 0 )
+#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)
+
void _lock_profile_register_struct(
int32_t type, struct lock_profile_qhead *qhead, int32_t idx);
void _lock_profile_deregister_struct(int32_t type,
@@ -151,8 +158,10 @@ struct lock_profile_qhead { };
.debug =_LOCK_DEBUG, \
}
#define DEFINE_SPINLOCK(l) spinlock_t l = SPIN_LOCK_UNLOCKED
+#define DEFINE_RSPINLOCK(l) rspinlock_t l = SPIN_LOCK_UNLOCKED
#define spin_lock_init_prof(s, l) spin_lock_init(&((s)->l))
+#define rspin_lock_init_prof(s, l) rspin_lock_init(&((s)->l))
#define lock_profile_register_struct(type, ptr, idx)
#define lock_profile_deregister_struct(type, ptr)
#define spinlock_profile_printall(key)
@@ -182,8 +191,10 @@ typedef struct spinlock {
#endif
} spinlock_t;
+typedef spinlock_t rspinlock_t;
#define spin_lock_init(l) (*(l) = (spinlock_t)SPIN_LOCK_UNLOCKED)
+#define rspin_lock_init(l) (*(l) = (rspinlock_t)SPIN_LOCK_UNLOCKED)
void _spin_lock(spinlock_t *lock);
void _spin_lock_cb(spinlock_t *lock, void (*cb)(void *), void *data);
--
2.35.3
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH v3 04/13] xen/spinlock: introduce new type for recursive spinlocks
2023-11-20 11:38 ` [PATCH v3 04/13] xen/spinlock: introduce new type for recursive spinlocks Juergen Gross
@ 2023-11-24 18:59 ` Alejandro Vallejo
2023-11-28 13:16 ` Juergen Gross
0 siblings, 1 reply; 23+ messages in thread
From: Alejandro Vallejo @ 2023-11-24 18:59 UTC (permalink / raw)
To: Juergen Gross, xen-devel
Cc: javi.merino, Jan Beulich, Andrew Cooper, Roger Pau Monné,
Wei Liu, George Dunlap, Julien Grall, Stefano Stabellini,
Paul Durrant
On 20/11/2023 11:38, Juergen Gross wrote:
> Introduce a new type "rspinlock_t" to be used for recursive spinlocks.
>
> For now it is only an alias of spinlock_t, so both types can still be
> used for recursive spinlocks. This will be changed later, though.
>
> Switch all recursive spinlocks to the new type.
>
> Define the initializer helpers and use them where appropriate.
>
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
> V2:
> - carved out from V1 patch
> ---
> xen/arch/x86/include/asm/mm.h | 2 +-
> xen/arch/x86/mm/mm-locks.h | 2 +-
> xen/common/domain.c | 4 ++--
> xen/common/ioreq.c | 2 +-
> xen/drivers/char/console.c | 4 ++--
> xen/drivers/passthrough/pci.c | 2 +-
> xen/include/xen/sched.h | 6 +++---
> xen/include/xen/spinlock.h | 19 +++++++++++++++----
> 8 files changed, 26 insertions(+), 15 deletions(-)
>
> diff --git a/xen/arch/x86/include/asm/mm.h b/xen/arch/x86/include/asm/mm.h
> index 05dfe35502..8a6e0c283f 100644
> --- a/xen/arch/x86/include/asm/mm.h
> +++ b/xen/arch/x86/include/asm/mm.h
> @@ -596,7 +596,7 @@ unsigned long domain_get_maximum_gpfn(struct domain *d);
>
> /* Definition of an mm lock: spinlock with extra fields for debugging */
> typedef struct mm_lock {
> - spinlock_t lock;
> + rspinlock_t lock;
Considering it's rspinlock_t rather than spinlock_recursive, do we want
to change also mm_lock_recursive() and paging_lock_recursive() into
mm_rlock() and paging_rlock() ?
> diff --git a/xen/include/xen/spinlock.h b/xen/include/xen/spinlock.h
> index bbe1472571..19561d5e61 100644
> --- a/xen/include/xen/spinlock.h
> +++ b/xen/include/xen/spinlock.h
> [snip]
> @@ -182,8 +191,10 @@ typedef struct spinlock {
> #endif
> } spinlock_t;
>
> +typedef spinlock_t rspinlock_t;
>
> #define spin_lock_init(l) (*(l) = (spinlock_t)SPIN_LOCK_UNLOCKED)
> +#define rspin_lock_init(l) (*(l) = (rspinlock_t)SPIN_LOCK_UNLOCKED)
nit: Both variants of [r]spin_lock_init(l) could be inline functions
rather than macros.
Cheers,
Alejandro
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH v3 04/13] xen/spinlock: introduce new type for recursive spinlocks
2023-11-24 18:59 ` Alejandro Vallejo
@ 2023-11-28 13:16 ` Juergen Gross
2023-11-28 13:36 ` Jan Beulich
0 siblings, 1 reply; 23+ messages in thread
From: Juergen Gross @ 2023-11-28 13:16 UTC (permalink / raw)
To: Alejandro Vallejo, xen-devel
Cc: javi.merino, Jan Beulich, Andrew Cooper, Roger Pau Monné,
Wei Liu, George Dunlap, Julien Grall, Stefano Stabellini,
Paul Durrant
[-- Attachment #1.1.1: Type: text/plain, Size: 2523 bytes --]
On 24.11.23 19:59, Alejandro Vallejo wrote:
> On 20/11/2023 11:38, Juergen Gross wrote:
>> Introduce a new type "rspinlock_t" to be used for recursive spinlocks.
>>
>> For now it is only an alias of spinlock_t, so both types can still be
>> used for recursive spinlocks. This will be changed later, though.
>>
>> Switch all recursive spinlocks to the new type.
>>
>> Define the initializer helpers and use them where appropriate.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>> V2:
>> - carved out from V1 patch
>> ---
>> xen/arch/x86/include/asm/mm.h | 2 +-
>> xen/arch/x86/mm/mm-locks.h | 2 +-
>> xen/common/domain.c | 4 ++--
>> xen/common/ioreq.c | 2 +-
>> xen/drivers/char/console.c | 4 ++--
>> xen/drivers/passthrough/pci.c | 2 +-
>> xen/include/xen/sched.h | 6 +++---
>> xen/include/xen/spinlock.h | 19 +++++++++++++++----
>> 8 files changed, 26 insertions(+), 15 deletions(-)
>>
>> diff --git a/xen/arch/x86/include/asm/mm.h b/xen/arch/x86/include/asm/mm.h
>> index 05dfe35502..8a6e0c283f 100644
>> --- a/xen/arch/x86/include/asm/mm.h
>> +++ b/xen/arch/x86/include/asm/mm.h
>> @@ -596,7 +596,7 @@ unsigned long domain_get_maximum_gpfn(struct domain *d);
>> /* Definition of an mm lock: spinlock with extra fields for debugging */
>> typedef struct mm_lock {
>> - spinlock_t lock;
>> + rspinlock_t lock;
>
> Considering it's rspinlock_t rather than spinlock_recursive, do we want
> to change also mm_lock_recursive() and paging_lock_recursive() into
> mm_rlock() and paging_rlock() ?
I don't think this should be part of this series.
>> diff --git a/xen/include/xen/spinlock.h b/xen/include/xen/spinlock.h
>> index bbe1472571..19561d5e61 100644
>> --- a/xen/include/xen/spinlock.h
>> +++ b/xen/include/xen/spinlock.h
> > [snip]
>> @@ -182,8 +191,10 @@ typedef struct spinlock {
>> #endif
>> } spinlock_t;
>> +typedef spinlock_t rspinlock_t;
>> #define spin_lock_init(l) (*(l) = (spinlock_t)SPIN_LOCK_UNLOCKED)
>> +#define rspin_lock_init(l) (*(l) = (rspinlock_t)SPIN_LOCK_UNLOCKED)
>
> nit: Both variants of [r]spin_lock_init(l) could be inline functions
> rather than macros.
I was following the spin_lock_init() example, and I guess that is following
the Linux kernel example.
I don't mind either way, but maybe other maintainers have a preference?
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 --]
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH v3 04/13] xen/spinlock: introduce new type for recursive spinlocks
2023-11-28 13:16 ` Juergen Gross
@ 2023-11-28 13:36 ` Jan Beulich
0 siblings, 0 replies; 23+ messages in thread
From: Jan Beulich @ 2023-11-28 13:36 UTC (permalink / raw)
To: Juergen Gross, Alejandro Vallejo
Cc: javi.merino, Andrew Cooper, Roger Pau Monné, Wei Liu,
George Dunlap, Julien Grall, Stefano Stabellini, Paul Durrant,
xen-devel
On 28.11.2023 14:16, Juergen Gross wrote:
> On 24.11.23 19:59, Alejandro Vallejo wrote:
>> On 20/11/2023 11:38, Juergen Gross wrote:
>>> --- a/xen/include/xen/spinlock.h
>>> +++ b/xen/include/xen/spinlock.h
>> > [snip]
>>> @@ -182,8 +191,10 @@ typedef struct spinlock {
>>> #endif
>>> } spinlock_t;
>>> +typedef spinlock_t rspinlock_t;
>>> #define spin_lock_init(l) (*(l) = (spinlock_t)SPIN_LOCK_UNLOCKED)
>>> +#define rspin_lock_init(l) (*(l) = (rspinlock_t)SPIN_LOCK_UNLOCKED)
>>
>> nit: Both variants of [r]spin_lock_init(l) could be inline functions
>> rather than macros.
>
> I was following the spin_lock_init() example, and I guess that is following
> the Linux kernel example.
>
> I don't mind either way, but maybe other maintainers have a preference?
While switching to inline functions would be nice, the new item should
imo be in line with the existing one (i.e. be a macro as long as that
other one also is a macro). And converting what has been there shouldn't
be a requirement for this series to land.
Jan
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v3 05/13] xen/spinlock: rename recursive lock functions
2023-11-20 11:38 [PATCH v3 00/13] xen/spinlock: make recursive spinlocks a dedicated type Juergen Gross
` (3 preceding siblings ...)
2023-11-20 11:38 ` [PATCH v3 04/13] xen/spinlock: introduce new type for recursive spinlocks Juergen Gross
@ 2023-11-20 11:38 ` Juergen Gross
2023-11-20 11:38 ` [PATCH v3 06/13] xen/spinlock: add rspin_[un]lock_irq[save|restore]() Juergen Gross
` (7 subsequent siblings)
12 siblings, 0 replies; 23+ messages in thread
From: Juergen Gross @ 2023-11-20 11:38 UTC (permalink / raw)
To: xen-devel
Cc: javi.merino, Juergen Gross, Stefano Stabellini, Julien Grall,
Bertrand Marquis, Michal Orzel, Volodymyr Babchuk, Andrew Cooper,
George Dunlap, Jan Beulich, Wei Liu, Roger Pau Monné,
Tamas K Lengyel, Paul Durrant
Rename the recursive spin_lock() functions by replacing the trailing
"_recursive" with a leading "r".
Switch the parameter to be a pointer to rspinlock_t.
Remove the indirection through a macro, as it is adding only complexity
without any gain.
Suggested-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: Juergen Gross <jgross@suse.com>
---
V2:
- new patch
---
xen/arch/arm/domain.c | 4 +--
xen/arch/x86/domain.c | 8 +++---
xen/arch/x86/mm/mem_sharing.c | 8 +++---
xen/arch/x86/mm/mm-locks.h | 4 +--
xen/common/ioreq.c | 52 +++++++++++++++++------------------
xen/common/page_alloc.c | 12 ++++----
xen/common/spinlock.c | 6 ++--
xen/drivers/char/console.c | 12 ++++----
xen/drivers/passthrough/pci.c | 4 +--
xen/include/xen/sched.h | 4 +--
xen/include/xen/spinlock.h | 24 +++++++---------
11 files changed, 67 insertions(+), 71 deletions(-)
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 28e3aaa5e4..603a5f7c81 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -996,7 +996,7 @@ static int relinquish_memory(struct domain *d, struct page_list_head *list)
int ret = 0;
/* Use a recursive lock, as we may enter 'free_domheap_page'. */
- spin_lock_recursive(&d->page_alloc_lock);
+ rspin_lock(&d->page_alloc_lock);
page_list_for_each_safe( page, tmp, list )
{
@@ -1023,7 +1023,7 @@ static int relinquish_memory(struct domain *d, struct page_list_head *list)
}
out:
- spin_unlock_recursive(&d->page_alloc_lock);
+ rspin_unlock(&d->page_alloc_lock);
return ret;
}
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 3712e36df9..69ce1fd5cf 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -1321,7 +1321,7 @@ int arch_set_info_guest(
{
bool done = false;
- spin_lock_recursive(&d->page_alloc_lock);
+ rspin_lock(&d->page_alloc_lock);
for ( i = 0; ; )
{
@@ -1342,7 +1342,7 @@ int arch_set_info_guest(
break;
}
- spin_unlock_recursive(&d->page_alloc_lock);
+ rspin_unlock(&d->page_alloc_lock);
if ( !done )
return -ERESTART;
@@ -2181,7 +2181,7 @@ static int relinquish_memory(
int ret = 0;
/* Use a recursive lock, as we may enter 'free_domheap_page'. */
- spin_lock_recursive(&d->page_alloc_lock);
+ rspin_lock(&d->page_alloc_lock);
while ( (page = page_list_remove_head(list)) )
{
@@ -2322,7 +2322,7 @@ static int relinquish_memory(
page_list_move(list, &d->arch.relmem_list);
out:
- spin_unlock_recursive(&d->page_alloc_lock);
+ rspin_unlock(&d->page_alloc_lock);
return ret;
}
diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c
index 142258f16a..9585406095 100644
--- a/xen/arch/x86/mm/mem_sharing.c
+++ b/xen/arch/x86/mm/mem_sharing.c
@@ -688,7 +688,7 @@ static int page_make_sharable(struct domain *d,
int rc = 0;
bool drop_dom_ref = false;
- spin_lock_recursive(&d->page_alloc_lock);
+ rspin_lock(&d->page_alloc_lock);
if ( d->is_dying )
{
@@ -731,7 +731,7 @@ static int page_make_sharable(struct domain *d,
}
out:
- spin_unlock_recursive(&d->page_alloc_lock);
+ rspin_unlock(&d->page_alloc_lock);
if ( drop_dom_ref )
put_domain(d);
@@ -1942,7 +1942,7 @@ int mem_sharing_fork_reset(struct domain *d, bool reset_state,
goto state;
/* need recursive lock because we will free pages */
- spin_lock_recursive(&d->page_alloc_lock);
+ rspin_lock(&d->page_alloc_lock);
page_list_for_each_safe(page, tmp, &d->page_list)
{
shr_handle_t sh;
@@ -1971,7 +1971,7 @@ int mem_sharing_fork_reset(struct domain *d, bool reset_state,
put_page_alloc_ref(page);
put_page_and_type(page);
}
- spin_unlock_recursive(&d->page_alloc_lock);
+ rspin_unlock(&d->page_alloc_lock);
state:
if ( reset_state )
diff --git a/xen/arch/x86/mm/mm-locks.h b/xen/arch/x86/mm/mm-locks.h
index b05cad1752..c867ad7d53 100644
--- a/xen/arch/x86/mm/mm-locks.h
+++ b/xen/arch/x86/mm/mm-locks.h
@@ -79,7 +79,7 @@ static inline void _mm_lock(const struct domain *d, mm_lock_t *l,
{
if ( !((mm_locked_by_me(l)) && rec) )
_check_lock_level(d, level);
- spin_lock_recursive(&l->lock);
+ rspin_lock(&l->lock);
if ( l->lock.recurse_cnt == 1 )
{
l->locker_function = func;
@@ -200,7 +200,7 @@ static inline void mm_unlock(mm_lock_t *l)
l->locker_function = "nobody";
_set_lock_level(l->unlock_level);
}
- spin_unlock_recursive(&l->lock);
+ rspin_unlock(&l->lock);
}
static inline void mm_enforce_order_unlock(int unlock_level,
diff --git a/xen/common/ioreq.c b/xen/common/ioreq.c
index 652c18a9b5..1257a3d972 100644
--- a/xen/common/ioreq.c
+++ b/xen/common/ioreq.c
@@ -329,7 +329,7 @@ bool is_ioreq_server_page(struct domain *d, const struct page_info *page)
unsigned int id;
bool found = false;
- spin_lock_recursive(&d->ioreq_server.lock);
+ rspin_lock(&d->ioreq_server.lock);
FOR_EACH_IOREQ_SERVER(d, id, s)
{
@@ -340,7 +340,7 @@ bool is_ioreq_server_page(struct domain *d, const struct page_info *page)
}
}
- spin_unlock_recursive(&d->ioreq_server.lock);
+ rspin_unlock(&d->ioreq_server.lock);
return found;
}
@@ -658,7 +658,7 @@ static int ioreq_server_create(struct domain *d, int bufioreq_handling,
return -ENOMEM;
domain_pause(d);
- spin_lock_recursive(&d->ioreq_server.lock);
+ rspin_lock(&d->ioreq_server.lock);
for ( i = 0; i < MAX_NR_IOREQ_SERVERS; i++ )
{
@@ -686,13 +686,13 @@ static int ioreq_server_create(struct domain *d, int bufioreq_handling,
if ( id )
*id = i;
- spin_unlock_recursive(&d->ioreq_server.lock);
+ rspin_unlock(&d->ioreq_server.lock);
domain_unpause(d);
return 0;
fail:
- spin_unlock_recursive(&d->ioreq_server.lock);
+ rspin_unlock(&d->ioreq_server.lock);
domain_unpause(d);
xfree(s);
@@ -704,7 +704,7 @@ static int ioreq_server_destroy(struct domain *d, ioservid_t id)
struct ioreq_server *s;
int rc;
- spin_lock_recursive(&d->ioreq_server.lock);
+ rspin_lock(&d->ioreq_server.lock);
s = get_ioreq_server(d, id);
@@ -736,7 +736,7 @@ static int ioreq_server_destroy(struct domain *d, ioservid_t id)
rc = 0;
out:
- spin_unlock_recursive(&d->ioreq_server.lock);
+ rspin_unlock(&d->ioreq_server.lock);
return rc;
}
@@ -749,7 +749,7 @@ static int ioreq_server_get_info(struct domain *d, ioservid_t id,
struct ioreq_server *s;
int rc;
- spin_lock_recursive(&d->ioreq_server.lock);
+ rspin_lock(&d->ioreq_server.lock);
s = get_ioreq_server(d, id);
@@ -783,7 +783,7 @@ static int ioreq_server_get_info(struct domain *d, ioservid_t id,
rc = 0;
out:
- spin_unlock_recursive(&d->ioreq_server.lock);
+ rspin_unlock(&d->ioreq_server.lock);
return rc;
}
@@ -796,7 +796,7 @@ int ioreq_server_get_frame(struct domain *d, ioservid_t id,
ASSERT(is_hvm_domain(d));
- spin_lock_recursive(&d->ioreq_server.lock);
+ rspin_lock(&d->ioreq_server.lock);
s = get_ioreq_server(d, id);
@@ -834,7 +834,7 @@ int ioreq_server_get_frame(struct domain *d, ioservid_t id,
}
out:
- spin_unlock_recursive(&d->ioreq_server.lock);
+ rspin_unlock(&d->ioreq_server.lock);
return rc;
}
@@ -850,7 +850,7 @@ static int ioreq_server_map_io_range(struct domain *d, ioservid_t id,
if ( start > end )
return -EINVAL;
- spin_lock_recursive(&d->ioreq_server.lock);
+ rspin_lock(&d->ioreq_server.lock);
s = get_ioreq_server(d, id);
@@ -886,7 +886,7 @@ static int ioreq_server_map_io_range(struct domain *d, ioservid_t id,
rc = rangeset_add_range(r, start, end);
out:
- spin_unlock_recursive(&d->ioreq_server.lock);
+ rspin_unlock(&d->ioreq_server.lock);
return rc;
}
@@ -902,7 +902,7 @@ static int ioreq_server_unmap_io_range(struct domain *d, ioservid_t id,
if ( start > end )
return -EINVAL;
- spin_lock_recursive(&d->ioreq_server.lock);
+ rspin_lock(&d->ioreq_server.lock);
s = get_ioreq_server(d, id);
@@ -938,7 +938,7 @@ static int ioreq_server_unmap_io_range(struct domain *d, ioservid_t id,
rc = rangeset_remove_range(r, start, end);
out:
- spin_unlock_recursive(&d->ioreq_server.lock);
+ rspin_unlock(&d->ioreq_server.lock);
return rc;
}
@@ -963,7 +963,7 @@ int ioreq_server_map_mem_type(struct domain *d, ioservid_t id,
if ( flags & ~XEN_DMOP_IOREQ_MEM_ACCESS_WRITE )
return -EINVAL;
- spin_lock_recursive(&d->ioreq_server.lock);
+ rspin_lock(&d->ioreq_server.lock);
s = get_ioreq_server(d, id);
@@ -978,7 +978,7 @@ int ioreq_server_map_mem_type(struct domain *d, ioservid_t id,
rc = arch_ioreq_server_map_mem_type(d, s, flags);
out:
- spin_unlock_recursive(&d->ioreq_server.lock);
+ rspin_unlock(&d->ioreq_server.lock);
if ( rc == 0 )
arch_ioreq_server_map_mem_type_completed(d, s, flags);
@@ -992,7 +992,7 @@ static int ioreq_server_set_state(struct domain *d, ioservid_t id,
struct ioreq_server *s;
int rc;
- spin_lock_recursive(&d->ioreq_server.lock);
+ rspin_lock(&d->ioreq_server.lock);
s = get_ioreq_server(d, id);
@@ -1016,7 +1016,7 @@ static int ioreq_server_set_state(struct domain *d, ioservid_t id,
rc = 0;
out:
- spin_unlock_recursive(&d->ioreq_server.lock);
+ rspin_unlock(&d->ioreq_server.lock);
return rc;
}
@@ -1026,7 +1026,7 @@ int ioreq_server_add_vcpu_all(struct domain *d, struct vcpu *v)
unsigned int id;
int rc;
- spin_lock_recursive(&d->ioreq_server.lock);
+ rspin_lock(&d->ioreq_server.lock);
FOR_EACH_IOREQ_SERVER(d, id, s)
{
@@ -1035,7 +1035,7 @@ int ioreq_server_add_vcpu_all(struct domain *d, struct vcpu *v)
goto fail;
}
- spin_unlock_recursive(&d->ioreq_server.lock);
+ rspin_unlock(&d->ioreq_server.lock);
return 0;
@@ -1050,7 +1050,7 @@ int ioreq_server_add_vcpu_all(struct domain *d, struct vcpu *v)
ioreq_server_remove_vcpu(s, v);
}
- spin_unlock_recursive(&d->ioreq_server.lock);
+ rspin_unlock(&d->ioreq_server.lock);
return rc;
}
@@ -1060,12 +1060,12 @@ void ioreq_server_remove_vcpu_all(struct domain *d, struct vcpu *v)
struct ioreq_server *s;
unsigned int id;
- spin_lock_recursive(&d->ioreq_server.lock);
+ rspin_lock(&d->ioreq_server.lock);
FOR_EACH_IOREQ_SERVER(d, id, s)
ioreq_server_remove_vcpu(s, v);
- spin_unlock_recursive(&d->ioreq_server.lock);
+ rspin_unlock(&d->ioreq_server.lock);
}
void ioreq_server_destroy_all(struct domain *d)
@@ -1076,7 +1076,7 @@ void ioreq_server_destroy_all(struct domain *d)
if ( !arch_ioreq_server_destroy_all(d) )
return;
- spin_lock_recursive(&d->ioreq_server.lock);
+ rspin_lock(&d->ioreq_server.lock);
/* No need to domain_pause() as the domain is being torn down */
@@ -1094,7 +1094,7 @@ void ioreq_server_destroy_all(struct domain *d)
xfree(s);
}
- spin_unlock_recursive(&d->ioreq_server.lock);
+ rspin_unlock(&d->ioreq_server.lock);
}
struct ioreq_server *ioreq_server_select(struct domain *d,
diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index 9b5df74fdd..8c6a3d9274 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -2497,7 +2497,7 @@ void free_domheap_pages(struct page_info *pg, unsigned int order)
if ( unlikely(is_xen_heap_page(pg)) )
{
/* NB. May recursively lock from relinquish_memory(). */
- spin_lock_recursive(&d->page_alloc_lock);
+ rspin_lock(&d->page_alloc_lock);
for ( i = 0; i < (1 << order); i++ )
arch_free_heap_page(d, &pg[i]);
@@ -2505,7 +2505,7 @@ void free_domheap_pages(struct page_info *pg, unsigned int order)
d->xenheap_pages -= 1 << order;
drop_dom_ref = (d->xenheap_pages == 0);
- spin_unlock_recursive(&d->page_alloc_lock);
+ rspin_unlock(&d->page_alloc_lock);
}
else
{
@@ -2514,7 +2514,7 @@ void free_domheap_pages(struct page_info *pg, unsigned int order)
if ( likely(d) && likely(d != dom_cow) )
{
/* NB. May recursively lock from relinquish_memory(). */
- spin_lock_recursive(&d->page_alloc_lock);
+ rspin_lock(&d->page_alloc_lock);
for ( i = 0; i < (1 << order); i++ )
{
@@ -2537,7 +2537,7 @@ void free_domheap_pages(struct page_info *pg, unsigned int order)
drop_dom_ref = !domain_adjust_tot_pages(d, -(1 << order));
- spin_unlock_recursive(&d->page_alloc_lock);
+ rspin_unlock(&d->page_alloc_lock);
/*
* Normally we expect a domain to clear pages before freeing them,
@@ -2753,7 +2753,7 @@ void free_domstatic_page(struct page_info *page)
ASSERT_ALLOC_CONTEXT();
/* NB. May recursively lock from relinquish_memory(). */
- spin_lock_recursive(&d->page_alloc_lock);
+ rspin_lock(&d->page_alloc_lock);
arch_free_heap_page(d, page);
@@ -2764,7 +2764,7 @@ void free_domstatic_page(struct page_info *page)
/* Add page on the resv_page_list *after* it has been freed. */
page_list_add_tail(page, &d->resv_page_list);
- spin_unlock_recursive(&d->page_alloc_lock);
+ rspin_unlock(&d->page_alloc_lock);
if ( drop_dom_ref )
put_domain(d);
diff --git a/xen/common/spinlock.c b/xen/common/spinlock.c
index ce18fbdd8a..26c667d3cc 100644
--- a/xen/common/spinlock.c
+++ b/xen/common/spinlock.c
@@ -436,7 +436,7 @@ void _spin_barrier(spinlock_t *lock)
smp_mb();
}
-int _spin_trylock_recursive(spinlock_t *lock)
+int rspin_trylock(rspinlock_t *lock)
{
unsigned int cpu = smp_processor_id();
@@ -460,7 +460,7 @@ int _spin_trylock_recursive(spinlock_t *lock)
return 1;
}
-void _spin_lock_recursive(spinlock_t *lock)
+void rspin_lock(rspinlock_t *lock)
{
unsigned int cpu = smp_processor_id();
@@ -475,7 +475,7 @@ void _spin_lock_recursive(spinlock_t *lock)
lock->recurse_cnt++;
}
-void _spin_unlock_recursive(spinlock_t *lock)
+void rspin_unlock(rspinlock_t *lock)
{
if ( likely(--lock->recurse_cnt == 0) )
{
diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
index 8b161488f6..369b2f9077 100644
--- a/xen/drivers/char/console.c
+++ b/xen/drivers/char/console.c
@@ -920,7 +920,7 @@ static void vprintk_common(const char *prefix, const char *fmt, va_list args)
/* console_lock can be acquired recursively from __printk_ratelimit(). */
local_irq_save(flags);
- spin_lock_recursive(&console_lock);
+ rspin_lock(&console_lock);
state = &this_cpu(state);
(void)vsnprintf(buf, sizeof(buf), fmt, args);
@@ -956,7 +956,7 @@ static void vprintk_common(const char *prefix, const char *fmt, va_list args)
state->continued = 1;
}
- spin_unlock_recursive(&console_lock);
+ rspin_unlock(&console_lock);
local_irq_restore(flags);
}
@@ -1163,14 +1163,14 @@ unsigned long console_lock_recursive_irqsave(void)
unsigned long flags;
local_irq_save(flags);
- spin_lock_recursive(&console_lock);
+ rspin_lock(&console_lock);
return flags;
}
void console_unlock_recursive_irqrestore(unsigned long flags)
{
- spin_unlock_recursive(&console_lock);
+ rspin_unlock(&console_lock);
local_irq_restore(flags);
}
@@ -1231,12 +1231,12 @@ int __printk_ratelimit(int ratelimit_ms, int ratelimit_burst)
char lost_str[8];
snprintf(lost_str, sizeof(lost_str), "%d", lost);
/* console_lock may already be acquired by printk(). */
- spin_lock_recursive(&console_lock);
+ rspin_lock(&console_lock);
printk_start_of_line("(XEN) ");
__putstr("printk: ");
__putstr(lost_str);
__putstr(" messages suppressed.\n");
- spin_unlock_recursive(&console_lock);
+ rspin_unlock(&console_lock);
}
local_irq_restore(flags);
return 1;
diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index 61be34e75f..22342f07ac 100644
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -54,12 +54,12 @@ static DEFINE_RSPINLOCK(_pcidevs_lock);
void pcidevs_lock(void)
{
- spin_lock_recursive(&_pcidevs_lock);
+ rspin_lock(&_pcidevs_lock);
}
void pcidevs_unlock(void)
{
- spin_unlock_recursive(&_pcidevs_lock);
+ rspin_unlock(&_pcidevs_lock);
}
bool pcidevs_locked(void)
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index c6604aef78..8cf751ad0c 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -358,8 +358,8 @@ struct sched_unit {
(v) = (v)->next_in_list )
/* Per-domain lock can be recursively acquired in fault handlers. */
-#define domain_lock(d) spin_lock_recursive(&(d)->domain_lock)
-#define domain_unlock(d) spin_unlock_recursive(&(d)->domain_lock)
+#define domain_lock(d) rspin_lock(&(d)->domain_lock)
+#define domain_unlock(d) rspin_unlock(&(d)->domain_lock)
struct evtchn_port_ops;
diff --git a/xen/include/xen/spinlock.h b/xen/include/xen/spinlock.h
index 19561d5e61..c99ee52458 100644
--- a/xen/include/xen/spinlock.h
+++ b/xen/include/xen/spinlock.h
@@ -209,9 +209,16 @@ int _spin_is_locked(const spinlock_t *lock);
int _spin_trylock(spinlock_t *lock);
void _spin_barrier(spinlock_t *lock);
-int _spin_trylock_recursive(spinlock_t *lock);
-void _spin_lock_recursive(spinlock_t *lock);
-void _spin_unlock_recursive(spinlock_t *lock);
+/*
+ * rspin_[un]lock(): Use these forms when the lock can (safely!) be
+ * reentered recursively on the same CPU. All critical regions that may form
+ * part of a recursively-nested set must be protected by these forms. If there
+ * are any critical regions that cannot form part of such a set, they can use
+ * standard spin_[un]lock().
+ */
+int rspin_trylock(rspinlock_t *lock);
+void rspin_lock(rspinlock_t *lock);
+void rspin_unlock(rspinlock_t *lock);
#define spin_lock(l) _spin_lock(l)
#define spin_lock_cb(l, c, d) _spin_lock_cb(l, c, d)
@@ -241,15 +248,4 @@ void _spin_unlock_recursive(spinlock_t *lock);
/* Ensure a lock is quiescent between two critical operations. */
#define spin_barrier(l) _spin_barrier(l)
-/*
- * spin_[un]lock_recursive(): Use these forms when the lock can (safely!) be
- * reentered recursively on the same CPU. All critical regions that may form
- * part of a recursively-nested set must be protected by these forms. If there
- * are any critical regions that cannot form part of such a set, they can use
- * standard spin_[un]lock().
- */
-#define spin_trylock_recursive(l) _spin_trylock_recursive(l)
-#define spin_lock_recursive(l) _spin_lock_recursive(l)
-#define spin_unlock_recursive(l) _spin_unlock_recursive(l)
-
#endif /* __SPINLOCK_H__ */
--
2.35.3
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH v3 06/13] xen/spinlock: add rspin_[un]lock_irq[save|restore]()
2023-11-20 11:38 [PATCH v3 00/13] xen/spinlock: make recursive spinlocks a dedicated type Juergen Gross
` (4 preceding siblings ...)
2023-11-20 11:38 ` [PATCH v3 05/13] xen/spinlock: rename recursive lock functions Juergen Gross
@ 2023-11-20 11:38 ` Juergen Gross
2023-11-24 19:23 ` Alejandro Vallejo
2023-11-20 11:38 ` [PATCH v3 07/13] xen/spinlock: make struct lock_profile rspinlock_t aware Juergen Gross
` (6 subsequent siblings)
12 siblings, 1 reply; 23+ messages in thread
From: Juergen Gross @ 2023-11-20 11:38 UTC (permalink / raw)
To: xen-devel
Cc: javi.merino, Juergen Gross, Jan Beulich, Andrew Cooper,
Roger Pau Monné, Wei Liu, George Dunlap, Julien Grall,
Stefano Stabellini
Instead of special casing rspin_lock_irqsave() and
rspin_unlock_irqrestore() for the console lock, add those functions
to spinlock handling and use them where needed.
Signed-off-by: Juergen Gross <jgross@suse.com>
---
V2:
- new patch
---
xen/arch/x86/traps.c | 14 ++++++++------
xen/common/spinlock.c | 16 ++++++++++++++++
xen/drivers/char/console.c | 18 +-----------------
xen/include/xen/console.h | 5 +++--
xen/include/xen/spinlock.h | 7 +++++++
5 files changed, 35 insertions(+), 25 deletions(-)
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index e1356f696a..f72769e79b 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -647,13 +647,15 @@ void show_stack_overflow(unsigned int cpu, const struct cpu_user_regs *regs)
void show_execution_state(const struct cpu_user_regs *regs)
{
/* Prevent interleaving of output. */
- unsigned long flags = console_lock_recursive_irqsave();
+ unsigned long flags;
+
+ rspin_lock_irqsave(&console_lock, flags);
show_registers(regs);
show_code(regs);
show_stack(regs);
- console_unlock_recursive_irqrestore(flags);
+ rspin_unlock_irqrestore(&console_lock, flags);
}
void cf_check show_execution_state_nonconst(struct cpu_user_regs *regs)
@@ -663,7 +665,7 @@ void cf_check show_execution_state_nonconst(struct cpu_user_regs *regs)
void vcpu_show_execution_state(struct vcpu *v)
{
- unsigned long flags = 0;
+ unsigned long flags;
if ( test_bit(_VPF_down, &v->pause_flags) )
{
@@ -698,7 +700,7 @@ void vcpu_show_execution_state(struct vcpu *v)
#endif
/* Prevent interleaving of output. */
- flags = console_lock_recursive_irqsave();
+ rspin_lock_irqsave(&console_lock, flags);
vcpu_show_registers(v);
@@ -708,7 +710,7 @@ void vcpu_show_execution_state(struct vcpu *v)
* Stop interleaving prevention: The necessary P2M lookups involve
* locking, which has to occur with IRQs enabled.
*/
- console_unlock_recursive_irqrestore(flags);
+ rspin_unlock_irqrestore(&console_lock, flags);
show_hvm_stack(v, &v->arch.user_regs);
}
@@ -717,7 +719,7 @@ void vcpu_show_execution_state(struct vcpu *v)
if ( guest_kernel_mode(v, &v->arch.user_regs) )
show_guest_stack(v, &v->arch.user_regs);
- console_unlock_recursive_irqrestore(flags);
+ rspin_unlock_irqrestore(&console_lock, flags);
}
#ifdef CONFIG_HVM
diff --git a/xen/common/spinlock.c b/xen/common/spinlock.c
index 26c667d3cc..17716fc4eb 100644
--- a/xen/common/spinlock.c
+++ b/xen/common/spinlock.c
@@ -475,6 +475,16 @@ void rspin_lock(rspinlock_t *lock)
lock->recurse_cnt++;
}
+unsigned long __rspin_lock_irqsave(rspinlock_t *lock)
+{
+ unsigned long flags;
+
+ local_irq_save(flags);
+ rspin_lock(lock);
+
+ return flags;
+}
+
void rspin_unlock(rspinlock_t *lock)
{
if ( likely(--lock->recurse_cnt == 0) )
@@ -484,6 +494,12 @@ void rspin_unlock(rspinlock_t *lock)
}
}
+void rspin_unlock_irqrestore(rspinlock_t *lock, unsigned long flags)
+{
+ rspin_unlock(lock);
+ local_irq_restore(flags);
+}
+
#ifdef CONFIG_DEBUG_LOCK_PROFILE
struct lock_profile_anc {
diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
index 369b2f9077..cc743b67ec 100644
--- a/xen/drivers/char/console.c
+++ b/xen/drivers/char/console.c
@@ -120,7 +120,7 @@ static int __read_mostly sercon_handle = -1;
int8_t __read_mostly opt_console_xen; /* console=xen */
#endif
-static DEFINE_RSPINLOCK(console_lock);
+DEFINE_RSPINLOCK(console_lock);
/*
* To control the amount of printing, thresholds are added.
@@ -1158,22 +1158,6 @@ void console_end_log_everything(void)
atomic_dec(&print_everything);
}
-unsigned long console_lock_recursive_irqsave(void)
-{
- unsigned long flags;
-
- local_irq_save(flags);
- rspin_lock(&console_lock);
-
- return flags;
-}
-
-void console_unlock_recursive_irqrestore(unsigned long flags)
-{
- rspin_unlock(&console_lock);
- local_irq_restore(flags);
-}
-
void console_force_unlock(void)
{
watchdog_disable();
diff --git a/xen/include/xen/console.h b/xen/include/xen/console.h
index ab5c30c0da..dff0096b27 100644
--- a/xen/include/xen/console.h
+++ b/xen/include/xen/console.h
@@ -8,8 +8,11 @@
#define __CONSOLE_H__
#include <xen/inttypes.h>
+#include <xen/spinlock.h>
#include <public/xen.h>
+extern rspinlock_t console_lock;
+
struct xen_sysctl_readconsole;
long read_console_ring(struct xen_sysctl_readconsole *op);
@@ -20,8 +23,6 @@ void console_init_postirq(void);
void console_endboot(void);
int console_has(const char *device);
-unsigned long console_lock_recursive_irqsave(void);
-void console_unlock_recursive_irqrestore(unsigned long flags);
void console_force_unlock(void);
void console_start_sync(void);
diff --git a/xen/include/xen/spinlock.h b/xen/include/xen/spinlock.h
index c99ee52458..53f0f72ac4 100644
--- a/xen/include/xen/spinlock.h
+++ b/xen/include/xen/spinlock.h
@@ -218,7 +218,14 @@ void _spin_barrier(spinlock_t *lock);
*/
int rspin_trylock(rspinlock_t *lock);
void rspin_lock(rspinlock_t *lock);
+#define rspin_lock_irqsave(l, f) \
+ ({ \
+ BUILD_BUG_ON(sizeof(f) != sizeof(unsigned long)); \
+ ((f) = __rspin_lock_irqsave(l)); \
+ })
+unsigned long __rspin_lock_irqsave(rspinlock_t *lock);
void rspin_unlock(rspinlock_t *lock);
+void rspin_unlock_irqrestore(rspinlock_t *lock, unsigned long flags);
#define spin_lock(l) _spin_lock(l)
#define spin_lock_cb(l, c, d) _spin_lock_cb(l, c, d)
--
2.35.3
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH v3 06/13] xen/spinlock: add rspin_[un]lock_irq[save|restore]()
2023-11-20 11:38 ` [PATCH v3 06/13] xen/spinlock: add rspin_[un]lock_irq[save|restore]() Juergen Gross
@ 2023-11-24 19:23 ` Alejandro Vallejo
2023-11-28 13:24 ` Juergen Gross
0 siblings, 1 reply; 23+ messages in thread
From: Alejandro Vallejo @ 2023-11-24 19:23 UTC (permalink / raw)
To: Juergen Gross, xen-devel
Cc: javi.merino, Jan Beulich, Andrew Cooper, Roger Pau Monné,
Wei Liu, George Dunlap, Julien Grall, Stefano Stabellini
On 20/11/2023 11:38, Juergen Gross wrote:
> Instead of special casing rspin_lock_irqsave() and
> rspin_unlock_irqrestore() for the console lock, add those functions
> to spinlock handling and use them where needed.
>
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
> V2:
> - new patch
> ---
> xen/arch/x86/traps.c | 14 ++++++++------
> xen/common/spinlock.c | 16 ++++++++++++++++
> xen/drivers/char/console.c | 18 +-----------------
> xen/include/xen/console.h | 5 +++--
> xen/include/xen/spinlock.h | 7 +++++++
> 5 files changed, 35 insertions(+), 25 deletions(-)
>
> diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
> index e1356f696a..f72769e79b 100644
> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -647,13 +647,15 @@ void show_stack_overflow(unsigned int cpu, const struct cpu_user_regs *regs)
> void show_execution_state(const struct cpu_user_regs *regs)
> {
> /* Prevent interleaving of output. */
> - unsigned long flags = console_lock_recursive_irqsave();
> + unsigned long flags;
> +
> + rspin_lock_irqsave(&console_lock, flags);
This feels a bit weird because rspin_lock_irqsave() being lowercase
hints that it's a either a function or behaves like one. For that it
should take &flags instead.
>
> show_registers(regs);
> show_code(regs);
> show_stack(regs);
>
> - console_unlock_recursive_irqrestore(flags);
> + rspin_unlock_irqrestore(&console_lock, flags);
> }
>
> void cf_check show_execution_state_nonconst(struct cpu_user_regs *regs)
> @@ -663,7 +665,7 @@ void cf_check show_execution_state_nonconst(struct cpu_user_regs *regs)
>
> void vcpu_show_execution_state(struct vcpu *v)
> {
> - unsigned long flags = 0;
> + unsigned long flags;
>
> if ( test_bit(_VPF_down, &v->pause_flags) )
> {
> @@ -698,7 +700,7 @@ void vcpu_show_execution_state(struct vcpu *v)
> #endif
>
> /* Prevent interleaving of output. */
> - flags = console_lock_recursive_irqsave();
> + rspin_lock_irqsave(&console_lock, flags);
>
> vcpu_show_registers(v);
>
> @@ -708,7 +710,7 @@ void vcpu_show_execution_state(struct vcpu *v)
> * Stop interleaving prevention: The necessary P2M lookups involve
> * locking, which has to occur with IRQs enabled.
> */
> - console_unlock_recursive_irqrestore(flags);
> + rspin_unlock_irqrestore(&console_lock, flags);
>
> show_hvm_stack(v, &v->arch.user_regs);
> }
> @@ -717,7 +719,7 @@ void vcpu_show_execution_state(struct vcpu *v)
> if ( guest_kernel_mode(v, &v->arch.user_regs) )
> show_guest_stack(v, &v->arch.user_regs);
>
> - console_unlock_recursive_irqrestore(flags);
> + rspin_unlock_irqrestore(&console_lock, flags);
> }
>
> #ifdef CONFIG_HVM
> diff --git a/xen/common/spinlock.c b/xen/common/spinlock.c
> index 26c667d3cc..17716fc4eb 100644
> --- a/xen/common/spinlock.c
> +++ b/xen/common/spinlock.c
> @@ -475,6 +475,16 @@ void rspin_lock(rspinlock_t *lock)
> lock->recurse_cnt++;
> }
>
> +unsigned long __rspin_lock_irqsave(rspinlock_t *lock)
> +{
> + unsigned long flags;
> +
> + local_irq_save(flags);
> + rspin_lock(lock);
> +
> + return flags;
> +}
> +
> void rspin_unlock(rspinlock_t *lock)
> {
> if ( likely(--lock->recurse_cnt == 0) )
> @@ -484,6 +494,12 @@ void rspin_unlock(rspinlock_t *lock)
> }
> }
>
> +void rspin_unlock_irqrestore(rspinlock_t *lock, unsigned long flags)
> +{
> + rspin_unlock(lock);
> + local_irq_restore(flags);
> +}
> +
> #ifdef CONFIG_DEBUG_LOCK_PROFILE
>
> struct lock_profile_anc {
> diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
> index 369b2f9077..cc743b67ec 100644
> --- a/xen/drivers/char/console.c
> +++ b/xen/drivers/char/console.c
> @@ -120,7 +120,7 @@ static int __read_mostly sercon_handle = -1;
> int8_t __read_mostly opt_console_xen; /* console=xen */
> #endif
>
> -static DEFINE_RSPINLOCK(console_lock);
> +DEFINE_RSPINLOCK(console_lock);
>
> /*
> * To control the amount of printing, thresholds are added.
> @@ -1158,22 +1158,6 @@ void console_end_log_everything(void)
> atomic_dec(&print_everything);
> }
>
> -unsigned long console_lock_recursive_irqsave(void)
> -{
> - unsigned long flags;
> -
> - local_irq_save(flags);
> - rspin_lock(&console_lock);
> -
> - return flags;
> -}
> -
> -void console_unlock_recursive_irqrestore(unsigned long flags)
> -{
> - rspin_unlock(&console_lock);
> - local_irq_restore(flags);
> -}
> -
> void console_force_unlock(void)
> {
> watchdog_disable();
> diff --git a/xen/include/xen/console.h b/xen/include/xen/console.h
> index ab5c30c0da..dff0096b27 100644
> --- a/xen/include/xen/console.h
> +++ b/xen/include/xen/console.h
> @@ -8,8 +8,11 @@
> #define __CONSOLE_H__
>
> #include <xen/inttypes.h>
> +#include <xen/spinlock.h>
> #include <public/xen.h>
>
> +extern rspinlock_t console_lock;
> +
> struct xen_sysctl_readconsole;
> long read_console_ring(struct xen_sysctl_readconsole *op);
>
> @@ -20,8 +23,6 @@ void console_init_postirq(void);
> void console_endboot(void);
> int console_has(const char *device);
>
> -unsigned long console_lock_recursive_irqsave(void);
> -void console_unlock_recursive_irqrestore(unsigned long flags);
> void console_force_unlock(void);
>
> void console_start_sync(void);
> diff --git a/xen/include/xen/spinlock.h b/xen/include/xen/spinlock.h
> index c99ee52458..53f0f72ac4 100644
> --- a/xen/include/xen/spinlock.h
> +++ b/xen/include/xen/spinlock.h
> @@ -218,7 +218,14 @@ void _spin_barrier(spinlock_t *lock);
> */
> int rspin_trylock(rspinlock_t *lock);
> void rspin_lock(rspinlock_t *lock);
> +#define rspin_lock_irqsave(l, f) \
> + ({ \
> + BUILD_BUG_ON(sizeof(f) != sizeof(unsigned long)); \
> + ((f) = __rspin_lock_irqsave(l)); \
> + })
If f is &flags, then s/f/*(f)/ would be needed in these 2 cases.
On other matters if we had -Wconversion turned on by default that
BUILD_BUG_ON() wouldn't be needed. Not that you can do it (I'm sure the
codebase would explode everywhere if we switched it on), but might be
something to consider in the future.
> +unsigned long __rspin_lock_irqsave(rspinlock_t *lock);
> void rspin_unlock(rspinlock_t *lock);
> +void rspin_unlock_irqrestore(rspinlock_t *lock, unsigned long flags);
>
> #define spin_lock(l) _spin_lock(l)
> #define spin_lock_cb(l, c, d) _spin_lock_cb(l, c, d)
Cheers,
Alejandro
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH v3 06/13] xen/spinlock: add rspin_[un]lock_irq[save|restore]()
2023-11-24 19:23 ` Alejandro Vallejo
@ 2023-11-28 13:24 ` Juergen Gross
0 siblings, 0 replies; 23+ messages in thread
From: Juergen Gross @ 2023-11-28 13:24 UTC (permalink / raw)
To: Alejandro Vallejo, xen-devel
Cc: javi.merino, Jan Beulich, Andrew Cooper, Roger Pau Monné,
Wei Liu, George Dunlap, Julien Grall, Stefano Stabellini
[-- Attachment #1.1.1: Type: text/plain, Size: 7612 bytes --]
On 24.11.23 20:23, Alejandro Vallejo wrote:
> On 20/11/2023 11:38, Juergen Gross wrote:
>> Instead of special casing rspin_lock_irqsave() and
>> rspin_unlock_irqrestore() for the console lock, add those functions
>> to spinlock handling and use them where needed.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>> V2:
>> - new patch
>> ---
>> xen/arch/x86/traps.c | 14 ++++++++------
>> xen/common/spinlock.c | 16 ++++++++++++++++
>> xen/drivers/char/console.c | 18 +-----------------
>> xen/include/xen/console.h | 5 +++--
>> xen/include/xen/spinlock.h | 7 +++++++
>> 5 files changed, 35 insertions(+), 25 deletions(-)
>>
>> diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
>> index e1356f696a..f72769e79b 100644
>> --- a/xen/arch/x86/traps.c
>> +++ b/xen/arch/x86/traps.c
>> @@ -647,13 +647,15 @@ void show_stack_overflow(unsigned int cpu, const struct
>> cpu_user_regs *regs)
>> void show_execution_state(const struct cpu_user_regs *regs)
>> {
>> /* Prevent interleaving of output. */
>> - unsigned long flags = console_lock_recursive_irqsave();
>> + unsigned long flags;
>> +
>> + rspin_lock_irqsave(&console_lock, flags);
>
> This feels a bit weird because rspin_lock_irqsave() being lowercase
> hints that it's a either a function or behaves like one. For that it
> should take &flags instead.
I don't think so. This is common behavior with the Linux kernel, and I
think we should keep it. Especially as I believe rspin_lock_irqsave()
and spin_lock_irqsave() should have a similar interface. Or would you want
us to change more than 250 call sites of spin_lock_irqsave()?
>
>> show_registers(regs);
>> show_code(regs);
>> show_stack(regs);
>> - console_unlock_recursive_irqrestore(flags);
>> + rspin_unlock_irqrestore(&console_lock, flags);
>> }
>> void cf_check show_execution_state_nonconst(struct cpu_user_regs *regs)
>> @@ -663,7 +665,7 @@ void cf_check show_execution_state_nonconst(struct
>> cpu_user_regs *regs)
>> void vcpu_show_execution_state(struct vcpu *v)
>> {
>> - unsigned long flags = 0;
>> + unsigned long flags;
>> if ( test_bit(_VPF_down, &v->pause_flags) )
>> {
>> @@ -698,7 +700,7 @@ void vcpu_show_execution_state(struct vcpu *v)
>> #endif
>> /* Prevent interleaving of output. */
>> - flags = console_lock_recursive_irqsave();
>> + rspin_lock_irqsave(&console_lock, flags);
>> vcpu_show_registers(v);
>> @@ -708,7 +710,7 @@ void vcpu_show_execution_state(struct vcpu *v)
>> * Stop interleaving prevention: The necessary P2M lookups involve
>> * locking, which has to occur with IRQs enabled.
>> */
>> - console_unlock_recursive_irqrestore(flags);
>> + rspin_unlock_irqrestore(&console_lock, flags);
>> show_hvm_stack(v, &v->arch.user_regs);
>> }
>> @@ -717,7 +719,7 @@ void vcpu_show_execution_state(struct vcpu *v)
>> if ( guest_kernel_mode(v, &v->arch.user_regs) )
>> show_guest_stack(v, &v->arch.user_regs);
>> - console_unlock_recursive_irqrestore(flags);
>> + rspin_unlock_irqrestore(&console_lock, flags);
>> }
>> #ifdef CONFIG_HVM
>> diff --git a/xen/common/spinlock.c b/xen/common/spinlock.c
>> index 26c667d3cc..17716fc4eb 100644
>> --- a/xen/common/spinlock.c
>> +++ b/xen/common/spinlock.c
>> @@ -475,6 +475,16 @@ void rspin_lock(rspinlock_t *lock)
>> lock->recurse_cnt++;
>> }
>> +unsigned long __rspin_lock_irqsave(rspinlock_t *lock)
>> +{
>> + unsigned long flags;
>> +
>> + local_irq_save(flags);
>> + rspin_lock(lock);
>> +
>> + return flags;
>> +}
>> +
>> void rspin_unlock(rspinlock_t *lock)
>> {
>> if ( likely(--lock->recurse_cnt == 0) )
>> @@ -484,6 +494,12 @@ void rspin_unlock(rspinlock_t *lock)
>> }
>> }
>> +void rspin_unlock_irqrestore(rspinlock_t *lock, unsigned long flags)
>> +{
>> + rspin_unlock(lock);
>> + local_irq_restore(flags);
>> +}
>> +
>> #ifdef CONFIG_DEBUG_LOCK_PROFILE
>> struct lock_profile_anc {
>> diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
>> index 369b2f9077..cc743b67ec 100644
>> --- a/xen/drivers/char/console.c
>> +++ b/xen/drivers/char/console.c
>> @@ -120,7 +120,7 @@ static int __read_mostly sercon_handle = -1;
>> int8_t __read_mostly opt_console_xen; /* console=xen */
>> #endif
>> -static DEFINE_RSPINLOCK(console_lock);
>> +DEFINE_RSPINLOCK(console_lock);
>> /*
>> * To control the amount of printing, thresholds are added.
>> @@ -1158,22 +1158,6 @@ void console_end_log_everything(void)
>> atomic_dec(&print_everything);
>> }
>> -unsigned long console_lock_recursive_irqsave(void)
>> -{
>> - unsigned long flags;
>> -
>> - local_irq_save(flags);
>> - rspin_lock(&console_lock);
>> -
>> - return flags;
>> -}
>> -
>> -void console_unlock_recursive_irqrestore(unsigned long flags)
>> -{
>> - rspin_unlock(&console_lock);
>> - local_irq_restore(flags);
>> -}
>> -
>> void console_force_unlock(void)
>> {
>> watchdog_disable();
>> diff --git a/xen/include/xen/console.h b/xen/include/xen/console.h
>> index ab5c30c0da..dff0096b27 100644
>> --- a/xen/include/xen/console.h
>> +++ b/xen/include/xen/console.h
>> @@ -8,8 +8,11 @@
>> #define __CONSOLE_H__
>> #include <xen/inttypes.h>
>> +#include <xen/spinlock.h>
>> #include <public/xen.h>
>> +extern rspinlock_t console_lock;
>> +
>> struct xen_sysctl_readconsole;
>> long read_console_ring(struct xen_sysctl_readconsole *op);
>> @@ -20,8 +23,6 @@ void console_init_postirq(void);
>> void console_endboot(void);
>> int console_has(const char *device);
>> -unsigned long console_lock_recursive_irqsave(void);
>> -void console_unlock_recursive_irqrestore(unsigned long flags);
>> void console_force_unlock(void);
>> void console_start_sync(void);
>> diff --git a/xen/include/xen/spinlock.h b/xen/include/xen/spinlock.h
>> index c99ee52458..53f0f72ac4 100644
>> --- a/xen/include/xen/spinlock.h
>> +++ b/xen/include/xen/spinlock.h
>> @@ -218,7 +218,14 @@ void _spin_barrier(spinlock_t *lock);
>> */
>> int rspin_trylock(rspinlock_t *lock);
>> void rspin_lock(rspinlock_t *lock);
>> +#define rspin_lock_irqsave(l, f) \
>> + ({ \
>> + BUILD_BUG_ON(sizeof(f) != sizeof(unsigned long)); \
>> + ((f) = __rspin_lock_irqsave(l)); \
>> + })
>
> If f is &flags, then s/f/*(f)/ would be needed in these 2 cases.
>
> On other matters if we had -Wconversion turned on by default that
> BUILD_BUG_ON() wouldn't be needed. Not that you can do it (I'm sure the codebase
> would explode everywhere if we switched it on), but might be
> something to consider in the future.
Again, this is like Linux kernel. And I don't think we want explicit casts
everywhere when e.g. moving values between fixed sized fields and standard
integer typed fields.
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 --]
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v3 07/13] xen/spinlock: make struct lock_profile rspinlock_t aware
2023-11-20 11:38 [PATCH v3 00/13] xen/spinlock: make recursive spinlocks a dedicated type Juergen Gross
` (5 preceding siblings ...)
2023-11-20 11:38 ` [PATCH v3 06/13] xen/spinlock: add rspin_[un]lock_irq[save|restore]() Juergen Gross
@ 2023-11-20 11:38 ` Juergen Gross
2023-11-24 20:09 ` Alejandro Vallejo
2023-11-20 11:38 ` [PATCH v3 08/13] xen/spinlock: add explicit non-recursive locking functions Juergen Gross
` (5 subsequent siblings)
12 siblings, 1 reply; 23+ messages in thread
From: Juergen Gross @ 2023-11-20 11:38 UTC (permalink / raw)
To: xen-devel
Cc: javi.merino, Juergen Gross, Andrew Cooper, George Dunlap,
Jan Beulich, Julien Grall, Stefano Stabellini, Wei Liu
Struct lock_profile contains a pointer to the spinlock it is associated
with. Prepare support of differing spinlock_t and rspinlock_t types by
adding a type indicator of the pointer. Use the highest bit of the
block_cnt member for this indicator in order to not grow the struct
while hurting only the slow path with slightly less performant code.
Signed-off-by: Juergen Gross <jgross@suse.com>
---
V2:
- new patch
---
xen/common/spinlock.c | 26 +++++++++++++++++++-------
xen/include/xen/spinlock.h | 10 ++++++++--
2 files changed, 27 insertions(+), 9 deletions(-)
diff --git a/xen/common/spinlock.c b/xen/common/spinlock.c
index 17716fc4eb..65f180203a 100644
--- a/xen/common/spinlock.c
+++ b/xen/common/spinlock.c
@@ -538,19 +538,31 @@ static void spinlock_profile_iterate(lock_profile_subfunc *sub, void *par)
static void cf_check spinlock_profile_print_elem(struct lock_profile *data,
int32_t type, int32_t idx, void *par)
{
- struct spinlock *lock = data->lock;
+ unsigned int cpu;
+ uint32_t lockval;
+
+ if ( data->is_rlock )
+ {
+ cpu = data->rlock->debug.cpu;
+ lockval = data->rlock->tickets.head_tail;
+ }
+ else
+ {
+ cpu = data->lock->debug.cpu;
+ lockval = data->lock->tickets.head_tail;
+ }
printk("%s ", lock_profile_ancs[type].name);
if ( type != LOCKPROF_TYPE_GLOBAL )
printk("%d ", idx);
- printk("%s: addr=%p, lockval=%08x, ", data->name, lock,
- lock->tickets.head_tail);
- if ( lock->debug.cpu == SPINLOCK_NO_CPU )
+ printk("%s: addr=%p, lockval=%08x, ", data->name, data->lock, lockval);
+ if ( cpu == SPINLOCK_NO_CPU )
printk("not locked\n");
else
- printk("cpu=%d\n", lock->debug.cpu);
- printk(" lock:%" PRId64 "(%" PRI_stime "), block:%" PRId64 "(%" PRI_stime ")\n",
- data->lock_cnt, data->time_hold, data->block_cnt, data->time_block);
+ printk("cpu=%u\n", cpu);
+ printk(" lock:%" PRIu64 "(%" PRI_stime "), block:%" PRIu64 "(%" PRI_stime ")\n",
+ data->lock_cnt, data->time_hold, (uint64_t)data->block_cnt,
+ data->time_block);
}
void cf_check spinlock_profile_printall(unsigned char key)
diff --git a/xen/include/xen/spinlock.h b/xen/include/xen/spinlock.h
index 53f0f72ac4..5ada9dce3d 100644
--- a/xen/include/xen/spinlock.h
+++ b/xen/include/xen/spinlock.h
@@ -76,13 +76,19 @@ union lock_debug { };
*/
struct spinlock;
+/* Temporary hack until a dedicated struct rspinlock is existing. */
+#define rspinlock spinlock
struct lock_profile {
struct lock_profile *next; /* forward link */
const char *name; /* lock name */
- struct spinlock *lock; /* the lock itself */
+ union {
+ struct spinlock *lock; /* the lock itself */
+ struct rspinlock *rlock; /* the recursive lock itself */
+ };
uint64_t lock_cnt; /* # of complete locking ops */
- uint64_t block_cnt; /* # of complete wait for lock */
+ uint64_t block_cnt:63; /* # of complete wait for lock */
+ uint64_t is_rlock:1; /* use rlock pointer */
s_time_t time_hold; /* cumulated lock time */
s_time_t time_block; /* cumulated wait time */
s_time_t time_locked; /* system time of last locking */
--
2.35.3
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH v3 07/13] xen/spinlock: make struct lock_profile rspinlock_t aware
2023-11-20 11:38 ` [PATCH v3 07/13] xen/spinlock: make struct lock_profile rspinlock_t aware Juergen Gross
@ 2023-11-24 20:09 ` Alejandro Vallejo
0 siblings, 0 replies; 23+ messages in thread
From: Alejandro Vallejo @ 2023-11-24 20:09 UTC (permalink / raw)
To: Juergen Gross, xen-devel
Cc: javi.merino, Andrew Cooper, George Dunlap, Jan Beulich,
Julien Grall, Stefano Stabellini, Wei Liu
On 20/11/2023 11:38, Juergen Gross wrote:
> Struct lock_profile contains a pointer to the spinlock it is associated
> with. Prepare support of differing spinlock_t and rspinlock_t types by
> adding a type indicator of the pointer. Use the highest bit of the
> block_cnt member for this indicator in order to not grow the struct
> while hurting only the slow path with slightly less performant code.
>
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
> V2:
> - new patch
> ---
> xen/common/spinlock.c | 26 +++++++++++++++++++-------
> xen/include/xen/spinlock.h | 10 ++++++++--
> 2 files changed, 27 insertions(+), 9 deletions(-)
>
> diff --git a/xen/common/spinlock.c b/xen/common/spinlock.c
> index 17716fc4eb..65f180203a 100644
> --- a/xen/common/spinlock.c
> +++ b/xen/common/spinlock.c
> @@ -538,19 +538,31 @@ static void spinlock_profile_iterate(lock_profile_subfunc *sub, void *par)
> static void cf_check spinlock_profile_print_elem(struct lock_profile *data,
> int32_t type, int32_t idx, void *par)
> {
> - struct spinlock *lock = data->lock;
> + unsigned int cpu;
> + uint32_t lockval;
> +
> + if ( data->is_rlock )
> + {
> + cpu = data->rlock->debug.cpu;
> + lockval = data->rlock->tickets.head_tail;
> + }
> + else
> + {
> + cpu = data->lock->debug.cpu;
> + lockval = data->lock->tickets.head_tail;
> + }
>
> printk("%s ", lock_profile_ancs[type].name);
> if ( type != LOCKPROF_TYPE_GLOBAL )
> printk("%d ", idx);
> - printk("%s: addr=%p, lockval=%08x, ", data->name, lock,
> - lock->tickets.head_tail);
> - if ( lock->debug.cpu == SPINLOCK_NO_CPU )
> + printk("%s: addr=%p, lockval=%08x, ", data->name, data->lock, lockval); > + if ( cpu == SPINLOCK_NO_CPU )
> printk("not locked\n");
> else
> - printk("cpu=%d\n", lock->debug.cpu);
> - printk(" lock:%" PRId64 "(%" PRI_stime "), block:%" PRId64 "(%" PRI_stime ")\n",
> - data->lock_cnt, data->time_hold, data->block_cnt, data->time_block);
> + printk("cpu=%u\n", cpu);
> + printk(" lock:%" PRIu64 "(%" PRI_stime "), block:%" PRIu64 "(%" PRI_stime ")\n",
> + data->lock_cnt, data->time_hold, (uint64_t)data->block_cnt,
> + data->time_block);
> }
>
> void cf_check spinlock_profile_printall(unsigned char key)
> diff --git a/xen/include/xen/spinlock.h b/xen/include/xen/spinlock.h
> index 53f0f72ac4..5ada9dce3d 100644
> --- a/xen/include/xen/spinlock.h
> +++ b/xen/include/xen/spinlock.h
> @@ -76,13 +76,19 @@ union lock_debug { };
> */
>
> struct spinlock;
> +/* Temporary hack until a dedicated struct rspinlock is existing. */
> +#define rspinlock spinlock
>
> struct lock_profile {
> struct lock_profile *next; /* forward link */
> const char *name; /* lock name */
> - struct spinlock *lock; /* the lock itself */
> + union {
> + struct spinlock *lock; /* the lock itself */
> + struct rspinlock *rlock; /* the recursive lock itself * > + };
> uint64_t lock_cnt; /* # of complete locking ops */
> - uint64_t block_cnt; /* # of complete wait for lock */
> + uint64_t block_cnt:63; /* # of complete wait for lock */
> + uint64_t is_rlock:1; /* use rlock pointer */
> s_time_t time_hold; /* cumulated lock time */
> s_time_t time_block; /* cumulated wait time */
> s_time_t time_locked; /* system time of last locking */
LGTM.
Acked-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
Cheers,
Alejandro
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v3 08/13] xen/spinlock: add explicit non-recursive locking functions
2023-11-20 11:38 [PATCH v3 00/13] xen/spinlock: make recursive spinlocks a dedicated type Juergen Gross
` (6 preceding siblings ...)
2023-11-20 11:38 ` [PATCH v3 07/13] xen/spinlock: make struct lock_profile rspinlock_t aware Juergen Gross
@ 2023-11-20 11:38 ` Juergen Gross
2023-11-20 11:38 ` [PATCH v3 09/13] xen/spinlock: add another function level Juergen Gross
` (4 subsequent siblings)
12 siblings, 0 replies; 23+ messages in thread
From: Juergen Gross @ 2023-11-20 11:38 UTC (permalink / raw)
To: xen-devel
Cc: javi.merino, Juergen Gross, Stefano Stabellini, Julien Grall,
Bertrand Marquis, Michal Orzel, Volodymyr Babchuk, Andrew Cooper,
George Dunlap, Jan Beulich, Wei Liu, Roger Pau Monné,
Tamas K Lengyel, Lukasz Hawrylko, Daniel P. Smith,
Mateusz Mówka
In order to prepare a type-safe recursive spinlock structure, add
explicitly non-recursive locking functions to be used for non-recursive
locking of spinlocks, which are used recursively, too.
Signed-off-by: Juergen Gross <jgross@suse.com>
---
V2:
- rename functions (Jan Beulich)
- get rid of !! in pcidevs_locked() (Jan Beulich)
---
xen/arch/arm/mm.c | 4 ++--
xen/arch/x86/domain.c | 12 ++++++------
xen/arch/x86/mm.c | 12 ++++++------
xen/arch/x86/mm/mem_sharing.c | 8 ++++----
xen/arch/x86/mm/p2m-pod.c | 4 ++--
xen/arch/x86/mm/p2m.c | 4 ++--
xen/arch/x86/tboot.c | 4 ++--
xen/common/domctl.c | 4 ++--
xen/common/grant_table.c | 10 +++++-----
xen/common/memory.c | 4 ++--
xen/common/numa.c | 4 ++--
xen/common/page_alloc.c | 16 ++++++++--------
xen/drivers/char/console.c | 16 ++++++++--------
xen/include/xen/spinlock.h | 24 +++++++++++++++++++-----
14 files changed, 70 insertions(+), 56 deletions(-)
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index c34cc94c90..c3a822b834 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -1317,7 +1317,7 @@ void share_xen_page_with_guest(struct page_info *page, struct domain *d,
if ( page_get_owner(page) == d )
return;
- spin_lock(&d->page_alloc_lock);
+ nrspin_lock(&d->page_alloc_lock);
/*
* The incremented type count pins as writable or read-only.
@@ -1348,7 +1348,7 @@ void share_xen_page_with_guest(struct page_info *page, struct domain *d,
page_list_add_tail(page, &d->xenpage_list);
}
- spin_unlock(&d->page_alloc_lock);
+ nrspin_unlock(&d->page_alloc_lock);
}
int xenmem_add_to_physmap_one(
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 69ce1fd5cf..998cb53a58 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -212,7 +212,7 @@ void dump_pageframe_info(struct domain *d)
{
unsigned long total[MASK_EXTR(PGT_type_mask, PGT_type_mask) + 1] = {};
- spin_lock(&d->page_alloc_lock);
+ nrspin_lock(&d->page_alloc_lock);
page_list_for_each ( page, &d->page_list )
{
unsigned int index = MASK_EXTR(page->u.inuse.type_info,
@@ -231,13 +231,13 @@ void dump_pageframe_info(struct domain *d)
_p(mfn_x(page_to_mfn(page))),
page->count_info, page->u.inuse.type_info);
}
- spin_unlock(&d->page_alloc_lock);
+ nrspin_unlock(&d->page_alloc_lock);
}
if ( is_hvm_domain(d) )
p2m_pod_dump_data(d);
- spin_lock(&d->page_alloc_lock);
+ nrspin_lock(&d->page_alloc_lock);
page_list_for_each ( page, &d->xenpage_list )
{
@@ -253,7 +253,7 @@ void dump_pageframe_info(struct domain *d)
page->count_info, page->u.inuse.type_info);
}
- spin_unlock(&d->page_alloc_lock);
+ nrspin_unlock(&d->page_alloc_lock);
}
void update_guest_memory_policy(struct vcpu *v,
@@ -2446,10 +2446,10 @@ int domain_relinquish_resources(struct domain *d)
d->arch.auto_unmask = 0;
}
- spin_lock(&d->page_alloc_lock);
+ nrspin_lock(&d->page_alloc_lock);
page_list_splice(&d->arch.relmem_list, &d->page_list);
INIT_PAGE_LIST_HEAD(&d->arch.relmem_list);
- spin_unlock(&d->page_alloc_lock);
+ nrspin_unlock(&d->page_alloc_lock);
PROGRESS(xen):
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 39544bd9f9..34682c1795 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -482,7 +482,7 @@ void share_xen_page_with_guest(struct page_info *page, struct domain *d,
set_gpfn_from_mfn(mfn_x(page_to_mfn(page)), INVALID_M2P_ENTRY);
- spin_lock(&d->page_alloc_lock);
+ nrspin_lock(&d->page_alloc_lock);
/* The incremented type count pins as writable or read-only. */
page->u.inuse.type_info =
@@ -502,7 +502,7 @@ void share_xen_page_with_guest(struct page_info *page, struct domain *d,
page_list_add_tail(page, &d->xenpage_list);
}
- spin_unlock(&d->page_alloc_lock);
+ nrspin_unlock(&d->page_alloc_lock);
}
void make_cr3(struct vcpu *v, mfn_t mfn)
@@ -3584,11 +3584,11 @@ long do_mmuext_op(
{
bool drop_ref;
- spin_lock(&pg_owner->page_alloc_lock);
+ nrspin_lock(&pg_owner->page_alloc_lock);
drop_ref = (pg_owner->is_dying &&
test_and_clear_bit(_PGT_pinned,
&page->u.inuse.type_info));
- spin_unlock(&pg_owner->page_alloc_lock);
+ nrspin_unlock(&pg_owner->page_alloc_lock);
if ( drop_ref )
{
pin_drop:
@@ -4411,7 +4411,7 @@ int steal_page(
* that it might be upon return from alloc_domheap_pages with
* MEMF_no_owner set.
*/
- spin_lock(&d->page_alloc_lock);
+ nrspin_lock(&d->page_alloc_lock);
BUG_ON(page->u.inuse.type_info & (PGT_count_mask | PGT_locked |
PGT_pinned));
@@ -4423,7 +4423,7 @@ int steal_page(
if ( !(memflags & MEMF_no_refcount) && !domain_adjust_tot_pages(d, -1) )
drop_dom_ref = true;
- spin_unlock(&d->page_alloc_lock);
+ nrspin_unlock(&d->page_alloc_lock);
if ( unlikely(drop_dom_ref) )
put_domain(d);
diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c
index 9585406095..d8268c32d7 100644
--- a/xen/arch/x86/mm/mem_sharing.c
+++ b/xen/arch/x86/mm/mem_sharing.c
@@ -746,11 +746,11 @@ static int page_make_private(struct domain *d, struct page_info *page)
if ( !get_page(page, dom_cow) )
return -EINVAL;
- spin_lock(&d->page_alloc_lock);
+ nrspin_lock(&d->page_alloc_lock);
if ( d->is_dying )
{
- spin_unlock(&d->page_alloc_lock);
+ nrspin_unlock(&d->page_alloc_lock);
put_page(page);
return -EBUSY;
}
@@ -758,7 +758,7 @@ static int page_make_private(struct domain *d, struct page_info *page)
expected_type = (PGT_shared_page | PGT_validated | PGT_locked | 2);
if ( page->u.inuse.type_info != expected_type )
{
- spin_unlock(&d->page_alloc_lock);
+ nrspin_unlock(&d->page_alloc_lock);
put_page(page);
return -EEXIST;
}
@@ -775,7 +775,7 @@ static int page_make_private(struct domain *d, struct page_info *page)
if ( domain_adjust_tot_pages(d, 1) == 1 )
get_knownalive_domain(d);
page_list_add_tail(page, &d->page_list);
- spin_unlock(&d->page_alloc_lock);
+ nrspin_unlock(&d->page_alloc_lock);
put_page(page);
diff --git a/xen/arch/x86/mm/p2m-pod.c b/xen/arch/x86/mm/p2m-pod.c
index 9969eb45fa..5361c2c5b1 100644
--- a/xen/arch/x86/mm/p2m-pod.c
+++ b/xen/arch/x86/mm/p2m-pod.c
@@ -27,7 +27,7 @@
static inline void lock_page_alloc(struct p2m_domain *p2m)
{
page_alloc_mm_pre_lock(p2m->domain);
- spin_lock(&(p2m->domain->page_alloc_lock));
+ nrspin_lock(&(p2m->domain->page_alloc_lock));
page_alloc_mm_post_lock(p2m->domain,
p2m->domain->arch.page_alloc_unlock_level);
}
@@ -35,7 +35,7 @@ static inline void lock_page_alloc(struct p2m_domain *p2m)
static inline void unlock_page_alloc(struct p2m_domain *p2m)
{
page_alloc_mm_unlock(p2m->domain->arch.page_alloc_unlock_level);
- spin_unlock(&(p2m->domain->page_alloc_lock));
+ nrspin_unlock(&(p2m->domain->page_alloc_lock));
}
/*
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 0983bd71d9..f78c46bda2 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -2225,7 +2225,7 @@ void audit_p2m(struct domain *d,
/* Audit part two: walk the domain's page allocation list, checking
* the m2p entries. */
- spin_lock(&d->page_alloc_lock);
+ nrspin_lock(&d->page_alloc_lock);
page_list_for_each ( page, &d->page_list )
{
mfn = mfn_x(page_to_mfn(page));
@@ -2277,7 +2277,7 @@ void audit_p2m(struct domain *d,
P2M_PRINTK("OK: mfn=%#lx, gfn=%#lx, p2mfn=%#lx\n",
mfn, gfn, mfn_x(p2mfn));
}
- spin_unlock(&d->page_alloc_lock);
+ nrspin_unlock(&d->page_alloc_lock);
pod_unlock(p2m);
p2m_unlock(p2m);
diff --git a/xen/arch/x86/tboot.c b/xen/arch/x86/tboot.c
index 86c4c22cac..5b33a1bf9d 100644
--- a/xen/arch/x86/tboot.c
+++ b/xen/arch/x86/tboot.c
@@ -205,14 +205,14 @@ static void tboot_gen_domain_integrity(const uint8_t key[TB_KEY_SIZE],
continue;
printk("MACing Domain %u\n", d->domain_id);
- spin_lock(&d->page_alloc_lock);
+ nrspin_lock(&d->page_alloc_lock);
page_list_for_each(page, &d->page_list)
{
void *pg = __map_domain_page(page);
vmac_update(pg, PAGE_SIZE, &ctx);
unmap_domain_page(pg);
}
- spin_unlock(&d->page_alloc_lock);
+ nrspin_unlock(&d->page_alloc_lock);
if ( is_iommu_enabled(d) && is_vtd )
{
diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index 505e29c0dc..3733f60f6a 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -621,14 +621,14 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
{
uint64_t new_max = op->u.max_mem.max_memkb >> (PAGE_SHIFT - 10);
- spin_lock(&d->page_alloc_lock);
+ nrspin_lock(&d->page_alloc_lock);
/*
* NB. We removed a check that new_max >= current tot_pages; this means
* that the domain will now be allowed to "ratchet" down to new_max. In
* the meantime, while tot > max, all new allocations are disallowed.
*/
d->max_pages = min(new_max, (uint64_t)(typeof(d->max_pages))-1);
- spin_unlock(&d->page_alloc_lock);
+ nrspin_unlock(&d->page_alloc_lock);
break;
}
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index 89b7811c51..8145974d3a 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -2376,7 +2376,7 @@ gnttab_transfer(
mfn = page_to_mfn(page);
}
- spin_lock(&e->page_alloc_lock);
+ nrspin_lock(&e->page_alloc_lock);
/*
* Check that 'e' will accept the page and has reservation
@@ -2387,7 +2387,7 @@ gnttab_transfer(
unlikely(domain_tot_pages(e) >= e->max_pages) ||
unlikely(!(e->tot_pages + 1)) )
{
- spin_unlock(&e->page_alloc_lock);
+ nrspin_unlock(&e->page_alloc_lock);
if ( e->is_dying )
gdprintk(XENLOG_INFO, "Transferee d%d is dying\n",
@@ -2411,7 +2411,7 @@ gnttab_transfer(
* safely drop the lock and re-aquire it later to add page to the
* pagelist.
*/
- spin_unlock(&e->page_alloc_lock);
+ nrspin_unlock(&e->page_alloc_lock);
okay = gnttab_prepare_for_transfer(e, d, gop.ref);
/*
@@ -2427,9 +2427,9 @@ gnttab_transfer(
* Need to grab this again to safely free our "reserved"
* page in the page total
*/
- spin_lock(&e->page_alloc_lock);
+ nrspin_lock(&e->page_alloc_lock);
drop_dom_ref = !domain_adjust_tot_pages(e, -1);
- spin_unlock(&e->page_alloc_lock);
+ nrspin_unlock(&e->page_alloc_lock);
if ( okay /* i.e. e->is_dying due to the surrounding if() */ )
gdprintk(XENLOG_INFO, "Transferee d%d is now dying\n",
diff --git a/xen/common/memory.c b/xen/common/memory.c
index fa165ebc14..f6a4dc17ee 100644
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -770,10 +770,10 @@ static long memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) arg)
(1UL << in_chunk_order)) -
(j * (1UL << exch.out.extent_order)));
- spin_lock(&d->page_alloc_lock);
+ nrspin_lock(&d->page_alloc_lock);
drop_dom_ref = (dec_count &&
!domain_adjust_tot_pages(d, -dec_count));
- spin_unlock(&d->page_alloc_lock);
+ nrspin_unlock(&d->page_alloc_lock);
if ( drop_dom_ref )
put_domain(d);
diff --git a/xen/common/numa.c b/xen/common/numa.c
index f454c4d894..47b1d0b5a8 100644
--- a/xen/common/numa.c
+++ b/xen/common/numa.c
@@ -718,13 +718,13 @@ static void cf_check dump_numa(unsigned char key)
memset(page_num_node, 0, sizeof(page_num_node));
- spin_lock(&d->page_alloc_lock);
+ nrspin_lock(&d->page_alloc_lock);
page_list_for_each ( page, &d->page_list )
{
i = page_to_nid(page);
page_num_node[i]++;
}
- spin_unlock(&d->page_alloc_lock);
+ nrspin_unlock(&d->page_alloc_lock);
for_each_online_node ( i )
printk(" Node %u: %u\n", i, page_num_node[i]);
diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index 8c6a3d9274..a25c00a7d4 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -515,7 +515,7 @@ int domain_set_outstanding_pages(struct domain *d, unsigned long pages)
* must always take the global heap_lock rather than only in the much
* rarer case that d->outstanding_pages is non-zero
*/
- spin_lock(&d->page_alloc_lock);
+ nrspin_lock(&d->page_alloc_lock);
spin_lock(&heap_lock);
/* pages==0 means "unset" the claim. */
@@ -561,7 +561,7 @@ int domain_set_outstanding_pages(struct domain *d, unsigned long pages)
out:
spin_unlock(&heap_lock);
- spin_unlock(&d->page_alloc_lock);
+ nrspin_unlock(&d->page_alloc_lock);
return ret;
}
@@ -2343,7 +2343,7 @@ int assign_pages(
int rc = 0;
unsigned int i;
- spin_lock(&d->page_alloc_lock);
+ nrspin_lock(&d->page_alloc_lock);
if ( unlikely(d->is_dying) )
{
@@ -2425,7 +2425,7 @@ int assign_pages(
}
out:
- spin_unlock(&d->page_alloc_lock);
+ nrspin_unlock(&d->page_alloc_lock);
return rc;
}
@@ -2906,9 +2906,9 @@ mfn_t acquire_reserved_page(struct domain *d, unsigned int memflags)
ASSERT_ALLOC_CONTEXT();
/* Acquire a page from reserved page list(resv_page_list). */
- spin_lock(&d->page_alloc_lock);
+ nrspin_lock(&d->page_alloc_lock);
page = page_list_remove_head(&d->resv_page_list);
- spin_unlock(&d->page_alloc_lock);
+ nrspin_unlock(&d->page_alloc_lock);
if ( unlikely(!page) )
return INVALID_MFN;
@@ -2927,9 +2927,9 @@ mfn_t acquire_reserved_page(struct domain *d, unsigned int memflags)
*/
unprepare_staticmem_pages(page, 1, false);
fail:
- spin_lock(&d->page_alloc_lock);
+ nrspin_lock(&d->page_alloc_lock);
page_list_add_tail(page, &d->resv_page_list);
- spin_unlock(&d->page_alloc_lock);
+ nrspin_unlock(&d->page_alloc_lock);
return INVALID_MFN;
}
#endif
diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
index cc743b67ec..c5609f32b5 100644
--- a/xen/drivers/char/console.c
+++ b/xen/drivers/char/console.c
@@ -369,9 +369,9 @@ long read_console_ring(struct xen_sysctl_readconsole *op)
if ( op->clear )
{
- spin_lock_irq(&console_lock);
+ nrspin_lock_irq(&console_lock);
conringc = p - c > conring_size ? p - conring_size : c;
- spin_unlock_irq(&console_lock);
+ nrspin_unlock_irq(&console_lock);
}
op->count = sofar;
@@ -639,7 +639,7 @@ static long guest_console_write(XEN_GUEST_HANDLE_PARAM(char) buffer,
if ( is_hardware_domain(cd) )
{
/* Use direct console output as it could be interactive */
- spin_lock_irq(&console_lock);
+ nrspin_lock_irq(&console_lock);
console_serial_puts(kbuf, kcount);
video_puts(kbuf, kcount);
@@ -660,7 +660,7 @@ static long guest_console_write(XEN_GUEST_HANDLE_PARAM(char) buffer,
tasklet_schedule(¬ify_dom0_con_ring_tasklet);
}
- spin_unlock_irq(&console_lock);
+ nrspin_unlock_irq(&console_lock);
}
else
{
@@ -1027,9 +1027,9 @@ void __init console_init_preirq(void)
pv_console_set_rx_handler(serial_rx);
/* HELLO WORLD --- start-of-day banner text. */
- spin_lock(&console_lock);
+ nrspin_lock(&console_lock);
__putstr(xen_banner());
- spin_unlock(&console_lock);
+ nrspin_unlock(&console_lock);
printk("Xen version %d.%d%s (%s@%s) (%s) %s %s\n",
xen_major_version(), xen_minor_version(), xen_extra_version(),
xen_compile_by(), xen_compile_domain(), xen_compiler(),
@@ -1066,13 +1066,13 @@ void __init console_init_ring(void)
}
opt_conring_size = PAGE_SIZE << order;
- spin_lock_irqsave(&console_lock, flags);
+ nrspin_lock_irqsave(&console_lock, flags);
for ( i = conringc ; i != conringp; i++ )
ring[i & (opt_conring_size - 1)] = conring[i & (conring_size - 1)];
conring = ring;
smp_wmb(); /* Allow users of console_force_unlock() to see larger buffer. */
conring_size = opt_conring_size;
- spin_unlock_irqrestore(&console_lock, flags);
+ nrspin_unlock_irqrestore(&console_lock, flags);
printk("Allocated console ring of %u KiB.\n", opt_conring_size >> 10);
}
diff --git a/xen/include/xen/spinlock.h b/xen/include/xen/spinlock.h
index 5ada9dce3d..12764bcddf 100644
--- a/xen/include/xen/spinlock.h
+++ b/xen/include/xen/spinlock.h
@@ -101,6 +101,8 @@ struct lock_profile_qhead {
};
#define _LOCK_PROFILE(lockname) { .name = #lockname, .lock = &lockname, }
+#define _RLOCK_PROFILE(lockname) { .name = #lockname, .rlock = &lockname, \
+ .is_rlock = 1, }
#define _LOCK_PROFILE_PTR(name) \
static struct lock_profile * const __lock_profile_##name \
__used_section(".lockprofile.data") = \
@@ -117,10 +119,10 @@ struct lock_profile_qhead {
_LOCK_PROFILE_PTR(l)
#define DEFINE_RSPINLOCK(l) \
rspinlock_t l = _SPIN_LOCK_UNLOCKED(NULL); \
- static struct lock_profile __lock_profile_data_##l = _LOCK_PROFILE(l); \
+ static struct lock_profile __lock_profile_data_##l = _RLOCK_PROFILE(l); \
_LOCK_PROFILE_PTR(l)
-#define __spin_lock_init_prof(s, l, locktype) \
+#define __spin_lock_init_prof(s, l, lockptr, locktype, isr) \
do { \
struct lock_profile *prof; \
prof = xzalloc(struct lock_profile); \
@@ -133,13 +135,16 @@ struct lock_profile_qhead {
break; \
} \
prof->name = #l; \
- prof->lock = &(s)->l; \
+ prof->lockptr = &(s)->l; \
+ prof->is_rlock = isr; \
prof->next = (s)->profile_head.elem_q; \
(s)->profile_head.elem_q = prof; \
} while( 0 )
-#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
+
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)
+
#endif /* __SPINLOCK_H__ */
--
2.35.3
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH v3 09/13] xen/spinlock: add another function level
2023-11-20 11:38 [PATCH v3 00/13] xen/spinlock: make recursive spinlocks a dedicated type Juergen Gross
` (7 preceding siblings ...)
2023-11-20 11:38 ` [PATCH v3 08/13] xen/spinlock: add explicit non-recursive locking functions Juergen Gross
@ 2023-11-20 11:38 ` Juergen Gross
2023-11-20 11:38 ` [PATCH v3 10/13] xen/spinlock: add missing rspin_is_locked() and rspin_barrier() Juergen Gross
` (3 subsequent siblings)
12 siblings, 0 replies; 23+ messages in thread
From: Juergen Gross @ 2023-11-20 11:38 UTC (permalink / raw)
To: xen-devel
Cc: javi.merino, Juergen Gross, Andrew Cooper, George Dunlap,
Jan Beulich, Julien Grall, Stefano Stabellini, Wei Liu
Add another function level in spinlock.c hiding the spinlock_t layout
from the low level locking code.
This is done in preparation of introducing rspinlock_t for recursive
locks without having to duplicate all of the locking code.
Signed-off-by: Juergen Gross <jgross@suse.com>
---
V2:
- new patch
---
xen/common/spinlock.c | 104 +++++++++++++++++++++++--------------
xen/include/xen/spinlock.h | 1 +
2 files changed, 65 insertions(+), 40 deletions(-)
diff --git a/xen/common/spinlock.c b/xen/common/spinlock.c
index 65f180203a..8b991cf385 100644
--- a/xen/common/spinlock.c
+++ b/xen/common/spinlock.c
@@ -261,29 +261,31 @@ void spin_debug_disable(void)
#ifdef CONFIG_DEBUG_LOCK_PROFILE
+#define LOCK_PROFILE_PAR lock->profile
#define LOCK_PROFILE_REL \
- if ( lock->profile ) \
+ if ( profile ) \
{ \
- lock->profile->time_hold += NOW() - lock->profile->time_locked; \
- lock->profile->lock_cnt++; \
+ profile->time_hold += NOW() - profile->time_locked; \
+ profile->lock_cnt++; \
}
#define LOCK_PROFILE_VAR(var, val) s_time_t var = (val)
#define LOCK_PROFILE_BLOCK(var ) var = var ? : NOW()
#define LOCK_PROFILE_BLKACC(tst, val) \
if ( tst ) \
{ \
- lock->profile->time_block += lock->profile->time_locked - (val); \
- lock->profile->block_cnt++; \
+ profile->time_block += profile->time_locked - (val); \
+ profile->block_cnt++; \
}
#define LOCK_PROFILE_GOT(val) \
- if ( lock->profile ) \
+ if ( profile ) \
{ \
- lock->profile->time_locked = NOW(); \
+ profile->time_locked = NOW(); \
LOCK_PROFILE_BLKACC(val, val); \
}
#else
+#define LOCK_PROFILE_PAR NULL
#define LOCK_PROFILE_REL
#define LOCK_PROFILE_VAR(var, val)
#define LOCK_PROFILE_BLOCK(var)
@@ -307,17 +309,18 @@ static always_inline uint16_t observe_head(const spinlock_tickets_t *t)
return read_atomic(&t->head);
}
-static void always_inline spin_lock_common(spinlock_t *lock,
+static void always_inline spin_lock_common(spinlock_tickets_t *t,
+ union lock_debug *debug,
+ struct lock_profile *profile,
void (*cb)(void *), void *data)
{
spinlock_tickets_t tickets = SPINLOCK_TICKET_INC;
LOCK_PROFILE_VAR(block, 0);
- check_lock(&lock->debug, false);
+ check_lock(debug, false);
preempt_disable();
- tickets.head_tail = arch_fetch_and_add(&lock->tickets.head_tail,
- tickets.head_tail);
- while ( tickets.tail != observe_head(&lock->tickets) )
+ tickets.head_tail = arch_fetch_and_add(&t->head_tail, tickets.head_tail);
+ while ( tickets.tail != observe_head(t) )
{
LOCK_PROFILE_BLOCK(block);
if ( cb )
@@ -325,18 +328,19 @@ static void always_inline spin_lock_common(spinlock_t *lock,
arch_lock_relax();
}
arch_lock_acquire_barrier();
- got_lock(&lock->debug);
+ got_lock(debug);
LOCK_PROFILE_GOT(block);
}
void _spin_lock(spinlock_t *lock)
{
- spin_lock_common(lock, NULL, NULL);
+ spin_lock_common(&lock->tickets, &lock->debug, LOCK_PROFILE_PAR, NULL,
+ NULL);
}
void _spin_lock_cb(spinlock_t *lock, void (*cb)(void *), void *data)
{
- spin_lock_common(lock, cb, data);
+ spin_lock_common(&lock->tickets, &lock->debug, LOCK_PROFILE_PAR, cb, data);
}
void _spin_lock_irq(spinlock_t *lock)
@@ -355,16 +359,23 @@ unsigned long _spin_lock_irqsave(spinlock_t *lock)
return flags;
}
-void _spin_unlock(spinlock_t *lock)
+static void always_inline spin_unlock_common(spinlock_tickets_t *t,
+ union lock_debug *debug,
+ struct lock_profile *profile)
{
LOCK_PROFILE_REL;
- rel_lock(&lock->debug);
+ rel_lock(debug);
arch_lock_release_barrier();
- add_sized(&lock->tickets.head, 1);
+ add_sized(&t->head, 1);
arch_lock_signal();
preempt_enable();
}
+void _spin_unlock(spinlock_t *lock)
+{
+ spin_unlock_common(&lock->tickets, &lock->debug, LOCK_PROFILE_PAR);
+}
+
void _spin_unlock_irq(spinlock_t *lock)
{
_spin_unlock(lock);
@@ -377,25 +388,25 @@ void _spin_unlock_irqrestore(spinlock_t *lock, unsigned long flags)
local_irq_restore(flags);
}
+static int always_inline spin_is_locked_common(const spinlock_tickets_t *t)
+{
+ return t->head != t->tail;
+}
+
int _spin_is_locked(const spinlock_t *lock)
{
- /*
- * Recursive locks may be locked by another CPU, yet we return
- * "false" here, making this function suitable only for use in
- * ASSERT()s and alike.
- */
- return lock->recurse_cpu == SPINLOCK_NO_CPU
- ? lock->tickets.head != lock->tickets.tail
- : lock->recurse_cpu == smp_processor_id();
+ return spin_is_locked_common(&lock->tickets);
}
-int _spin_trylock(spinlock_t *lock)
+static int always_inline spin_trylock_common(spinlock_tickets_t *t,
+ union lock_debug *debug,
+ struct lock_profile *profile)
{
spinlock_tickets_t old, new;
preempt_disable();
- check_lock(&lock->debug, true);
- old = observe_lock(&lock->tickets);
+ check_lock(debug, true);
+ old = observe_lock(t);
if ( old.head != old.tail )
{
preempt_enable();
@@ -403,8 +414,7 @@ int _spin_trylock(spinlock_t *lock)
}
new = old;
new.tail++;
- if ( cmpxchg(&lock->tickets.head_tail,
- old.head_tail, new.head_tail) != old.head_tail )
+ if ( cmpxchg(&t->head_tail, old.head_tail, new.head_tail) != old.head_tail )
{
preempt_enable();
return 0;
@@ -413,29 +423,41 @@ int _spin_trylock(spinlock_t *lock)
* cmpxchg() is a full barrier so no need for an
* arch_lock_acquire_barrier().
*/
- got_lock(&lock->debug);
+ got_lock(debug);
LOCK_PROFILE_GOT(0);
return 1;
}
-void _spin_barrier(spinlock_t *lock)
+int _spin_trylock(spinlock_t *lock)
+{
+ return spin_trylock_common(&lock->tickets, &lock->debug, LOCK_PROFILE_PAR);
+}
+
+static void always_inline spin_barrier_common(spinlock_tickets_t *t,
+ union lock_debug *debug,
+ struct lock_profile *profile)
{
spinlock_tickets_t sample;
LOCK_PROFILE_VAR(block, NOW());
- check_barrier(&lock->debug);
+ check_barrier(debug);
smp_mb();
- sample = observe_lock(&lock->tickets);
+ sample = observe_lock(t);
if ( sample.head != sample.tail )
{
- while ( observe_head(&lock->tickets) == sample.head )
+ while ( observe_head(t) == sample.head )
arch_lock_relax();
- LOCK_PROFILE_BLKACC(lock->profile, block);
+ LOCK_PROFILE_BLKACC(profile, block);
}
smp_mb();
}
+void _spin_barrier(spinlock_t *lock)
+{
+ spin_barrier_common(&lock->tickets, &lock->debug, LOCK_PROFILE_PAR);
+}
+
int rspin_trylock(rspinlock_t *lock)
{
unsigned int cpu = smp_processor_id();
@@ -448,7 +470,8 @@ int rspin_trylock(rspinlock_t *lock)
if ( likely(lock->recurse_cpu != cpu) )
{
- if ( !spin_trylock(lock) )
+ if ( !spin_trylock_common(&lock->tickets, &lock->debug,
+ LOCK_PROFILE_PAR) )
return 0;
lock->recurse_cpu = cpu;
}
@@ -466,7 +489,8 @@ void rspin_lock(rspinlock_t *lock)
if ( likely(lock->recurse_cpu != cpu) )
{
- _spin_lock(lock);
+ spin_lock_common(&lock->tickets, &lock->debug, LOCK_PROFILE_PAR, NULL,
+ NULL);
lock->recurse_cpu = cpu;
}
@@ -490,7 +514,7 @@ void rspin_unlock(rspinlock_t *lock)
if ( likely(--lock->recurse_cnt == 0) )
{
lock->recurse_cpu = SPINLOCK_NO_CPU;
- spin_unlock(lock);
+ spin_unlock_common(&lock->tickets, &lock->debug, LOCK_PROFILE_PAR);
}
}
diff --git a/xen/include/xen/spinlock.h b/xen/include/xen/spinlock.h
index 12764bcddf..ccb1cafa5f 100644
--- a/xen/include/xen/spinlock.h
+++ b/xen/include/xen/spinlock.h
@@ -163,6 +163,7 @@ extern void cf_check spinlock_profile_reset(unsigned char key);
#else
struct lock_profile_qhead { };
+struct lock_profile { };
#define SPIN_LOCK_UNLOCKED { \
.recurse_cpu = SPINLOCK_NO_CPU, \
--
2.35.3
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH v3 10/13] xen/spinlock: add missing rspin_is_locked() and rspin_barrier()
2023-11-20 11:38 [PATCH v3 00/13] xen/spinlock: make recursive spinlocks a dedicated type Juergen Gross
` (8 preceding siblings ...)
2023-11-20 11:38 ` [PATCH v3 09/13] xen/spinlock: add another function level Juergen Gross
@ 2023-11-20 11:38 ` Juergen Gross
2023-11-20 11:38 ` [PATCH v3 11/13] xen/spinlock: split recursive spinlocks from normal ones Juergen Gross
` (2 subsequent siblings)
12 siblings, 0 replies; 23+ messages in thread
From: Juergen Gross @ 2023-11-20 11:38 UTC (permalink / raw)
To: xen-devel
Cc: javi.merino, Juergen Gross, Jan Beulich, Andrew Cooper,
George Dunlap, Roger Pau Monné, Wei Liu, Julien Grall,
Stefano Stabellini, Paul Durrant
Add rspin_is_locked() and rspin_barrier() in order to prepare differing
spinlock_t and rspinlock_t types.
Signed-off-by: Juergen Gross <jgross@suse.com>
---
V2:
- partially carved out from V1 patch, partially new
---
xen/arch/x86/mm/p2m-pod.c | 2 +-
xen/common/domain.c | 2 +-
xen/common/page_alloc.c | 2 +-
xen/common/spinlock.c | 17 +++++++++++++++++
xen/drivers/char/console.c | 4 ++--
xen/drivers/passthrough/pci.c | 2 +-
xen/include/xen/spinlock.h | 2 ++
7 files changed, 25 insertions(+), 6 deletions(-)
diff --git a/xen/arch/x86/mm/p2m-pod.c b/xen/arch/x86/mm/p2m-pod.c
index 5361c2c5b1..ad49c64ead 100644
--- a/xen/arch/x86/mm/p2m-pod.c
+++ b/xen/arch/x86/mm/p2m-pod.c
@@ -385,7 +385,7 @@ int p2m_pod_empty_cache(struct domain *d)
/* After this barrier no new PoD activities can happen. */
BUG_ON(!d->is_dying);
- spin_barrier(&p2m->pod.lock.lock);
+ rspin_barrier(&p2m->pod.lock.lock);
lock_page_alloc(p2m);
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 604f70ff5a..17da8e0746 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -974,7 +974,7 @@ int domain_kill(struct domain *d)
case DOMDYING_alive:
domain_pause(d);
d->is_dying = DOMDYING_dying;
- spin_barrier(&d->domain_lock);
+ rspin_barrier(&d->domain_lock);
argo_destroy(d);
vnuma_destroy(d->vnuma);
domain_set_outstanding_pages(d, 0);
diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index a25c00a7d4..14010b6fa5 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -476,7 +476,7 @@ unsigned long domain_adjust_tot_pages(struct domain *d, long pages)
{
long dom_before, dom_after, dom_claimed, sys_before, sys_after;
- ASSERT(spin_is_locked(&d->page_alloc_lock));
+ ASSERT(rspin_is_locked(&d->page_alloc_lock));
d->tot_pages += pages;
/*
diff --git a/xen/common/spinlock.c b/xen/common/spinlock.c
index 8b991cf385..8bb6691b14 100644
--- a/xen/common/spinlock.c
+++ b/xen/common/spinlock.c
@@ -458,6 +458,23 @@ void _spin_barrier(spinlock_t *lock)
spin_barrier_common(&lock->tickets, &lock->debug, LOCK_PROFILE_PAR);
}
+int rspin_is_locked(const rspinlock_t *lock)
+{
+ /*
+ * Recursive locks may be locked by another CPU, yet we return
+ * "false" here, making this function suitable only for use in
+ * ASSERT()s and alike.
+ */
+ return lock->recurse_cpu == SPINLOCK_NO_CPU
+ ? spin_is_locked_common(&lock->tickets)
+ : lock->recurse_cpu == smp_processor_id();
+}
+
+void rspin_barrier(rspinlock_t *lock)
+{
+ spin_barrier_common(&lock->tickets, &lock->debug, LOCK_PROFILE_PAR);
+}
+
int rspin_trylock(rspinlock_t *lock)
{
unsigned int cpu = smp_processor_id();
diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
index c5609f32b5..a1aca2b60b 100644
--- a/xen/drivers/char/console.c
+++ b/xen/drivers/char/console.c
@@ -328,7 +328,7 @@ static void cf_check do_dec_thresh(unsigned char key, struct cpu_user_regs *regs
static void conring_puts(const char *str, size_t len)
{
- ASSERT(spin_is_locked(&console_lock));
+ ASSERT(rspin_is_locked(&console_lock));
while ( len-- )
conring[CONRING_IDX_MASK(conringp++)] = *str++;
@@ -766,7 +766,7 @@ static void __putstr(const char *str)
{
size_t len = strlen(str);
- ASSERT(spin_is_locked(&console_lock));
+ ASSERT(rspin_is_locked(&console_lock));
console_serial_puts(str, len);
video_puts(str, len);
diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index 22342f07ac..f55c1a5cf4 100644
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -64,7 +64,7 @@ void pcidevs_unlock(void)
bool pcidevs_locked(void)
{
- return !!spin_is_locked(&_pcidevs_lock);
+ return rspin_is_locked(&_pcidevs_lock);
}
static struct radix_tree_root pci_segments;
diff --git a/xen/include/xen/spinlock.h b/xen/include/xen/spinlock.h
index ccb1cafa5f..fb6ca1949a 100644
--- a/xen/include/xen/spinlock.h
+++ b/xen/include/xen/spinlock.h
@@ -239,6 +239,8 @@ void rspin_lock(rspinlock_t *lock);
unsigned long __rspin_lock_irqsave(rspinlock_t *lock);
void rspin_unlock(rspinlock_t *lock);
void rspin_unlock_irqrestore(rspinlock_t *lock, unsigned long flags);
+int rspin_is_locked(const rspinlock_t *lock);
+void rspin_barrier(rspinlock_t *lock);
#define spin_lock(l) _spin_lock(l)
#define spin_lock_cb(l, c, d) _spin_lock_cb(l, c, d)
--
2.35.3
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH v3 11/13] xen/spinlock: split recursive spinlocks from normal ones
2023-11-20 11:38 [PATCH v3 00/13] xen/spinlock: make recursive spinlocks a dedicated type Juergen Gross
` (9 preceding siblings ...)
2023-11-20 11:38 ` [PATCH v3 10/13] xen/spinlock: add missing rspin_is_locked() and rspin_barrier() Juergen Gross
@ 2023-11-20 11:38 ` Juergen Gross
2023-11-20 11:38 ` [PATCH v3 12/13] xen/spinlock: remove indirection through macros for spin_*() functions Juergen Gross
2023-11-20 11:38 ` [PATCH v3 13/13] xen/spinlock: support higher number of cpus Juergen Gross
12 siblings, 0 replies; 23+ messages in thread
From: Juergen Gross @ 2023-11-20 11:38 UTC (permalink / raw)
To: xen-devel
Cc: javi.merino, Juergen Gross, Andrew Cooper, George Dunlap,
Jan Beulich, Julien Grall, Stefano Stabellini, Wei Liu
Recursive and normal spinlocks are sharing the same data structure for
representation of the lock. This has two major disadvantages:
- it is not clear from the definition of a lock, whether it is intended
to be used recursive or not, while a mixture of both usage variants
needs to be
- in production builds (builds without CONFIG_DEBUG_LOCKS) the needed
data size of an ordinary spinlock is 8 bytes instead of 4, due to the
additional recursion data needed (associated with that the rwlock
data is using 12 instead of only 8 bytes)
Fix that by introducing a struct spinlock_recursive for recursive
spinlocks only, and switch recursive spinlock functions to require
pointers to this new struct.
This allows to check the correct usage at build time.
Signed-off-by: Juergen Gross <jgross@suse.com>
---
V2:
- use shorter names (Jan Beulich)
- don't embed spinlock_t in rspinlock_t (Jan Beulich)
---
xen/common/spinlock.c | 49 ++++++++++++++++++++++++++++++++
xen/include/xen/spinlock.h | 58 +++++++++++++++++++++++++-------------
2 files changed, 88 insertions(+), 19 deletions(-)
diff --git a/xen/common/spinlock.c b/xen/common/spinlock.c
index 8bb6691b14..0467541f43 100644
--- a/xen/common/spinlock.c
+++ b/xen/common/spinlock.c
@@ -541,6 +541,55 @@ void rspin_unlock_irqrestore(rspinlock_t *lock, unsigned long flags)
local_irq_restore(flags);
}
+int nrspin_trylock(rspinlock_t *lock)
+{
+ check_lock(&lock->debug, true);
+
+ if ( unlikely(lock->recurse_cpu != SPINLOCK_NO_CPU) )
+ return 0;
+
+ return spin_trylock_common(&lock->tickets, &lock->debug, LOCK_PROFILE_PAR);
+}
+
+void nrspin_lock(rspinlock_t *lock)
+{
+ spin_lock_common(&lock->tickets, &lock->debug, LOCK_PROFILE_PAR, NULL,
+ NULL);
+}
+
+void nrspin_unlock(rspinlock_t *lock)
+{
+ spin_unlock_common(&lock->tickets, &lock->debug, LOCK_PROFILE_PAR);
+}
+
+void nrspin_lock_irq(rspinlock_t *lock)
+{
+ ASSERT(local_irq_is_enabled());
+ local_irq_disable();
+ nrspin_lock(lock);
+}
+
+void nrspin_unlock_irq(rspinlock_t *lock)
+{
+ nrspin_unlock(lock);
+ local_irq_enable();
+}
+
+unsigned long __nrspin_lock_irqsave(rspinlock_t *lock)
+{
+ unsigned long flags;
+
+ local_irq_save(flags);
+ nrspin_lock(lock);
+ return flags;
+}
+
+void nrspin_unlock_irqrestore(rspinlock_t *lock, unsigned long flags)
+{
+ nrspin_unlock(lock);
+ local_irq_restore(flags);
+}
+
#ifdef CONFIG_DEBUG_LOCK_PROFILE
struct lock_profile_anc {
diff --git a/xen/include/xen/spinlock.h b/xen/include/xen/spinlock.h
index fb6ca1949a..a2b1bb2df6 100644
--- a/xen/include/xen/spinlock.h
+++ b/xen/include/xen/spinlock.h
@@ -76,8 +76,6 @@ union lock_debug { };
*/
struct spinlock;
-/* Temporary hack until a dedicated struct rspinlock is existing. */
-#define rspinlock spinlock
struct lock_profile {
struct lock_profile *next; /* forward link */
@@ -108,6 +106,10 @@ struct lock_profile_qhead {
__used_section(".lockprofile.data") = \
&__lock_profile_data_##name
#define _SPIN_LOCK_UNLOCKED(x) { \
+ .debug =_LOCK_DEBUG, \
+ .profile = x, \
+}
+#define _RSPIN_LOCK_UNLOCKED(x) { \
.recurse_cpu = SPINLOCK_NO_CPU, \
.debug =_LOCK_DEBUG, \
.profile = x, \
@@ -117,8 +119,9 @@ struct lock_profile_qhead {
spinlock_t l = _SPIN_LOCK_UNLOCKED(NULL); \
static struct lock_profile __lock_profile_data_##l = _LOCK_PROFILE(l); \
_LOCK_PROFILE_PTR(l)
+#define RSPIN_LOCK_UNLOCKED _RSPIN_LOCK_UNLOCKED(NULL)
#define DEFINE_RSPINLOCK(l) \
- rspinlock_t l = _SPIN_LOCK_UNLOCKED(NULL); \
+ rspinlock_t l = _RSPIN_LOCK_UNLOCKED(NULL); \
static struct lock_profile __lock_profile_data_##l = _RLOCK_PROFILE(l); \
_LOCK_PROFILE_PTR(l)
@@ -143,8 +146,11 @@ struct lock_profile_qhead {
#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)
+#define rspin_lock_init_prof(s, l) do { \
+ __spin_lock_init_prof(s, l, rlock, rspinlock_t, 1); \
+ (s)->l.recurse_cpu = SPINLOCK_NO_CPU; \
+ (s)->l.recurse_cnt = 0; \
+ } while (0)
void _lock_profile_register_struct(
int32_t type, struct lock_profile_qhead *qhead, int32_t idx);
@@ -166,11 +172,15 @@ struct lock_profile_qhead { };
struct lock_profile { };
#define SPIN_LOCK_UNLOCKED { \
+ .debug =_LOCK_DEBUG, \
+}
+#define RSPIN_LOCK_UNLOCKED { \
+ .debug =_LOCK_DEBUG, \
.recurse_cpu = SPINLOCK_NO_CPU, \
.debug =_LOCK_DEBUG, \
}
#define DEFINE_SPINLOCK(l) spinlock_t l = SPIN_LOCK_UNLOCKED
-#define DEFINE_RSPINLOCK(l) rspinlock_t l = SPIN_LOCK_UNLOCKED
+#define DEFINE_RSPINLOCK(l) rspinlock_t l = RSPIN_LOCK_UNLOCKED
#define spin_lock_init_prof(s, l) spin_lock_init(&((s)->l))
#define rspin_lock_init_prof(s, l) rspin_lock_init(&((s)->l))
@@ -180,7 +190,6 @@ struct lock_profile { };
#endif
-
typedef union {
uint32_t head_tail;
struct {
@@ -192,6 +201,14 @@ typedef union {
#define SPINLOCK_TICKET_INC { .head_tail = 0x10000, }
typedef struct spinlock {
+ spinlock_tickets_t tickets;
+ union lock_debug debug;
+#ifdef CONFIG_DEBUG_LOCK_PROFILE
+ struct lock_profile *profile;
+#endif
+} spinlock_t;
+
+typedef struct rspinlock {
spinlock_tickets_t tickets;
uint16_t recurse_cpu:SPINLOCK_CPU_BITS;
#define SPINLOCK_NO_CPU ((1u << SPINLOCK_CPU_BITS) - 1)
@@ -202,12 +219,10 @@ typedef struct spinlock {
#ifdef CONFIG_DEBUG_LOCK_PROFILE
struct lock_profile *profile;
#endif
-} spinlock_t;
-
-typedef spinlock_t rspinlock_t;
+} rspinlock_t;
#define spin_lock_init(l) (*(l) = (spinlock_t)SPIN_LOCK_UNLOCKED)
-#define rspin_lock_init(l) (*(l) = (rspinlock_t)SPIN_LOCK_UNLOCKED)
+#define rspin_lock_init(l) (*(l) = (rspinlock_t)RSPIN_LOCK_UNLOCKED)
void _spin_lock(spinlock_t *lock);
void _spin_lock_cb(spinlock_t *lock, void (*cb)(void *), void *data);
@@ -242,6 +257,19 @@ void rspin_unlock_irqrestore(rspinlock_t *lock, unsigned long flags);
int rspin_is_locked(const rspinlock_t *lock);
void rspin_barrier(rspinlock_t *lock);
+int nrspin_trylock(rspinlock_t *lock);
+void nrspin_lock(rspinlock_t *lock);
+void nrspin_unlock(rspinlock_t *lock);
+void nrspin_lock_irq(rspinlock_t *lock);
+void nrspin_unlock_irq(rspinlock_t *lock);
+#define nrspin_lock_irqsave(l, f) \
+ ({ \
+ BUILD_BUG_ON(sizeof(f) != sizeof(unsigned long)); \
+ ((f) = __nrspin_lock_irqsave(l)); \
+ })
+unsigned long __nrspin_lock_irqsave(rspinlock_t *lock);
+void nrspin_unlock_irqrestore(rspinlock_t *lock, unsigned long flags);
+
#define spin_lock(l) _spin_lock(l)
#define spin_lock_cb(l, c, d) _spin_lock_cb(l, c, d)
#define spin_lock_irq(l) _spin_lock_irq(l)
@@ -270,12 +298,4 @@ void rspin_barrier(rspinlock_t *lock);
/* 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)
-
#endif /* __SPINLOCK_H__ */
--
2.35.3
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH v3 12/13] xen/spinlock: remove indirection through macros for spin_*() functions
2023-11-20 11:38 [PATCH v3 00/13] xen/spinlock: make recursive spinlocks a dedicated type Juergen Gross
` (10 preceding siblings ...)
2023-11-20 11:38 ` [PATCH v3 11/13] xen/spinlock: split recursive spinlocks from normal ones Juergen Gross
@ 2023-11-20 11:38 ` Juergen Gross
2023-11-20 11:38 ` [PATCH v3 13/13] xen/spinlock: support higher number of cpus Juergen Gross
12 siblings, 0 replies; 23+ messages in thread
From: Juergen Gross @ 2023-11-20 11:38 UTC (permalink / raw)
To: xen-devel
Cc: javi.merino, Juergen Gross, Andrew Cooper, George Dunlap,
Jan Beulich, Julien Grall, Stefano Stabellini, Wei Liu
In reality all spin_*() functions are macros which are defined to just
call a related real function.
Remove this macro layer, as it is adding complexity without any gain.
Signed-off-by: Juergen Gross <jgross@suse.com>
---
V2:
- new patch
---
xen/common/spinlock.c | 28 +++++++++---------
xen/include/xen/spinlock.h | 58 +++++++++++++++-----------------------
2 files changed, 36 insertions(+), 50 deletions(-)
diff --git a/xen/common/spinlock.c b/xen/common/spinlock.c
index 0467541f43..43c876b64a 100644
--- a/xen/common/spinlock.c
+++ b/xen/common/spinlock.c
@@ -332,30 +332,30 @@ static void always_inline spin_lock_common(spinlock_tickets_t *t,
LOCK_PROFILE_GOT(block);
}
-void _spin_lock(spinlock_t *lock)
+void spin_lock(spinlock_t *lock)
{
spin_lock_common(&lock->tickets, &lock->debug, LOCK_PROFILE_PAR, NULL,
NULL);
}
-void _spin_lock_cb(spinlock_t *lock, void (*cb)(void *), void *data)
+void spin_lock_cb(spinlock_t *lock, void (*cb)(void *), void *data)
{
spin_lock_common(&lock->tickets, &lock->debug, LOCK_PROFILE_PAR, cb, data);
}
-void _spin_lock_irq(spinlock_t *lock)
+void spin_lock_irq(spinlock_t *lock)
{
ASSERT(local_irq_is_enabled());
local_irq_disable();
- _spin_lock(lock);
+ spin_lock(lock);
}
-unsigned long _spin_lock_irqsave(spinlock_t *lock)
+unsigned long __spin_lock_irqsave(spinlock_t *lock)
{
unsigned long flags;
local_irq_save(flags);
- _spin_lock(lock);
+ spin_lock(lock);
return flags;
}
@@ -371,20 +371,20 @@ static void always_inline spin_unlock_common(spinlock_tickets_t *t,
preempt_enable();
}
-void _spin_unlock(spinlock_t *lock)
+void spin_unlock(spinlock_t *lock)
{
spin_unlock_common(&lock->tickets, &lock->debug, LOCK_PROFILE_PAR);
}
-void _spin_unlock_irq(spinlock_t *lock)
+void spin_unlock_irq(spinlock_t *lock)
{
- _spin_unlock(lock);
+ spin_unlock(lock);
local_irq_enable();
}
-void _spin_unlock_irqrestore(spinlock_t *lock, unsigned long flags)
+void spin_unlock_irqrestore(spinlock_t *lock, unsigned long flags)
{
- _spin_unlock(lock);
+ spin_unlock(lock);
local_irq_restore(flags);
}
@@ -393,7 +393,7 @@ static int always_inline spin_is_locked_common(const spinlock_tickets_t *t)
return t->head != t->tail;
}
-int _spin_is_locked(const spinlock_t *lock)
+int spin_is_locked(const spinlock_t *lock)
{
return spin_is_locked_common(&lock->tickets);
}
@@ -429,7 +429,7 @@ static int always_inline spin_trylock_common(spinlock_tickets_t *t,
return 1;
}
-int _spin_trylock(spinlock_t *lock)
+int spin_trylock(spinlock_t *lock)
{
return spin_trylock_common(&lock->tickets, &lock->debug, LOCK_PROFILE_PAR);
}
@@ -453,7 +453,7 @@ static void always_inline spin_barrier_common(spinlock_tickets_t *t,
smp_mb();
}
-void _spin_barrier(spinlock_t *lock)
+void spin_barrier(spinlock_t *lock)
{
spin_barrier_common(&lock->tickets, &lock->debug, LOCK_PROFILE_PAR);
}
diff --git a/xen/include/xen/spinlock.h b/xen/include/xen/spinlock.h
index a2b1bb2df6..e4f2e36e4f 100644
--- a/xen/include/xen/spinlock.h
+++ b/xen/include/xen/spinlock.h
@@ -224,18 +224,30 @@ typedef struct rspinlock {
#define spin_lock_init(l) (*(l) = (spinlock_t)SPIN_LOCK_UNLOCKED)
#define rspin_lock_init(l) (*(l) = (rspinlock_t)RSPIN_LOCK_UNLOCKED)
-void _spin_lock(spinlock_t *lock);
-void _spin_lock_cb(spinlock_t *lock, void (*cb)(void *), void *data);
-void _spin_lock_irq(spinlock_t *lock);
-unsigned long _spin_lock_irqsave(spinlock_t *lock);
+void spin_lock(spinlock_t *lock);
+void spin_lock_cb(spinlock_t *lock, void (*cb)(void *), void *data);
+void spin_lock_irq(spinlock_t *lock);
+#define spin_lock_irqsave(l, f) \
+ ({ \
+ BUILD_BUG_ON(sizeof(f) != sizeof(unsigned long)); \
+ ((f) = __spin_lock_irqsave(l)); \
+ })
+unsigned long __spin_lock_irqsave(spinlock_t *lock);
-void _spin_unlock(spinlock_t *lock);
-void _spin_unlock_irq(spinlock_t *lock);
-void _spin_unlock_irqrestore(spinlock_t *lock, unsigned long flags);
+void spin_unlock(spinlock_t *lock);
+void spin_unlock_irq(spinlock_t *lock);
+void spin_unlock_irqrestore(spinlock_t *lock, unsigned long flags);
-int _spin_is_locked(const spinlock_t *lock);
-int _spin_trylock(spinlock_t *lock);
-void _spin_barrier(spinlock_t *lock);
+int spin_is_locked(const spinlock_t *lock);
+int spin_trylock(spinlock_t *lock);
+#define spin_trylock_irqsave(lock, flags) \
+({ \
+ local_irq_save(flags); \
+ spin_trylock(lock) ? \
+ 1 : ({ local_irq_restore(flags); 0; }); \
+})
+/* Ensure a lock is quiescent between two critical operations. */
+void spin_barrier(spinlock_t *lock);
/*
* rspin_[un]lock(): Use these forms when the lock can (safely!) be
@@ -270,32 +282,6 @@ void nrspin_unlock_irq(rspinlock_t *lock);
unsigned long __nrspin_lock_irqsave(rspinlock_t *lock);
void nrspin_unlock_irqrestore(rspinlock_t *lock, unsigned long flags);
-#define spin_lock(l) _spin_lock(l)
-#define spin_lock_cb(l, c, d) _spin_lock_cb(l, c, d)
-#define spin_lock_irq(l) _spin_lock_irq(l)
-#define spin_lock_irqsave(l, f) \
- ({ \
- BUILD_BUG_ON(sizeof(f) != sizeof(unsigned long)); \
- ((f) = _spin_lock_irqsave(l)); \
- })
-
-#define spin_unlock(l) _spin_unlock(l)
-#define spin_unlock_irq(l) _spin_unlock_irq(l)
-#define spin_unlock_irqrestore(l, f) _spin_unlock_irqrestore(l, f)
-
-#define spin_is_locked(l) _spin_is_locked(l)
-#define spin_trylock(l) _spin_trylock(l)
-
-#define spin_trylock_irqsave(lock, flags) \
-({ \
- local_irq_save(flags); \
- spin_trylock(lock) ? \
- 1 : ({ local_irq_restore(flags); 0; }); \
-})
-
#define spin_lock_kick(l) arch_lock_signal_wmb()
-/* Ensure a lock is quiescent between two critical operations. */
-#define spin_barrier(l) _spin_barrier(l)
-
#endif /* __SPINLOCK_H__ */
--
2.35.3
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH v3 13/13] xen/spinlock: support higher number of cpus
2023-11-20 11:38 [PATCH v3 00/13] xen/spinlock: make recursive spinlocks a dedicated type Juergen Gross
` (11 preceding siblings ...)
2023-11-20 11:38 ` [PATCH v3 12/13] xen/spinlock: remove indirection through macros for spin_*() functions Juergen Gross
@ 2023-11-20 11:38 ` Juergen Gross
12 siblings, 0 replies; 23+ messages in thread
From: Juergen Gross @ 2023-11-20 11:38 UTC (permalink / raw)
To: xen-devel
Cc: javi.merino, Juergen Gross, Andrew Cooper, George Dunlap,
Jan Beulich, Julien Grall, Stefano Stabellini, Wei Liu
Allow 16 bits per cpu number, which is the limit imposed by
spinlock_tickets_t.
This will allow up to 65535 cpus, while increasing only the size of
recursive spinlocks in debug builds from 8 to 12 bytes.
Signed-off-by: Juergen Gross <jgross@suse.com>
---
xen/common/spinlock.c | 1 +
xen/include/xen/spinlock.h | 18 +++++++++---------
2 files changed, 10 insertions(+), 9 deletions(-)
diff --git a/xen/common/spinlock.c b/xen/common/spinlock.c
index 43c876b64a..8faf7864ed 100644
--- a/xen/common/spinlock.c
+++ b/xen/common/spinlock.c
@@ -481,6 +481,7 @@ int rspin_trylock(rspinlock_t *lock)
/* Don't allow overflow of recurse_cpu field. */
BUILD_BUG_ON(NR_CPUS > SPINLOCK_NO_CPU);
+ BUILD_BUG_ON(SPINLOCK_CPU_BITS > sizeof(lock->recurse_cpu) * 8);
BUILD_BUG_ON(SPINLOCK_RECURSE_BITS < 3);
check_lock(&lock->debug, true);
diff --git a/xen/include/xen/spinlock.h b/xen/include/xen/spinlock.h
index e4f2e36e4f..ea88f3ad75 100644
--- a/xen/include/xen/spinlock.h
+++ b/xen/include/xen/spinlock.h
@@ -7,16 +7,16 @@
#include <asm/system.h>
#include <asm/spinlock.h>
-#define SPINLOCK_CPU_BITS 12
+#define SPINLOCK_CPU_BITS 16
#ifdef CONFIG_DEBUG_LOCKS
union lock_debug {
- uint16_t val;
-#define LOCK_DEBUG_INITVAL 0xffff
+ uint32_t val;
+#define LOCK_DEBUG_INITVAL 0xffffffff
struct {
- uint16_t cpu:SPINLOCK_CPU_BITS;
-#define LOCK_DEBUG_PAD_BITS (14 - SPINLOCK_CPU_BITS)
- uint16_t :LOCK_DEBUG_PAD_BITS;
+ uint32_t cpu:SPINLOCK_CPU_BITS;
+#define LOCK_DEBUG_PAD_BITS (30 - SPINLOCK_CPU_BITS)
+ uint32_t :LOCK_DEBUG_PAD_BITS;
bool irq_safe:1;
bool unseen:1;
};
@@ -210,10 +210,10 @@ typedef struct spinlock {
typedef struct rspinlock {
spinlock_tickets_t tickets;
- uint16_t recurse_cpu:SPINLOCK_CPU_BITS;
+ uint16_t recurse_cpu;
#define SPINLOCK_NO_CPU ((1u << SPINLOCK_CPU_BITS) - 1)
-#define SPINLOCK_RECURSE_BITS (16 - SPINLOCK_CPU_BITS)
- uint16_t recurse_cnt:SPINLOCK_RECURSE_BITS;
+#define SPINLOCK_RECURSE_BITS 8
+ uint8_t recurse_cnt;
#define SPINLOCK_MAX_RECURSE ((1u << SPINLOCK_RECURSE_BITS) - 1)
union lock_debug debug;
#ifdef CONFIG_DEBUG_LOCK_PROFILE
--
2.35.3
^ permalink raw reply related [flat|nested] 23+ messages in thread