[Xen-devel] [PATCH 0/2] smp: convert cpu_add_remove_lock int a rw lock

[Xen-devel] [PATCH 0/2] smp: convert cpu_add_remove_lock int a rw lock

[Roger Pau Monne]

Hello,

The main aim of this series is to reduce the pressure around
cpu_add_remove_lock by converting it into a rw lock. Most users of the
lock want to take it in read mode, as they only care about the maps not
changing.

Patch #2 makes the writers take the lock in blocking mode, this is
mainly done to reduce the failure of the CPU plug/unplug operations,
since the lock is more contended now and trylock can easily fail if
there are readers.

Thanks, Roger.

Roger Pau Monne (2):
  smp: convert the cpu maps lock into a rw lock
  smp: convert cpu_hotplug_begin into a blocking lock acquisition

 xen/arch/x86/smpboot.c |  3 +--
 xen/common/cpu.c       | 29 ++++++++++++++++++-----------
 xen/include/xen/cpu.h  | 13 +++----------
 3 files changed, 22 insertions(+), 23 deletions(-)

-- 
2.25.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH 1/2] smp: convert the cpu maps lock into a rw lock

[Roger Pau Monne]

Most users of the cpu maps just care about the maps not changing while
the lock is being held, but don't actually modify the maps.

Convert the lock into a rw lock, and take the lock in read mode in
get_cpu_maps and in write mode in cpu_hotplug_begin. This will lower
the contention around the lock, since plug and unplug operations that
take the lock in write mode are not that common.

Note that the read lock can be taken recursively (as it's a shared
lock), and hence will keep the same behavior as the previously used
recursive lock. As for the write lock, it's only used by CPU
plug/unplug operations, and the lock is never taken recursively in
that case.

While there also change get_cpu_maps return type to bool.

Reported-by: Julien Grall <julien@xen.org>
Suggested-also-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/common/cpu.c      | 22 ++++++++++++++++------
 xen/include/xen/cpu.h | 13 +++----------
 2 files changed, 19 insertions(+), 16 deletions(-)

diff --git a/xen/common/cpu.c b/xen/common/cpu.c
index 66c855c5d9..0d7a10878c 100644
--- a/xen/common/cpu.c
+++ b/xen/common/cpu.c
@@ -39,26 +39,36 @@ const unsigned long cpu_bit_bitmap[BITS_PER_LONG+1][BITS_TO_LONGS(NR_CPUS)] = {
 #endif
 };
 
-static DEFINE_SPINLOCK(cpu_add_remove_lock);
+static DEFINE_RWLOCK(cpu_add_remove_lock);
 
-bool_t get_cpu_maps(void)
+bool get_cpu_maps(void)
 {
-    return spin_trylock_recursive(&cpu_add_remove_lock);
+    return read_trylock(&cpu_add_remove_lock);
 }
 
 void put_cpu_maps(void)
 {
-    spin_unlock_recursive(&cpu_add_remove_lock);
+    read_unlock(&cpu_add_remove_lock);
+}
+
+bool cpu_hotplug_begin(void)
+{
+    return write_trylock(&cpu_add_remove_lock);
+}
+
+void cpu_hotplug_done(void)
+{
+    write_unlock(&cpu_add_remove_lock);
 }
 
 static NOTIFIER_HEAD(cpu_chain);
 
 void __init register_cpu_notifier(struct notifier_block *nb)
 {
-    if ( !spin_trylock(&cpu_add_remove_lock) )
+    if ( !write_trylock(&cpu_add_remove_lock) )
         BUG(); /* Should never fail as we are called only during boot. */
     notifier_chain_register(&cpu_chain, nb);
-    spin_unlock(&cpu_add_remove_lock);
+    write_unlock(&cpu_add_remove_lock);
 }
 
 static int cpu_notifier_call_chain(unsigned int cpu, unsigned long action,
diff --git a/xen/include/xen/cpu.h b/xen/include/xen/cpu.h
index 2c87db26f6..e49172f64c 100644
--- a/xen/include/xen/cpu.h
+++ b/xen/include/xen/cpu.h
@@ -6,19 +6,12 @@
 #include <xen/notifier.h>
 
 /* Safely access cpu_online_map, cpu_present_map, etc. */
-bool_t get_cpu_maps(void);
+bool get_cpu_maps(void);
 void put_cpu_maps(void);
 
 /* Safely perform CPU hotplug and update cpu_online_map, etc. */
-static inline bool cpu_hotplug_begin(void)
-{
-    return get_cpu_maps();
-}
-
-static inline void cpu_hotplug_done(void)
-{
-    put_cpu_maps();
-}
+bool cpu_hotplug_begin(void);
+void cpu_hotplug_done(void);
 
 /* Receive notification of CPU hotplug events. */
 void register_cpu_notifier(struct notifier_block *nb);
-- 
2.25.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH 2/2] smp: convert cpu_hotplug_begin into a blocking lock acquisition

[Roger Pau Monne]

Don't allow cpu_hotplug_begin to fail by converting the trylock into a
blocking lock acquisition. Write users of the cpu_add_remove_lock are
limited to CPU plug/unplug operations, and cannot deadlock between
themselves or other users taking the lock in read mode as
cpu_add_remove_lock is always locked with interrupts enabled. There
are also no other locks taken during the plug/unplug operations.

The exclusive lock usage in register_cpu_notifier is also converted
into a blocking lock acquisition, as it was previously not allowed to
fail anyway.

This is meaningful when running Xen in shim mode, since VCPU_{up/down}
hypercalls use cpu hotplug/unplug operations in the background, and
hence failing to take the lock results in VPCU_{up/down} failing with
-EBUSY, which most users are not prepared to handle.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
I've tested this and seems to work fine AFAICT either when running on
native or when used in the shim. I'm not sure if I'm missing something
that would prevent the write lock acquisition from being made
blocking.
---
 xen/arch/x86/smpboot.c |  3 +--
 xen/common/cpu.c       | 13 +++++--------
 xen/include/xen/cpu.h  |  2 +-
 3 files changed, 7 insertions(+), 11 deletions(-)

diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c
index 67ee490f94..cc0d62f9e2 100644
--- a/xen/arch/x86/smpboot.c
+++ b/xen/arch/x86/smpboot.c
@@ -1286,8 +1286,7 @@ int cpu_add(uint32_t apic_id, uint32_t acpi_id, uint32_t pxm)
          (pxm >= 256) )
         return -EINVAL;
 
-    if ( !cpu_hotplug_begin() )
-        return -EBUSY;
+    cpu_hotplug_begin();
 
     /* Detect if the cpu has been added before */
     if ( x86_acpiid_to_apicid[acpi_id] != BAD_APICID )
diff --git a/xen/common/cpu.c b/xen/common/cpu.c
index 0d7a10878c..31953f32e4 100644
--- a/xen/common/cpu.c
+++ b/xen/common/cpu.c
@@ -51,9 +51,9 @@ void put_cpu_maps(void)
     read_unlock(&cpu_add_remove_lock);
 }
 
-bool cpu_hotplug_begin(void)
+void cpu_hotplug_begin(void)
 {
-    return write_trylock(&cpu_add_remove_lock);
+    write_lock(&cpu_add_remove_lock);
 }
 
 void cpu_hotplug_done(void)
@@ -65,8 +65,7 @@ static NOTIFIER_HEAD(cpu_chain);
 
 void __init register_cpu_notifier(struct notifier_block *nb)
 {
-    if ( !write_trylock(&cpu_add_remove_lock) )
-        BUG(); /* Should never fail as we are called only during boot. */
+    write_lock(&cpu_add_remove_lock);
     notifier_chain_register(&cpu_chain, nb);
     write_unlock(&cpu_add_remove_lock);
 }
@@ -100,8 +99,7 @@ int cpu_down(unsigned int cpu)
     int err;
     struct notifier_block *nb = NULL;
 
-    if ( !cpu_hotplug_begin() )
-        return -EBUSY;
+    cpu_hotplug_begin();
 
     err = -EINVAL;
     if ( (cpu >= nr_cpu_ids) || (cpu == 0) )
@@ -142,8 +140,7 @@ int cpu_up(unsigned int cpu)
     int err;
     struct notifier_block *nb = NULL;
 
-    if ( !cpu_hotplug_begin() )
-        return -EBUSY;
+    cpu_hotplug_begin();
 
     err = -EINVAL;
     if ( (cpu >= nr_cpu_ids) || !cpu_present(cpu) )
diff --git a/xen/include/xen/cpu.h b/xen/include/xen/cpu.h
index e49172f64c..e8eeb217a0 100644
--- a/xen/include/xen/cpu.h
+++ b/xen/include/xen/cpu.h
@@ -10,7 +10,7 @@ bool get_cpu_maps(void);
 void put_cpu_maps(void);
 
 /* Safely perform CPU hotplug and update cpu_online_map, etc. */
