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

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

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

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

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

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

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

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

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

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

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