[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 2/2] xen/spinlock: merge recurse_cpu and debug.cpu fields in struct spinlock
On 25.02.22 10:24, Jan Beulich wrote:
On 25.02.2022 09:55, Juergen Gross wrote:
On 25.02.22 09:36, Juergen Gross wrote:
On 24.02.22 17:11, Jan Beulich wrote:
On 24.02.2022 11:54, Juergen Gross wrote:
--- a/xen/arch/x86/mm/mm-locks.h
+++ b/xen/arch/x86/mm/mm-locks.h
@@ -42,7 +42,7 @@ static inline void mm_lock_init(mm_lock_t *l)
static inline bool mm_locked_by_me(const mm_lock_t *l)
{
- return (l->lock.recurse_cpu == current->processor);
+ return (l->lock.data.cpu == current->processor);
}
I see a fair risk with this: Behavior will now differ between debug and
non-debug builds. E.g. a livelock because of trying to acquire the same
lock again would not be noticed in a debug build if the acquire is
conditional upon this function's return value. I think this is the main
reason behind having two separate field, despite the apparent redundancy.
You are aware that mm_locked_by_me() is used for recursive spinlocks
only?
BTW, it might make sense to add another bool for the debug case to mark
recursive lock usage. I don't think anything good can come from using a
lock in both modes (recursive and non-recursive).
But beware of the coexisting paging_lock() and paging_lock_recursive().
Albeit I guess your comment was for spinlocks in general, not the
mm-lock machinery. Yet mentioning this reminds me of the page alloc
lock, which different paths acquire in different ways. So the bit
couldn't be a sticky one.
Interesting.
Seems as if e.g. console_lock is used in both ways, too.
Might be a good idea to at least add some self-deadlock detection
support to debug builds.
Juergen
Attachment:
OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key
Attachment:
OpenPGP_signature
Description: OpenPGP digital signature
|