-bool cpu_hotplug_begin(void);
+void cpu_hotplug_begin(void);
 void cpu_hotplug_done(void);
 
 /* Receive notification of CPU hotplug events. */
-- 
2.25.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 1/2] smp: convert the cpu maps lock into a rw lock

[Julien Grall]

Hi Roger,

On 13/02/2020 11:32, Roger Pau Monne wrote:
> Most users of the cpu maps just care about the maps not changing while
> the lock is being held, but don't actually modify the maps.
> 
> Convert the lock into a rw lock, and take the lock in read mode in
> get_cpu_maps and in write mode in cpu_hotplug_begin. This will lower
> the contention around the lock, since plug and unplug operations that
> take the lock in write mode are not that common.
> 
> Note that the read lock can be taken recursively (as it's a shared
> lock), and hence will keep the same behavior as the previously used
> recursive lock. As for the write lock, it's only used by CPU
> plug/unplug operations, and the lock is never taken recursively in
> that case.
> 
> While there also change get_cpu_maps return type to bool.
> 
> Reported-by: Julien Grall <julien@xen.org>
> Suggested-also-by: Jan Beulich <jbeulich@suse.com>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Reviewed-by: Julien Grall <julien@xen.org>

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 0/2] smp: convert cpu_add_remove_lock int a rw lock

[Andrew Cooper]

On 13/02/2020 11:32, Roger Pau Monne wrote:
> Hello,
>
> The main aim of this series is to reduce the pressure around
> cpu_add_remove_lock by converting it into a rw lock. Most users of the
> lock want to take it in read mode, as they only care about the maps not
> changing.
>
> Patch #2 makes the writers take the lock in blocking mode, this is
> mainly done to reduce the failure of the CPU plug/unplug operations,
> since the lock is more contended now and trylock can easily fail if
> there are readers.

Seems like a very sensible move.

Both patches Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 1/2] smp: convert the cpu maps lock into a rw lock

[Jan Beulich]

On 13.02.2020 12:32, Roger Pau Monne wrote:
>  void __init register_cpu_notifier(struct notifier_block *nb)
>  {
> -    if ( !spin_trylock(&cpu_add_remove_lock) )
> +    if ( !write_trylock(&cpu_add_remove_lock) )
>          BUG(); /* Should never fail as we are called only during boot. */
>      notifier_chain_register(&cpu_chain, nb);
> -    spin_unlock(&cpu_add_remove_lock);
> +    write_unlock(&cpu_add_remove_lock);
>  }

So why a write lock here?

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 2/2] smp: convert cpu_hotplug_begin into a blocking lock acquisition

[Jan Beulich]

On 13.02.2020 12:32, Roger Pau Monne wrote:
> Don't allow cpu_hotplug_begin to fail by converting the trylock into a
> blocking lock acquisition. Write users of the cpu_add_remove_lock are
> limited to CPU plug/unplug operations, and cannot deadlock between
> themselves or other users taking the lock in read mode as
> cpu_add_remove_lock is always locked with interrupts enabled. There
> are also no other locks taken during the plug/unplug operations.

I don't think the goal was deadlock avoidance, but rather limiting
of the time spent spinning while trying to acquire the lock, in
favor of having the caller retry.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 1/2] smp: convert the cpu maps lock into a rw lock

[Roger Pau Monné]

On Wed, Feb 19, 2020 at 01:56:02PM +0100, Jan Beulich wrote:
> On 13.02.2020 12:32, Roger Pau Monne wrote:
> >  void __init register_cpu_notifier(struct notifier_block *nb)
> >  {
> > -    if ( !spin_trylock(&cpu_add_remove_lock) )
> > +    if ( !write_trylock(&cpu_add_remove_lock) )
> >          BUG(); /* Should never fail as we are called only during boot. */
> >      notifier_chain_register(&cpu_chain, nb);
> > -    spin_unlock(&cpu_add_remove_lock);
> > +    write_unlock(&cpu_add_remove_lock);
> >  }
> 
> So why a write lock here?

notifier_chain_register calls cannot be made in parallel, as they
modify the underlying notifier list without taking any additional
locks.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 2/2] smp: convert cpu_hotplug_begin into a blocking lock acquisition

[Roger Pau Monné]

On Wed, Feb 19, 2020 at 01:59:51PM +0100, Jan Beulich wrote:
> On 13.02.2020 12:32, Roger Pau Monne wrote:
> > Don't allow cpu_hotplug_begin to fail by converting the trylock into a
> > blocking lock acquisition. Write users of the cpu_add_remove_lock are
> > limited to CPU plug/unplug operations, and cannot deadlock between
> > themselves or other users taking the lock in read mode as
> > cpu_add_remove_lock is always locked with interrupts enabled. There
> > are also no other locks taken during the plug/unplug operations.
> 
> I don't think the goal was deadlock avoidance, but rather limiting
> of the time spent spinning while trying to acquire the lock, in
> favor of having the caller retry.

Now that the contention between read-only users is reduced as those
can take the lock in parallel I think it's safe to switch writers to
blocking mode.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 1/2] smp: convert the cpu maps lock into a rw lock

[Jan Beulich]

On 19.02.2020 14:19, Roger Pau Monné wrote:
> On Wed, Feb 19, 2020 at 01:56:02PM +0100, Jan Beulich wrote:
>> On 13.02.2020 12:32, Roger Pau Monne wrote:
>>>  void __init register_cpu_notifier(struct notifier_block *nb)
>>>  {
>>> -    if ( !spin_trylock(&cpu_add_remove_lock) )
>>> +    if ( !write_trylock(&cpu_add_remove_lock) )
>>>          BUG(); /* Should never fail as we are called only during boot. */
>>>      notifier_chain_register(&cpu_chain, nb);
>>> -    spin_unlock(&cpu_add_remove_lock);
>>> +    write_unlock(&cpu_add_remove_lock);
>>>  }
>>
>> So why a write lock here?
> 
> notifier_chain_register calls cannot be made in parallel, as they
> modify the underlying notifier list without taking any additional
> locks.

I.e. the lock is being (ab)used to also protect the notifier list,
which is certainly not its purpose. (The locking seems quite
pointless here anyway considering the __init together with the
nature of the function.)

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 2/2] smp: convert cpu_hotplug_begin into a blocking lock acquisition

[Jan Beulich]

On 19.02.2020 14:22, Roger Pau Monné wrote:
> On Wed, Feb 19, 2020 at 01:59:51PM +0100, Jan Beulich wrote:
>> On 13.02.2020 12:32, Roger Pau Monne wrote:
>>> Don't allow cpu_hotplug_begin to fail by converting the trylock into a
>>> blocking lock acquisition. Write users of the cpu_add_remove_lock are
>>> limited to CPU plug/unplug operations, and cannot deadlock between
>>> themselves or other users taking the lock in read mode as
>>> cpu_add_remove_lock is always locked with interrupts enabled. There
>>> are also no other locks taken during the plug/unplug operations.
>>
>> I don't think the goal was deadlock avoidance, but rather limiting
>> of the time spent spinning while trying to acquire the lock, in
>> favor of having the caller retry.
> 
> Now that the contention between read-only users is reduced as those
> can take the lock in parallel I think it's safe to switch writers to
> blocking mode.

I'd agree if writers couldn't be starved by (many) readers.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 1/2] smp: convert the cpu maps lock into a rw lock

[Roger Pau Monné]

On Wed, Feb 19, 2020 at 02:42:58PM +0100, Jan Beulich wrote:
> On 19.02.2020 14:19, Roger Pau Monné wrote:
> > On Wed, Feb 19, 2020 at 01:56:02PM +0100, Jan Beulich wrote:
> >> On 13.02.2020 12:32, Roger Pau Monne wrote:
> >>>  void __init register_cpu_notifier(struct notifier_block *nb)
> >>>  {
> >>> -    if ( !spin_trylock(&cpu_add_remove_lock) )
> >>> +    if ( !write_trylock(&cpu_add_remove_lock) )
> >>>          BUG(); /* Should never fail as we are called only during boot. */
> >>>      notifier_chain_register(&cpu_chain, nb);
> >>> -    spin_unlock(&cpu_add_remove_lock);
> >>> +    write_unlock(&cpu_add_remove_lock);
> >>>  }
> >>
> >> So why a write lock here?
> > 
> > notifier_chain_register calls cannot be made in parallel, as they
> > modify the underlying notifier list without taking any additional
> > locks.
> 
> I.e. the lock is being (ab)used to also protect the notifier list,
> which is certainly not its purpose. (The locking seems quite
> pointless here anyway considering the __init together with the
> nature of the function.)

