[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [RFC PATCH 2/3] xen/spinlock: split recursive spinlocks from normal ones



On 14.12.22 12:29, Jan Beulich wrote:
On 14.12.2022 12:10, Juergen Gross wrote:
On 14.12.22 11:39, Jan Beulich wrote:
On 10.09.2022 17:49, Juergen Gross wrote:
--- a/xen/arch/x86/mm/p2m-pod.c
+++ b/xen/arch/x86/mm/p2m-pod.c
@@ -397,7 +397,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);
+    spin_barrier(&p2m->pod.lock.lock.lock);

This is getting unwieldy as well, and ...

@@ -160,21 +165,30 @@ typedef union {
typedef struct spinlock {
       spinlock_tickets_t tickets;
-    u16 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;
-#define SPINLOCK_MAX_RECURSE   ((1u << SPINLOCK_RECURSE_BITS) - 1)
       union lock_debug debug;
   #ifdef CONFIG_DEBUG_LOCK_PROFILE
       struct lock_profile *profile;
   #endif
   } spinlock_t;
+struct spinlock_recursive {
+    struct spinlock lock;
+    u16 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;
+#define SPINLOCK_MAX_RECURSE   ((1u << SPINLOCK_RECURSE_BITS) - 1)
+};

... I'm not sure anyway it's a good idea to embed spinlock_t inside the
new struct. I'd prefer if non-optional fields were always at the same
position, and there's not going to be that much duplication if we went
with

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;
      u16 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;
#define SPINLOCK_MAX_RECURSE   ((1u << SPINLOCK_RECURSE_BITS) - 1)
      union lock_debug debug;
#ifdef CONFIG_DEBUG_LOCK_PROFILE
      struct lock_profile *profile;
#endif
} rspinlock_t;

This would also eliminate the size increase of recursive locks in
debug builds. And functions like spin_barrier() then also would
(have to) properly indicate what kind of lock they act on.

You are aware that this would require to duplicate all the spinlock
functions for the recursive spinlocks?

Well, to be honest I didn't really consider this aspect, but I think
that's a reasonable price to pay (with some de-duplication potential
if we wanted to), provided we want to go this route in the first place.

Okay.

The latest with this aspect in mind I wonder whether we aren't better
off with the current state (the more that iirc Andrew thinks that we
should get rid of recursive locking altogether).

The question is how (and when) to reach that goal.

In the end this was the reason to send the series as RFC first.

I'm waiting with addressing your comments until there is consensus
that the whole idea is really worth to be pursued.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.