Right, it's quite likely that the lock is pointless, I haven't looked
at all the callers to assure this. Anyway, iff the lock can be safely
removed that should be done in a different patch, and not this one
IMO. This merely switching existing users to use a rw lock.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 2/2] smp: convert cpu_hotplug_begin into a blocking lock acquisition

[Roger Pau Monné]

On Wed, Feb 19, 2020 at 02:44:12PM +0100, Jan Beulich wrote:
> On 19.02.2020 14:22, Roger Pau Monné wrote:
> > On Wed, Feb 19, 2020 at 01:59:51PM +0100, Jan Beulich wrote:
> >> On 13.02.2020 12:32, Roger Pau Monne wrote:
> >>> Don't allow cpu_hotplug_begin to fail by converting the trylock into a
> >>> blocking lock acquisition. Write users of the cpu_add_remove_lock are
> >>> limited to CPU plug/unplug operations, and cannot deadlock between
> >>> themselves or other users taking the lock in read mode as
> >>> cpu_add_remove_lock is always locked with interrupts enabled. There
> >>> are also no other locks taken during the plug/unplug operations.
> >>
> >> I don't think the goal was deadlock avoidance, but rather limiting
> >> of the time spent spinning while trying to acquire the lock, in
> >> favor of having the caller retry.
> > 
> > Now that the contention between read-only users is reduced as those
> > can take the lock in parallel I think it's safe to switch writers to
> > blocking mode.
> 
> I'd agree if writers couldn't be starved by (many) readers.

AFAICT from the rw lock implementation readers won't be able to pick
the lock as soon as there's a writer waiting, which should avoid this
starvation?

You still need to wait for current readers to drop the lock, but no
new readers would be able to lock it, which I think should givbe us
enough fairness. OTOH when using _trylock new readers can still pick
the lock in read mode, and hence I think using blocking mode for
writers is actually better, as you can assure that readers won't be
able to starve writers.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 2/2] smp: convert cpu_hotplug_begin into a blocking lock acquisition

[Jan Beulich]

On 19.02.2020 15:45, Roger Pau Monné wrote:
> On Wed, Feb 19, 2020 at 02:44:12PM +0100, Jan Beulich wrote:
>> On 19.02.2020 14:22, Roger Pau Monné wrote:
>>> On Wed, Feb 19, 2020 at 01:59:51PM +0100, Jan Beulich wrote:
>>>> On 13.02.2020 12:32, Roger Pau Monne wrote:
>>>>> Don't allow cpu_hotplug_begin to fail by converting the trylock into a
>>>>> blocking lock acquisition. Write users of the cpu_add_remove_lock are
>>>>> limited to CPU plug/unplug operations, and cannot deadlock between
>>>>> themselves or other users taking the lock in read mode as
>>>>> cpu_add_remove_lock is always locked with interrupts enabled. There
>>>>> are also no other locks taken during the plug/unplug operations.
>>>>
>>>> I don't think the goal was deadlock avoidance, but rather limiting
>>>> of the time spent spinning while trying to acquire the lock, in
>>>> favor of having the caller retry.
>>>
>>> Now that the contention between read-only users is reduced as those
>>> can take the lock in parallel I think it's safe to switch writers to
>>> blocking mode.
>>
>> I'd agree if writers couldn't be starved by (many) readers.
> 
> AFAICT from the rw lock implementation readers won't be able to pick
> the lock as soon as there's a writer waiting, which should avoid this
> starvation?
> 
> You still need to wait for current readers to drop the lock, but no
> new readers would be able to lock it, which I think should givbe us
> enough fairness.

Ah, right, it was rather the other way around - back-to-back
writers can starve readers with our current implementation.

> OTOH when using _trylock new readers can still pick
> the lock in read mode, and hence I think using blocking mode for
> writers is actually better, as you can assure that readers won't be
> able to starve writers.

This is a good point. Nevertheless I remain unconvinced that
the change is warranted given the original intentions (as far
as we're able to reconstruct them). If the current behavior
gets in the way of sensible shim operation, perhaps the
behavior should be made dependent upon running in shim mode?

In any event I think the commit message here would want
updating. In the meantime I think I'll commit patch 1 with
Andrew's ack.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 2/2] smp: convert cpu_hotplug_begin into a blocking lock acquisition

[Andrew Cooper]

On 19/02/2020 13:44, Jan Beulich wrote:
> On 19.02.2020 14:22, Roger Pau Monné wrote:
>> On Wed, Feb 19, 2020 at 01:59:51PM +0100, Jan Beulich wrote:
>>> On 13.02.2020 12:32, Roger Pau Monne wrote:
>>>> Don't allow cpu_hotplug_begin to fail by converting the trylock into a
>>>> blocking lock acquisition. Write users of the cpu_add_remove_lock are
>>>> limited to CPU plug/unplug operations, and cannot deadlock between
>>>> themselves or other users taking the lock in read mode as
>>>> cpu_add_remove_lock is always locked with interrupts enabled. There
>>>> are also no other locks taken during the plug/unplug operations.
>>> I don't think the goal was deadlock avoidance, but rather limiting
>>> of the time spent spinning while trying to acquire the lock, in
>>> favor of having the caller retry.
>> Now that the contention between read-only users is reduced as those
>> can take the lock in parallel I think it's safe to switch writers to
>> blocking mode.
> I'd agree if writers couldn't be starved by (many) readers.

XSA-114.

We fixed that vulnerability ages ago.

A writer wanting the lock will prevent further readers from taking it
before the writer drops it.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 2/2] smp: convert cpu_hotplug_begin into a blocking lock acquisition

[Andrew Cooper]

On 19/02/2020 14:57, Jan Beulich wrote:
> On 19.02.2020 15:45, Roger Pau Monné wrote:
>> On Wed, Feb 19, 2020 at 02:44:12PM +0100, Jan Beulich wrote:
>>> On 19.02.2020 14:22, Roger Pau Monné wrote:
>>>> On Wed, Feb 19, 2020 at 01:59:51PM +0100, Jan Beulich wrote:
>>>>> On 13.02.2020 12:32, Roger Pau Monne wrote:
>>>>>> Don't allow cpu_hotplug_begin to fail by converting the trylock into a
>>>>>> blocking lock acquisition. Write users of the cpu_add_remove_lock are
>>>>>> limited to CPU plug/unplug operations, and cannot deadlock between
>>>>>> themselves or other users taking the lock in read mode as
>>>>>> cpu_add_remove_lock is always locked with interrupts enabled. There
>>>>>> are also no other locks taken during the plug/unplug operations.
>>>>> I don't think the goal was deadlock avoidance, but rather limiting
>>>>> of the time spent spinning while trying to acquire the lock, in
>>>>> favor of having the caller retry.
>>>> Now that the contention between read-only users is reduced as those
>>>> can take the lock in parallel I think it's safe to switch writers to
>>>> blocking mode.
>>> I'd agree if writers couldn't be starved by (many) readers.
>> AFAICT from the rw lock implementation readers won't be able to pick
>> the lock as soon as there's a writer waiting, which should avoid this
>> starvation?
>>
>> You still need to wait for current readers to drop the lock, but no
>> new readers would be able to lock it, which I think should givbe us
>> enough fairness.
> Ah, right, it was rather the other way around - back-to-back
> writers can starve readers with our current implementation.
>
>> OTOH when using _trylock new readers can still pick
>> the lock in read mode, and hence I think using blocking mode for
>> writers is actually better, as you can assure that readers won't be
>> able to starve writers.
> This is a good point. Nevertheless I remain unconvinced that
> the change is warranted given the original intentions (as far
> as we're able to reconstruct them). If the current behavior
> gets in the way of sensible shim operation, perhaps the
> behavior should be made dependent upon running in shim mode?

Hotplug isn't generally used at all, so there is 0 write pressure on the
lock.

When it is used, it is all at explicit request from the controlling
entity in the system (hardware domain, or singleton shim domain).

If that entity is trying to DoS you, you've already lost.

There might be attempts to justify why the locking was done like that in
the first place, but it doesn't mean they were necessarily correct to
being with, and they don't match up with the realistic usage of the lock.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 2/2] smp: convert cpu_hotplug_begin into a blocking lock acquisition

[Jan Beulich]

On 19.02.2020 16:07, Andrew Cooper wrote:
> On 19/02/2020 14:57, Jan Beulich wrote:
>> On 19.02.2020 15:45, Roger Pau Monné wrote:
>>> On Wed, Feb 19, 2020 at 02:44:12PM +0100, Jan Beulich wrote:
>>>> On 19.02.2020 14:22, Roger Pau Monné wrote:
>>>>> On Wed, Feb 19, 2020 at 01:59:51PM +0100, Jan Beulich wrote:
>>>>>> On 13.02.2020 12:32, Roger Pau Monne wrote:
>>>>>>> Don't allow cpu_hotplug_begin to fail by converting the trylock into a
>>>>>>> blocking lock acquisition. Write users of the cpu_add_remove_lock are
>>>>>>> limited to CPU plug/unplug operations, and cannot deadlock between
>>>>>>> themselves or other users taking the lock in read mode as
>>>>>>> cpu_add_remove_lock is always locked with interrupts enabled. There
>>>>>>> are also no other locks taken during the plug/unplug operations.
>>>>>> I don't think the goal was deadlock avoidance, but rather limiting
>>>>>> of the time spent spinning while trying to acquire the lock, in
>>>>>> favor of having the caller retry.
>>>>> Now that the contention between read-only users is reduced as those
>>>>> can take the lock in parallel I think it's safe to switch writers to
>>>>> blocking mode.
>>>> I'd agree if writers couldn't be starved by (many) readers.
>>> AFAICT from the rw lock implementation readers won't be able to pick
>>> the lock as soon as there's a writer waiting, which should avoid this
>>> starvation?
>>>
>>> You still need to wait for current readers to drop the lock, but no
>>> new readers would be able to lock it, which I think should givbe us
>>> enough fairness.
>> Ah, right, it was rather the other way around - back-to-back
>> writers can starve readers with our current implementation.
>>
>>> OTOH when using _trylock new readers can still pick
>>> the lock in read mode, and hence I think using blocking mode for
>>> writers is actually better, as you can assure that readers won't be
>>> able to starve writers.
>> This is a good point. Nevertheless I remain unconvinced that
>> the change is warranted given the original intentions (as far
>> as we're able to reconstruct them). If the current behavior
>> gets in the way of sensible shim operation, perhaps the
>> behavior should be made dependent upon running in shim mode?
> 
> Hotplug isn't generally used at all, so there is 0 write pressure on the
> lock.
> 
> When it is used, it is all at explicit request from the controlling
> entity in the system (hardware domain, or singleton shim domain).
> 
> If that entity is trying to DoS you, you've already lost.

But write pressure was never in question. My concern is with
how long it might take for all readers to drop their locks.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 2/2] smp: convert cpu_hotplug_begin into a blocking lock acquisition

[Roger Pau Monné]

On Wed, Feb 19, 2020 at 03:07:14PM +0000, Andrew Cooper wrote:
> On 19/02/2020 14:57, Jan Beulich wrote:
> > On 19.02.2020 15:45, Roger Pau Monné wrote:
> >> On Wed, Feb 19, 2020 at 02:44:12PM +0100, Jan Beulich wrote:
> >>> On 19.02.2020 14:22, Roger Pau Monné wrote:
> >>>> On Wed, Feb 19, 2020 at 01:59:51PM +0100, Jan Beulich wrote:
> >>>>> On 13.02.2020 12:32, Roger Pau Monne wrote:
> >>>>>> Don't allow cpu_hotplug_begin to fail by converting the trylock into a
> >>>>>> blocking lock acquisition. Write users of the cpu_add_remove_lock are
> >>>>>> limited to CPU plug/unplug operations, and cannot deadlock between
> >>>>>> themselves or other users taking the lock in read mode as
> >>>>>> cpu_add_remove_lock is always locked with interrupts enabled. There
> >>>>>> are also no other locks taken during the plug/unplug operations.
> >>>>> I don't think the goal was deadlock avoidance, but rather limiting
> >>>>> of the time spent spinning while trying to acquire the lock, in
> >>>>> favor of having the caller retry.
> >>>> Now that the contention between read-only users is reduced as those
> >>>> can take the lock in parallel I think it's safe to switch writers to
> >>>> blocking mode.
> >>> I'd agree if writers couldn't be starved by (many) readers.
> >> AFAICT from the rw lock implementation readers won't be able to pick
> >> the lock as soon as there's a writer waiting, which should avoid this
> >> starvation?
> >>
> >> You still need to wait for current readers to drop the lock, but no
> >> new readers would be able to lock it, which I think should givbe us
> >> enough fairness.
> > Ah, right, it was rather the other way around - back-to-back
> > writers can starve readers with our current implementation.
> >
> >> OTOH when using _trylock new readers can still pick
> >> the lock in read mode, and hence I think using blocking mode for
> >> writers is actually better, as you can assure that readers won't be
> >> able to starve writers.
> > This is a good point. Nevertheless I remain unconvinced that
> > the change is warranted given the original intentions (as far
> > as we're able to reconstruct them). If the current behavior
> > gets in the way of sensible shim operation, perhaps the
> > behavior should be made dependent upon running in shim mode?
> 
> Hotplug isn't generally used at all, so there is 0 write pressure on the
> lock.
> 
> When it is used, it is all at explicit request from the controlling
> entity in the system (hardware domain, or singleton shim domain).
> 
> If that entity is trying to DoS you, you've already lost.
> 
> There might be attempts to justify why the locking was done like that in
> the first place, but it doesn't mean they were necessarily correct to
> being with, and they don't match up with the realistic usage of the lock.

I'm happy to rewrite the commit message in order to include the
discussion here. What about adding:

Note that when using rw locks a writer wanting to take the lock will
prevent further reads from locking it, hence preventing readers from
starving writers. Writers OTOH could starve readers, but since the
lock is only picked in write mode by actions requested by privileged
domains such entities already have the ability to DoS the hypervisor
in many other ways.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 2/2] smp: convert cpu_hotplug_begin into a blocking lock acquisition

[Roger Pau Monné]

On Wed, Feb 19, 2020 at 05:06:20PM +0100, Jan Beulich wrote:
> On 19.02.2020 16:07, Andrew Cooper wrote:
> > On 19/02/2020 14:57, Jan Beulich wrote:
> >> On 19.02.2020 15:45, Roger Pau Monné wrote:
> >>> On Wed, Feb 19, 2020 at 02:44:12PM +0100, Jan Beulich wrote:
> >>>> On 19.02.2020 14:22, Roger Pau Monné wrote:
> >>>>> On Wed, Feb 19, 2020 at 01:59:51PM +0100, Jan Beulich wrote:
> >>>>>> On 13.02.2020 12:32, Roger Pau Monne wrote:
> >>>>>>> Don't allow cpu_hotplug_begin to fail by converting the trylock into a
> >>>>>>> blocking lock acquisition. Write users of the cpu_add_remove_lock are
> >>>>>>> limited to CPU plug/unplug operations, and cannot deadlock between
> >>>>>>> themselves or other users taking the lock in read mode as
> >>>>>>> cpu_add_remove_lock is always locked with interrupts enabled. There
> >>>>>>> are also no other locks taken during the plug/unplug operations.
> >>>>>> I don't think the goal was deadlock avoidance, but rather limiting
> >>>>>> of the time spent spinning while trying to acquire the lock, in
> >>>>>> favor of having the caller retry.
> >>>>> Now that the contention between read-only users is reduced as those
> >>>>> can take the lock in parallel I think it's safe to switch writers to
> >>>>> blocking mode.
> >>>> I'd agree if writers couldn't be starved by (many) readers.
> >>> AFAICT from the rw lock implementation readers won't be able to pick
> >>> the lock as soon as there's a writer waiting, which should avoid this
> >>> starvation?
> >>>
> >>> You still need to wait for current readers to drop the lock, but no
> >>> new readers would be able to lock it, which I think should givbe us
> >>> enough fairness.
> >> Ah, right, it was rather the other way around - back-to-back
> >> writers can starve readers with our current implementation.
> >>
> >>> OTOH when using _trylock new readers can still pick
> >>> the lock in read mode, and hence I think using blocking mode for
> >>> writers is actually better, as you can assure that readers won't be
> >>> able to starve writers.
> >> This is a good point. Nevertheless I remain unconvinced that
> >> the change is warranted given the original intentions (as far
> >> as we're able to reconstruct them). If the current behavior
> >> gets in the way of sensible shim operation, perhaps the
> >> behavior should be made dependent upon running in shim mode?
> > 
> > Hotplug isn't generally used at all, so there is 0 write pressure on the
> > lock.
> > 
> > When it is used, it is all at explicit request from the controlling
> > entity in the system (hardware domain, or singleton shim domain).
> > 
> > If that entity is trying to DoS you, you've already lost.
> 
> But write pressure was never in question. My concern is with
> how long it might take for all readers to drop their locks.

The only long running operation that takes the CPU maps read lock is
microcode updating or livepatching, and since those are also started
by a privileged domain I think it's safe. Any sane admin wouldn't do a
CPU plug/unplug while updating microcode or doing a livepatching.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 2/2] smp: convert cpu_hotplug_begin into a blocking lock acquisition

[Andrew Cooper]

On 19/02/2020 16:06, Jan Beulich wrote:
> On 19.02.2020 16:07, Andrew Cooper wrote:
>> On 19/02/2020 14:57, Jan Beulich wrote:
>>> On 19.02.2020 15:45, Roger Pau Monné wrote:
>>>> On Wed, Feb 19, 2020 at 02:44:12PM +0100, Jan Beulich wrote:
>>>>> On 19.02.2020 14:22, Roger Pau Monné wrote:
>>>>>> On Wed, Feb 19, 2020 at 01:59:51PM +0100, Jan Beulich wrote:
>>>>>>> On 13.02.2020 12:32, Roger Pau Monne wrote:
>>>>>>>> Don't allow cpu_hotplug_begin to fail by converting the trylock into a
>>>>>>>> blocking lock acquisition. Write users of the cpu_add_remove_lock are
>>>>>>>> limited to CPU plug/unplug operations, and cannot deadlock between
>>>>>>>> themselves or other users taking the lock in read mode as
>>>>>>>> cpu_add_remove_lock is always locked with interrupts enabled. There
>>>>>>>> are also no other locks taken during the plug/unplug operations.
>>>>>>> I don't think the goal was deadlock avoidance, but rather limiting
>>>>>>> of the time spent spinning while trying to acquire the lock, in
>>>>>>> favor of having the caller retry.
>>>>>> Now that the contention between read-only users is reduced as those
>>>>>> can take the lock in parallel I think it's safe to switch writers to
>>>>>> blocking mode.
>>>>> I'd agree if writers couldn't be starved by (many) readers.
>>>> AFAICT from the rw lock implementation readers won't be able to pick
>>>> the lock as soon as there's a writer waiting, which should avoid this
>>>> starvation?
>>>>
>>>> You still need to wait for current readers to drop the lock, but no
>>>> new readers would be able to lock it, which I think should givbe us
>>>> enough fairness.
>>> Ah, right, it was rather the other way around - back-to-back
>>> writers can starve readers with our current implementation.
>>>
>>>> OTOH when using _trylock new readers can still pick
>>>> the lock in read mode, and hence I think using blocking mode for
>>>> writers is actually better, as you can assure that readers won't be
>>>> able to starve writers.
>>> This is a good point. Nevertheless I remain unconvinced that
>>> the change is warranted given the original intentions (as far
>>> as we're able to reconstruct them). If the current behavior
>>> gets in the way of sensible shim operation, perhaps the
>>> behavior should be made dependent upon running in shim mode?
>> Hotplug isn't generally used at all, so there is 0 write pressure on the
>> lock.
>>
>> When it is used, it is all at explicit request from the controlling
>> entity in the system (hardware domain, or singleton shim domain).
>>
>> If that entity is trying to DoS you, you've already lost.
> But write pressure was never in question. My concern is with
> how long it might take for all readers to drop their locks.

Why is that of concern?

Failing with -EBUSY is not a useful property for any caller - if you
initiate a hotplug request, then you're going to loop until it
completes.  (again - scarcity of hotplug requests in the first place is
why this interface works in the general case.)

With a spinlock (and indeed, ticketlock in our case), everyone is
serialised and you've got to wait until your turn to do anything. 
trylock and backoff makes the situation probabilistic as to whether it
succeeds, and gets increasingly worse as the lock becomes more contended.

With a rwlock, the maximum amount of time a writer needs to wait is the
longest remaining critical region of the readers, which by definition is
something short enough to not be an issue for said reader.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 2/2] smp: convert cpu_hotplug_begin into a blocking lock acquisition

[Jan Beulich]

On 19.02.2020 17:08, Roger Pau Monné wrote:
> On Wed, Feb 19, 2020 at 03:07:14PM +0000, Andrew Cooper wrote:
>> On 19/02/2020 14:57, Jan Beulich wrote:
>>> On 19.02.2020 15:45, Roger Pau Monné wrote:
>>>> On Wed, Feb 19, 2020 at 02:44:12PM +0100, Jan Beulich wrote:
>>>>> On 19.02.2020 14:22, Roger Pau Monné wrote:
>>>>>> On Wed, Feb 19, 2020 at 01:59:51PM +0100, Jan Beulich wrote:
>>>>>>> On 13.02.2020 12:32, Roger Pau Monne wrote:
>>>>>>>> Don't allow cpu_hotplug_begin to fail by converting the trylock into a
>>>>>>>> blocking lock acquisition. Write users of the cpu_add_remove_lock are
>>>>>>>> limited to CPU plug/unplug operations, and cannot deadlock between
>>>>>>>> themselves or other users taking the lock in read mode as
>>>>>>>> cpu_add_remove_lock is always locked with interrupts enabled. There
>>>>>>>> are also no other locks taken during the plug/unplug operations.
>>>>>>> I don't think the goal was deadlock avoidance, but rather limiting
>>>>>>> of the time spent spinning while trying to acquire the lock, in
>>>>>>> favor of having the caller retry.
>>>>>> Now that the contention between read-only users is reduced as those
>>>>>> can take the lock in parallel I think it's safe to switch writers to
>>>>>> blocking mode.
>>>>> I'd agree if writers couldn't be starved by (many) readers.
>>>> AFAICT from the rw lock implementation readers won't be able to pick
>>>> the lock as soon as there's a writer waiting, which should avoid this
>>>> starvation?
>>>>
>>>> You still need to wait for current readers to drop the lock, but no
>>>> new readers would be able to lock it, which I think should givbe us
>>>> enough fairness.
>>> Ah, right, it was rather the other way around - back-to-back
>>> writers can starve readers with our current implementation.
>>>
>>>> OTOH when using _trylock new readers can still pick
>>>> the lock in read mode, and hence I think using blocking mode for
>>>> writers is actually better, as you can assure that readers won't be
>>>> able to starve writers.
>>> This is a good point. Nevertheless I remain unconvinced that
>>> the change is warranted given the original intentions (as far
>>> as we're able to reconstruct them). If the current behavior
>>> gets in the way of sensible shim operation, perhaps the
>>> behavior should be made dependent upon running in shim mode?
>>
>> Hotplug isn't generally used at all, so there is 0 write pressure on the
>> lock.
>>
>> When it is used, it is all at explicit request from the controlling
>> entity in the system (hardware domain, or singleton shim domain).
>>
>> If that entity is trying to DoS you, you've already lost.
>>
>> There might be attempts to justify why the locking was done like that in
>> the first place, but it doesn't mean they were necessarily correct to
>> being with, and they don't match up with the realistic usage of the lock.
> 
> I'm happy to rewrite the commit message in order to include the
> discussion here. What about adding:
> 
> Note that when using rw locks a writer wanting to take the lock will
> prevent further reads from locking it, hence preventing readers from
> starving writers. Writers OTOH could starve readers, but since the
> lock is only picked in write mode by actions requested by privileged
> domains such entities already have the ability to DoS the hypervisor
> in many other ways.

While this sounds fine, my primary request was more towards removing
(or at least making less scary) the part about deadlocks.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 2/2] smp: convert cpu_hotplug_begin into a blocking lock acquisition

[Jan Beulich]

On 19.02.2020 17:26, Roger Pau Monné wrote:
> On Wed, Feb 19, 2020 at 05:06:20PM +0100, Jan Beulich wrote:
>> On 19.02.2020 16:07, Andrew Cooper wrote:
>>> On 19/02/2020 14:57, Jan Beulich wrote:
>>>> On 19.02.2020 15:45, Roger Pau Monné wrote:
>>>>> On Wed, Feb 19, 2020 at 02:44:12PM +0100, Jan Beulich wrote:
>>>>>> On 19.02.2020 14:22, Roger Pau Monné wrote:
>>>>>>> On Wed, Feb 19, 2020 at 01:59:51PM +0100, Jan Beulich wrote:
>>>>>>>> On 13.02.2020 12:32, Roger Pau Monne wrote:
>>>>>>>>> Don't allow cpu_hotplug_begin to fail by converting the trylock into a
>>>>>>>>> blocking lock acquisition. Write users of the cpu_add_remove_lock are
>>>>>>>>> limited to CPU plug/unplug operations, and cannot deadlock between
>>>>>>>>> themselves or other users taking the lock in read mode as
>>>>>>>>> cpu_add_remove_lock is always locked with interrupts enabled. There
>>>>>>>>> are also no other locks taken during the plug/unplug operations.
>>>>>>>> I don't think the goal was deadlock avoidance, but rather limiting
>>>>>>>> of the time spent spinning while trying to acquire the lock, in
>>>>>>>> favor of having the caller retry.
>>>>>>> Now that the contention between read-only users is reduced as those
>>>>>>> can take the lock in parallel I think it's safe to switch writers to
>>>>>>> blocking mode.
>>>>>> I'd agree if writers couldn't be starved by (many) readers.
>>>>> AFAICT from the rw lock implementation readers won't be able to pick
>>>>> the lock as soon as there's a writer waiting, which should avoid this
>>>>> starvation?
>>>>>
>>>>> You still need to wait for current readers to drop the lock, but no
>>>>> new readers would be able to lock it, which I think should givbe us
>>>>> enough fairness.
>>>> Ah, right, it was rather the other way around - back-to-back
>>>> writers can starve readers with our current implementation.
>>>>
>>>>> OTOH when using _trylock new readers can still pick
>>>>> the lock in read mode, and hence I think using blocking mode for
>>>>> writers is actually better, as you can assure that readers won't be
>>>>> able to starve writers.
>>>> This is a good point. Nevertheless I remain unconvinced that
>>>> the change is warranted given the original intentions (as far
>>>> as we're able to reconstruct them). If the current behavior
>>>> gets in the way of sensible shim operation, perhaps the
>>>> behavior should be made dependent upon running in shim mode?
>>>
>>> Hotplug isn't generally used at all, so there is 0 write pressure on the
>>> lock.
>>>
>>> When it is used, it is all at explicit request from the controlling
>>> entity in the system (hardware domain, or singleton shim domain).
>>>
>>> If that entity is trying to DoS you, you've already lost.
>>
>> But write pressure was never in question. My concern is with
>> how long it might take for all readers to drop their locks.
> 
> The only long running operation that takes the CPU maps read lock is
> microcode updating or livepatching, and since those are also started
> by a privileged domain I think it's safe. Any sane admin wouldn't do a
> CPU plug/unplug while updating microcode or doing a livepatching.

Ah, yes, and perhaps one can even imply that further users of
this lock would also be admin invoked (we'd have to watch out
for future violations of this principle).

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 1/2] smp: convert the cpu maps lock into a rw lock

[Jan Beulich]

On 13.02.2020 12:32, Roger Pau Monne wrote:
> Most users of the cpu maps just care about the maps not changing while
> the lock is being held, but don't actually modify the maps.
> 
> Convert the lock into a rw lock, and take the lock in read mode in
> get_cpu_maps and in write mode in cpu_hotplug_begin. This will lower
> the contention around the lock, since plug and unplug operations that
> take the lock in write mode are not that common.
> 
> Note that the read lock can be taken recursively (as it's a shared
> lock), and hence will keep the same behavior as the previously used
> recursive lock. As for the write lock, it's only used by CPU
> plug/unplug operations, and the lock is never taken recursively in
> that case.
> 
> While there also change get_cpu_maps return type to bool.
> 
> Reported-by: Julien Grall <julien@xen.org>
> Suggested-also-by: Jan Beulich <jbeulich@suse.com>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

I'm afraid I can't see how offlining a CPU would now work.
Condensed to just the relevant calls, the sequence from
cpu_down() is

cpu_hotplug_begin() (i.e. lock taken in write mode)
stop_machine_run()
-> get_cpu_maps() (lock unavailable to readers)

Other than recursive spin locks, rw locks don't currently
have a concept of permitting in a reader when this CPU
already holds the lock in write mode. Hence I can't see
how the get_cpu_maps() above would now ever succeed. Am I
missing anything, or does the patch need reverting until
the read_trylock() got enhanced to cope with this?

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 2/2] smp: convert cpu_hotplug_begin into a blocking lock acquisition

[Jan Beulich]

On 19.02.2020 18:03, Jan Beulich wrote:
> On 19.02.2020 17:08, Roger Pau Monné wrote:
>> On Wed, Feb 19, 2020 at 03:07:14PM +0000, Andrew Cooper wrote:
>>> On 19/02/2020 14:57, Jan Beulich wrote:
>>>> On 19.02.2020 15:45, Roger Pau Monné wrote:
>>>>> On Wed, Feb 19, 2020 at 02:44:12PM +0100, Jan Beulich wrote:
>>>>>> On 19.02.2020 14:22, Roger Pau Monné wrote:
>>>>>>> On Wed, Feb 19, 2020 at 01:59:51PM +0100, Jan Beulich wrote:
>>>>>>>> On 13.02.2020 12:32, Roger Pau Monne wrote:
>>>>>>>>> Don't allow cpu_hotplug_begin to fail by converting the trylock into a
>>>>>>>>> blocking lock acquisition. Write users of the cpu_add_remove_lock are
>>>>>>>>> limited to CPU plug/unplug operations, and cannot deadlock between
>>>>>>>>> themselves or other users taking the lock in read mode as
>>>>>>>>> cpu_add_remove_lock is always locked with interrupts enabled. There
>>>>>>>>> are also no other locks taken during the plug/unplug operations.
>>>>>>>> I don't think the goal was deadlock avoidance, but rather limiting
>>>>>>>> of the time spent spinning while trying to acquire the lock, in
>>>>>>>> favor of having the caller retry.
>>>>>>> Now that the contention between read-only users is reduced as those
>>>>>>> can take the lock in parallel I think it's safe to switch writers to
>>>>>>> blocking mode.
>>>>>> I'd agree if writers couldn't be starved by (many) readers.
>>>>> AFAICT from the rw lock implementation readers won't be able to pick
>>>>> the lock as soon as there's a writer waiting, which should avoid this
>>>>> starvation?
>>>>>
>>>>> You still need to wait for current readers to drop the lock, but no
>>>>> new readers would be able to lock it, which I think should givbe us
>>>>> enough fairness.
>>>> Ah, right, it was rather the other way around - back-to-back
>>>> writers can starve readers with our current implementation.
>>>>
>>>>> OTOH when using _trylock new readers can still pick
>>>>> the lock in read mode, and hence I think using blocking mode for
>>>>> writers is actually better, as you can assure that readers won't be
>>>>> able to starve writers.
>>>> This is a good point. Nevertheless I remain unconvinced that
>>>> the change is warranted given the original intentions (as far
>>>> as we're able to reconstruct them). If the current behavior
>>>> gets in the way of sensible shim operation, perhaps the
>>>> behavior should be made dependent upon running in shim mode?
>>>
>>> Hotplug isn't generally used at all, so there is 0 write pressure on the
>>> lock.
>>>
>>> When it is used, it is all at explicit request from the controlling
>>> entity in the system (hardware domain, or singleton shim domain).
>>>
>>> If that entity is trying to DoS you, you've already lost.
>>>
>>> There might be attempts to justify why the locking was done like that in
>>> the first place, but it doesn't mean they were necessarily correct to
>>> being with, and they don't match up with the realistic usage of the lock.
>>
>> I'm happy to rewrite the commit message in order to include the
>> discussion here. What about adding:
>>
>> Note that when using rw locks a writer wanting to take the lock will
>> prevent further reads from locking it, hence preventing readers from
>> starving writers. Writers OTOH could starve readers, but since the
>> lock is only picked in write mode by actions requested by privileged
>> domains such entities already have the ability to DoS the hypervisor
>> in many other ways.
> 
> While this sounds fine, my primary request was more towards removing
> (or at least making less scary) the part about deadlocks.

Actually, having thought about this some more over night, I'm fine
with the mentioning of the deadlock scenario as you have it right now.
I'm not overly fussed as to the addition (or not) of the above extra
paragraph.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 1/2] smp: convert the cpu maps lock into a rw lock

[Jürgen Groß]

On 20.02.20 09:13, Jan Beulich wrote:
> On 13.02.2020 12:32, Roger Pau Monne wrote:
>> Most users of the cpu maps just care about the maps not changing while
>> the lock is being held, but don't actually modify the maps.
>>
>> Convert the lock into a rw lock, and take the lock in read mode in
>> get_cpu_maps and in write mode in cpu_hotplug_begin. This will lower
>> the contention around the lock, since plug and unplug operations that
>> take the lock in write mode are not that common.
>>
>> Note that the read lock can be taken recursively (as it's a shared
>> lock), and hence will keep the same behavior as the previously used
>> recursive lock. As for the write lock, it's only used by CPU
>> plug/unplug operations, and the lock is never taken recursively in
>> that case.
>>
>> While there also change get_cpu_maps return type to bool.
>>
>> Reported-by: Julien Grall <julien@xen.org>
>> Suggested-also-by: Jan Beulich <jbeulich@suse.com>
>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> I'm afraid I can't see how offlining a CPU would now work.
> Condensed to just the relevant calls, the sequence from
> cpu_down() is
> 
> cpu_hotplug_begin() (i.e. lock taken in write mode)
> stop_machine_run()
> -> get_cpu_maps() (lock unavailable to readers)

I've already pointed that out in another thread. :-)

> 
> Other than recursive spin locks, rw locks don't currently
> have a concept of permitting in a reader when this CPU
> already holds the lock in write mode. Hence I can't see
> how the get_cpu_maps() above would now ever succeed. Am I
> missing anything, or does the patch need reverting until
> the read_trylock() got enhanced to cope with this?

I think this can be handled locally in get_cpu_maps() and
cpu_hotplug_begin() with the use of a variable holding the cpu (or
NR_CPUS) of the cpu holding the write lock. get_cpu_maps() can just
succeed in case this variable contains smp_processor_id().


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 1/2] smp: convert the cpu maps lock into a rw lock

[Jan Beulich]

On 20.02.2020 09:27, Jürgen Groß wrote:
> On 20.02.20 09:13, Jan Beulich wrote:
>> On 13.02.2020 12:32, Roger Pau Monne wrote:
>>> Most users of the cpu maps just care about the maps not changing while
>>> the lock is being held, but don't actually modify the maps.
>>>
>>> Convert the lock into a rw lock, and take the lock in read mode in
>>> get_cpu_maps and in write mode in cpu_hotplug_begin. This will lower
>>> the contention around the lock, since plug and unplug operations that
>>> take the lock in write mode are not that common.
>>>
>>> Note that the read lock can be taken recursively (as it's a shared
>>> lock), and hence will keep the same behavior as the previously used
>>> recursive lock. As for the write lock, it's only used by CPU
>>> plug/unplug operations, and the lock is never taken recursively in
>>> that case.
>>>
>>> While there also change get_cpu_maps return type to bool.
>>>
>>> Reported-by: Julien Grall <julien@xen.org>
>>> Suggested-also-by: Jan Beulich <jbeulich@suse.com>
>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>
>> I'm afraid I can't see how offlining a CPU would now work.
>> Condensed to just the relevant calls, the sequence from
>> cpu_down() is
>>
>> cpu_hotplug_begin() (i.e. lock taken in write mode)
>> stop_machine_run()
>> -> get_cpu_maps() (lock unavailable to readers)
> 
> I've already pointed that out in another thread. :-)

Oh, I didn't recall. Or else I wouldn't have committed the
patch in the first place.

>> Other than recursive spin locks, rw locks don't currently
>> have a concept of permitting in a reader when this CPU
>> already holds the lock in write mode. Hence I can't see
>> how the get_cpu_maps() above would now ever succeed. Am I
>> missing anything, or does the patch need reverting until
>> the read_trylock() got enhanced to cope with this?
> 
> I think this can be handled locally in get_cpu_maps() and
> cpu_hotplug_begin() with the use of a variable holding the cpu (or
> NR_CPUS) of the cpu holding the write lock. get_cpu_maps() can just
> succeed in case this variable contains smp_processor_id().

It could, yes. But this is a general shortcoming of our rw
lock implementation (and imo a trap waiting for others to
fall into as well), and hence I think it would better be
taken care of in a generic manner.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 1/2] smp: convert the cpu maps lock into a rw lock

[Julien Grall]

On 20/02/2020 08:36, Jan Beulich wrote:
> On 20.02.2020 09:27, Jürgen Groß wrote:
>> On 20.02.20 09:13, Jan Beulich wrote:
>>> On 13.02.2020 12:32, Roger Pau Monne wrote:
>>>> Most users of the cpu maps just care about the maps not changing while
>>>> the lock is being held, but don't actually modify the maps.
>>>>
>>>> Convert the lock into a rw lock, and take the lock in read mode in
>>>> get_cpu_maps and in write mode in cpu_hotplug_begin. This will lower
>>>> the contention around the lock, since plug and unplug operations that
>>>> take the lock in write mode are not that common.
>>>>
>>>> Note that the read lock can be taken recursively (as it's a shared
>>>> lock), and hence will keep the same behavior as the previously used
>>>> recursive lock. As for the write lock, it's only used by CPU
>>>> plug/unplug operations, and the lock is never taken recursively in
>>>> that case.
>>>>
>>>> While there also change get_cpu_maps return type to bool.
>>>>
>>>> Reported-by: Julien Grall <julien@xen.org>
>>>> Suggested-also-by: Jan Beulich <jbeulich@suse.com>
>>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>>
>>> I'm afraid I can't see how offlining a CPU would now work.
>>> Condensed to just the relevant calls, the sequence from
>>> cpu_down() is
>>>
>>> cpu_hotplug_begin() (i.e. lock taken in write mode)
>>> stop_machine_run()
>>> -> get_cpu_maps() (lock unavailable to readers)
>>
>> I've already pointed that out in another thread. :-)
> 
> Oh, I didn't recall. Or else I wouldn't have committed the
> patch in the first place.
> 
>>> Other than recursive spin locks, rw locks don't currently
>>> have a concept of permitting in a reader when this CPU
>>> already holds the lock in write mode. Hence I can't see
>>> how the get_cpu_maps() above would now ever succeed. Am I
>>> missing anything, or does the patch need reverting until
>>> the read_trylock() got enhanced to cope with this?
>>
>> I think this can be handled locally in get_cpu_maps() and
>> cpu_hotplug_begin() with the use of a variable holding the cpu (or
>> NR_CPUS) of the cpu holding the write lock. get_cpu_maps() can just
>> succeed in case this variable contains smp_processor_id().
> 
> It could, yes. But this is a general shortcoming of our rw
> lock implementation (and imo a trap waiting for others to
> fall into as well), and hence I think it would better be
> taken care of in a generic manner.
I actually did fall into this trap last week when playing with the p2m 
code and the emulation code. The emulation code is grabbing the write 
lock very early (which I didn't initially spot) and I was trying to use 
the read lock in a subsequent caller quite deep in the stack.

So a generic manner to solve the problem here would be ideal.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 1/2] smp: convert the cpu maps lock into a rw lock

[Roger Pau Monné]

On Thu, Feb 20, 2020 at 08:57:33AM +0000, Julien Grall wrote:
> 
> 
> On 20/02/2020 08:36, Jan Beulich wrote:
> > On 20.02.2020 09:27, Jürgen Groß wrote:
> > > On 20.02.20 09:13, Jan Beulich wrote:
> > > > On 13.02.2020 12:32, Roger Pau Monne wrote:
> > > > > Most users of the cpu maps just care about the maps not changing while
> > > > > the lock is being held, but don't actually modify the maps.
> > > > > 
> > > > > Convert the lock into a rw lock, and take the lock in read mode in
> > > > > get_cpu_maps and in write mode in cpu_hotplug_begin. This will lower
> > > > > the contention around the lock, since plug and unplug operations that
> > > > > take the lock in write mode are not that common.
> > > > > 
> > > > > Note that the read lock can be taken recursively (as it's a shared
> > > > > lock), and hence will keep the same behavior as the previously used
> > > > > recursive lock. As for the write lock, it's only used by CPU
> > > > > plug/unplug operations, and the lock is never taken recursively in
> > > > > that case.
> > > > > 
> > > > > While there also change get_cpu_maps return type to bool.
> > > > > 
> > > > > Reported-by: Julien Grall <julien@xen.org>
> > > > > Suggested-also-by: Jan Beulich <jbeulich@suse.com>
> > > > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > > > 
> > > > I'm afraid I can't see how offlining a CPU would now work.
> > > > Condensed to just the relevant calls, the sequence from
> > > > cpu_down() is
> > > > 
> > > > cpu_hotplug_begin() (i.e. lock taken in write mode)
> > > > stop_machine_run()
> > > > -> get_cpu_maps() (lock unavailable to readers)
> > > 
> > > I've already pointed that out in another thread. :-)
> > 
> > Oh, I didn't recall. Or else I wouldn't have committed the
> > patch in the first place.
> > 
> > > > Other than recursive spin locks, rw locks don't currently
> > > > have a concept of permitting in a reader when this CPU
> > > > already holds the lock in write mode. Hence I can't see
> > > > how the get_cpu_maps() above would now ever succeed. Am I
> > > > missing anything, or does the patch need reverting until
> > > > the read_trylock() got enhanced to cope with this?
> > > 
> > > I think this can be handled locally in get_cpu_maps() and
> > > cpu_hotplug_begin() with the use of a variable holding the cpu (or
> > > NR_CPUS) of the cpu holding the write lock. get_cpu_maps() can just
> > > succeed in case this variable contains smp_processor_id().
> > 
> > It could, yes. But this is a general shortcoming of our rw
> > lock implementation (and imo a trap waiting for others to
> > fall into as well), and hence I think it would better be
> > taken care of in a generic manner.
> I actually did fall into this trap last week when playing with the p2m code
> and the emulation code. The emulation code is grabbing the write lock very
> early (which I didn't initially spot) and I was trying to use the read lock
> in a subsequent caller quite deep in the stack.
> 
> So a generic manner to solve the problem here would be ideal.

Let me take a look, it doesn't seem very convoluted to adapt some of
the recursive logic to be used in a rw lock.

Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 2/2] smp: convert cpu_hotplug_begin into a blocking lock acquisition

[Roger Pau Monné]

On Thu, Feb 20, 2020 at 09:16:48AM +0100, Jan Beulich wrote:
> On 19.02.2020 18:03, Jan Beulich wrote:
> > On 19.02.2020 17:08, Roger Pau Monné wrote:
> >> On Wed, Feb 19, 2020 at 03:07:14PM +0000, Andrew Cooper wrote:
> >>> On 19/02/2020 14:57, Jan Beulich wrote:
> >>>> On 19.02.2020 15:45, Roger Pau Monné wrote:
> >>>>> On Wed, Feb 19, 2020 at 02:44:12PM +0100, Jan Beulich wrote:
> >>>>>> On 19.02.2020 14:22, Roger Pau Monné wrote:
> >>>>>>> On Wed, Feb 19, 2020 at 01:59:51PM +0100, Jan Beulich wrote:
> >>>>>>>> On 13.02.2020 12:32, Roger Pau Monne wrote:
> >>>>>>>>> Don't allow cpu_hotplug_begin to fail by converting the trylock into a
> >>>>>>>>> blocking lock acquisition. Write users of the cpu_add_remove_lock are
> >>>>>>>>> limited to CPU plug/unplug operations, and cannot deadlock between
> >>>>>>>>> themselves or other users taking the lock in read mode as
> >>>>>>>>> cpu_add_remove_lock is always locked with interrupts enabled. There
> >>>>>>>>> are also no other locks taken during the plug/unplug operations.
> >>>>>>>> I don't think the goal was deadlock avoidance, but rather limiting
> >>>>>>>> of the time spent spinning while trying to acquire the lock, in
> >>>>>>>> favor of having the caller retry.
> >>>>>>> Now that the contention between read-only users is reduced as those
> >>>>>>> can take the lock in parallel I think it's safe to switch writers to
> >>>>>>> blocking mode.
> >>>>>> I'd agree if writers couldn't be starved by (many) readers.
> >>>>> AFAICT from the rw lock implementation readers won't be able to pick
> >>>>> the lock as soon as there's a writer waiting, which should avoid this
> >>>>> starvation?
> >>>>>
> >>>>> You still need to wait for current readers to drop the lock, but no
> >>>>> new readers would be able to lock it, which I think should givbe us
> >>>>> enough fairness.
> >>>> Ah, right, it was rather the other way around - back-to-back
> >>>> writers can starve readers with our current implementation.
> >>>>
> >>>>> OTOH when using _trylock new readers can still pick
> >>>>> the lock in read mode, and hence I think using blocking mode for
> >>>>> writers is actually better, as you can assure that readers won't be
> >>>>> able to starve writers.
> >>>> This is a good point. Nevertheless I remain unconvinced that
> >>>> the change is warranted given the original intentions (as far
> >>>> as we're able to reconstruct them). If the current behavior
> >>>> gets in the way of sensible shim operation, perhaps the
> >>>> behavior should be made dependent upon running in shim mode?
> >>>
> >>> Hotplug isn't generally used at all, so there is 0 write pressure on the
> >>> lock.
> >>>
> >>> When it is used, it is all at explicit request from the controlling
> >>> entity in the system (hardware domain, or singleton shim domain).
> >>>
> >>> If that entity is trying to DoS you, you've already lost.
> >>>
> >>> There might be attempts to justify why the locking was done like that in
> >>> the first place, but it doesn't mean they were necessarily correct to
> >>> being with, and they don't match up with the realistic usage of the lock.
> >>
> >> I'm happy to rewrite the commit message in order to include the
> >> discussion here. What about adding:
> >>
> >> Note that when using rw locks a writer wanting to take the lock will
> >> prevent further reads from locking it, hence preventing readers from
> >> starving writers. Writers OTOH could starve readers, but since the
> >> lock is only picked in write mode by actions requested by privileged
> >> domains such entities already have the ability to DoS the hypervisor
> >> in many other ways.
> > 
> > While this sounds fine, my primary request was more towards removing
> > (or at least making less scary) the part about deadlocks.
> 
> Actually, having thought about this some more over night, I'm fine
> with the mentioning of the deadlock scenario as you have it right now.
> I'm not overly fussed as to the addition (or not) of the above extra
> paragraph.

Up to you, I don't have a strong opinion.

AFAICT there's no need for me to resend then?

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 2/2] smp: convert cpu_hotplug_begin into a blocking lock acquisition

[Jan Beulich]

On 21.02.2020 11:23, Roger Pau Monné wrote:
> On Thu, Feb 20, 2020 at 09:16:48AM +0100, Jan Beulich wrote:
>> On 19.02.2020 18:03, Jan Beulich wrote:
>>> On 19.02.2020 17:08, Roger Pau Monné wrote:
>>>> On Wed, Feb 19, 2020 at 03:07:14PM +0000, Andrew Cooper wrote:
>>>>> On 19/02/2020 14:57, Jan Beulich wrote:
>>>>>> On 19.02.2020 15:45, Roger Pau Monné wrote:
>>>>>>> On Wed, Feb 19, 2020 at 02:44:12PM +0100, Jan Beulich wrote:
>>>>>>>> On 19.02.2020 14:22, Roger Pau Monné wrote:
>>>>>>>>> On Wed, Feb 19, 2020 at 01:59:51PM +0100, Jan Beulich wrote:
>>>>>>>>>> On 13.02.2020 12:32, Roger Pau Monne wrote:
>>>>>>>>>>> Don't allow cpu_hotplug_begin to fail by converting the trylock into a
>>>>>>>>>>> blocking lock acquisition. Write users of the cpu_add_remove_lock are
>>>>>>>>>>> limited to CPU plug/unplug operations, and cannot deadlock between
>>>>>>>>>>> themselves or other users taking the lock in read mode as
>>>>>>>>>>> cpu_add_remove_lock is always locked with interrupts enabled. There
>>>>>>>>>>> are also no other locks taken during the plug/unplug operations.
>>>>>>>>>> I don't think the goal was deadlock avoidance, but rather limiting
>>>>>>>>>> of the time spent spinning while trying to acquire the lock, in
>>>>>>>>>> favor of having the caller retry.
>>>>>>>>> Now that the contention between read-only users is reduced as those
>>>>>>>>> can take the lock in parallel I think it's safe to switch writers to
>>>>>>>>> blocking mode.
>>>>>>>> I'd agree if writers couldn't be starved by (many) readers.
>>>>>>> AFAICT from the rw lock implementation readers won't be able to pick
>>>>>>> the lock as soon as there's a writer waiting, which should avoid this
>>>>>>> starvation?
>>>>>>>
>>>>>>> You still need to wait for current readers to drop the lock, but no
>>>>>>> new readers would be able to lock it, which I think should givbe us
>>>>>>> enough fairness.
>>>>>> Ah, right, it was rather the other way around - back-to-back
>>>>>> writers can starve readers with our current implementation.
>>>>>>
>>>>>>> OTOH when using _trylock new readers can still pick
>>>>>>> the lock in read mode, and hence I think using blocking mode for
>>>>>>> writers is actually better, as you can assure that readers won't be
>>>>>>> able to starve writers.
>>>>>> This is a good point. Nevertheless I remain unconvinced that
>>>>>> the change is warranted given the original intentions (as far
>>>>>> as we're able to reconstruct them). If the current behavior
>>>>>> gets in the way of sensible shim operation, perhaps the
>>>>>> behavior should be made dependent upon running in shim mode?
>>>>>
>>>>> Hotplug isn't generally used at all, so there is 0 write pressure on the
>>>>> lock.
>>>>>
>>>>> When it is used, it is all at explicit request from the controlling
>>>>> entity in the system (hardware domain, or singleton shim domain).
>>>>>
>>>>> If that entity is trying to DoS you, you've already lost.
>>>>>
>>>>> There might be attempts to justify why the locking was done like that in
>>>>> the first place, but it doesn't mean they were necessarily correct to
>>>>> being with, and they don't match up with the realistic usage of the lock.
>>>>
>>>> I'm happy to rewrite the commit message in order to include the
>>>> discussion here. What about adding:
>>>>
>>>> Note that when using rw locks a writer wanting to take the lock will
>>>> prevent further reads from locking it, hence preventing readers from
>>>> starving writers. Writers OTOH could starve readers, but since the
>>>> lock is only picked in write mode by actions requested by privileged
>>>> domains such entities already have the ability to DoS the hypervisor
>>>> in many other ways.
>>>
>>> While this sounds fine, my primary request was more towards removing
>>> (or at least making less scary) the part about deadlocks.
>>
>> Actually, having thought about this some more over night, I'm fine
>> with the mentioning of the deadlock scenario as you have it right now.
>> I'm not overly fussed as to the addition (or not) of the above extra
>> paragraph.
> 
> Up to you, I don't have a strong opinion.
> 
> AFAICT there's no need for me to resend then?

Indeed, but I wouldn't want to apply this one until the regression
from patch 1 was fixed, as else that change may also still get
reverted. But I'll keep it queued for committing until such time.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel