[Xen-devel] [PATCH 0/2] xen/rcu: let rcu work better with core scheduling

[Xen-devel] [PATCH 0/2] xen/rcu: let rcu work better with core scheduling

[Juergen Gross]

Today the RCU handling in Xen is affecting scheduling in several ways.
It is raising sched softirqs without any real need and it requires
tasklets for rcu_barrier(), which interacts badly with core scheduling.

This small series repairs those issues.

Juergen Gross (2):
  xen/rcu: use rcu softirq for forcing quiescent state
  xen/rcu: don't use stop_machine_run() for rcu_barrier()

 xen/common/rcupdate.c      | 69 ++++++++++++++++++++++++++++++----------------
 xen/include/xen/rcupdate.h |  2 +-
 2 files changed, 46 insertions(+), 25 deletions(-)

-- 
2.16.4


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

[Xen-devel] [PATCH 1/2] xen/rcu: use rcu softirq for forcing quiescent state

[Juergen Gross]

As rcu callbacks are processed in __do_softirq() there is no need to
use the scheduling softirq for forcing quiescent state. Any other
softirq would do the job and the scheduling one is the most expensive.

So use the already existing rcu softirq for that purpose. For telling
apart why the rcu softirq was raised add a flag for the current usage.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 xen/common/rcupdate.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/xen/common/rcupdate.c b/xen/common/rcupdate.c
index 91d4ad0fd8..079ea9d8a1 100644
--- a/xen/common/rcupdate.c
+++ b/xen/common/rcupdate.c
@@ -89,6 +89,8 @@ struct rcu_data {
     /* 3) idle CPUs handling */
     struct timer idle_timer;
     bool idle_timer_active;
+
+    bool            process_callbacks;
 };
 
 /*
@@ -194,7 +196,7 @@ static void force_quiescent_state(struct rcu_data *rdp,
                                   struct rcu_ctrlblk *rcp)
 {
     cpumask_t cpumask;
-    raise_softirq(SCHEDULE_SOFTIRQ);
+    raise_softirq(RCU_SOFTIRQ);
     if (unlikely(rdp->qlen - rdp->last_rs_qlen > rsinterval)) {
         rdp->last_rs_qlen = rdp->qlen;
         /*
@@ -202,7 +204,7 @@ static void force_quiescent_state(struct rcu_data *rdp,
          * rdp->cpu is the current cpu.
          */
         cpumask_andnot(&cpumask, &rcp->cpumask, cpumask_of(rdp->cpu));
-        cpumask_raise_softirq(&cpumask, SCHEDULE_SOFTIRQ);
+        cpumask_raise_softirq(&cpumask, RCU_SOFTIRQ);
     }
 }
 
@@ -259,7 +261,10 @@ static void rcu_do_batch(struct rcu_data *rdp)
     if (!rdp->donelist)
         rdp->donetail = &rdp->donelist;
     else
+    {
+        rdp->process_callbacks = true;
         raise_softirq(RCU_SOFTIRQ);
+    }
 }
 
 /*
@@ -410,7 +415,13 @@ static void __rcu_process_callbacks(struct rcu_ctrlblk *rcp,
 
 static void rcu_process_callbacks(void)
 {
-    __rcu_process_callbacks(&rcu_ctrlblk, &this_cpu(rcu_data));
+    struct rcu_data *rdp = &this_cpu(rcu_data);
+
+    if ( rdp->process_callbacks )
+    {
+        rdp->process_callbacks = false;
+        __rcu_process_callbacks(&rcu_ctrlblk, rdp);
+    }
 }
 
 static int __rcu_pending(struct rcu_ctrlblk *rcp, struct rcu_data *rdp)
@@ -518,6 +529,9 @@ static void rcu_idle_timer_handler(void* data)
 
 void rcu_check_callbacks(int cpu)
 {
+    struct rcu_data *rdp = &this_cpu(rcu_data);
+
+    rdp->process_callbacks = true;
     raise_softirq(RCU_SOFTIRQ);
 }
 
-- 
2.16.4


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

[Xen-devel] [PATCH 2/2] xen/rcu: don't use stop_machine_run() for rcu_barrier()

[Juergen Gross]

Today rcu_barrier() is calling stop_machine_run() to synchronize all
physical cpus in order to ensure all pending rcu calls have finished
when returning.

As stop_machine_run() is using tasklets this requires scheduling of
idle vcpus on all cpus imposing the need to call rcu_barrier() on idle
cpus only in case of core scheduling being active, as otherwise a
scheduling deadlock would occur.

There is no need at all to do the syncing of the cpus in tasklets, as
rcu activity is started in __do_softirq() called whenever softirq
activity is allowed. So rcu_barrier() can easily be modified to use
softirq for synchronization of the cpus no longer requiring any
scheduling activity.

As there already is a rcu softirq reuse that for the synchronization.

Finally switch rcu_barrier() to return void as it now can never fail.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 xen/common/rcupdate.c      | 49 ++++++++++++++++++++++++++--------------------
 xen/include/xen/rcupdate.h |  2 +-
 2 files changed, 29 insertions(+), 22 deletions(-)

diff --git a/xen/common/rcupdate.c b/xen/common/rcupdate.c
index 079ea9d8a1..1f02a804e3 100644
--- a/xen/common/rcupdate.c
+++ b/xen/common/rcupdate.c
@@ -143,47 +143,51 @@ static int qhimark = 10000;
 static int qlowmark = 100;
 static int rsinterval = 1000;
 
-struct rcu_barrier_data {
-    struct rcu_head head;
-    atomic_t *cpu_count;
-};
+/*
+ * rcu_barrier() handling:
+ * cpu_count holds the number of cpu required to finish barrier handling.
+ * Cpus are synchronized via softirq mechanism. rcu_barrier() is regarded to
+ * be active if cpu_count is not zero. In case rcu_barrier() is called on
+ * multiple cpus it is enough to check for cpu_count being not zero on entry
+ * and to call process_pending_softirqs() in a loop until cpu_count drops to
+ * zero, as syncing has been requested already and we don't need to sync
+ * multiple times.
+ */
+static atomic_t cpu_count = ATOMIC_INIT(0);
 
 static void rcu_barrier_callback(struct rcu_head *head)
 {
-    struct rcu_barrier_data *data = container_of(
-        head, struct rcu_barrier_data, head);
-    atomic_inc(data->cpu_count);
+    atomic_dec(&cpu_count);
 }
 
-static int rcu_barrier_action(void *_cpu_count)
+static void rcu_barrier_action(void)
 {
-    struct rcu_barrier_data data = { .cpu_count = _cpu_count };
-
-    ASSERT(!local_irq_is_enabled());
-    local_irq_enable();
+    struct rcu_head head;
 
     /*
      * When callback is executed, all previously-queued RCU work on this CPU
      * is completed. When all CPUs have executed their callback, data.cpu_count
      * will have been incremented to include every online CPU.
      */
-    call_rcu(&data.head, rcu_barrier_callback);
+    call_rcu(&head, rcu_barrier_callback);
 
-    while ( atomic_read(data.cpu_count) != num_online_cpus() )
+    while ( atomic_read(&cpu_count) )
     {
         process_pending_softirqs();
         cpu_relax();
     }
-
-    local_irq_disable();
-
-    return 0;
 }
 
-int rcu_barrier(void)
+void rcu_barrier(void)
 {
-    atomic_t cpu_count = ATOMIC_INIT(0);
-    return stop_machine_run(rcu_barrier_action, &cpu_count, NR_CPUS);
+    if ( !atomic_cmpxchg(&cpu_count, 0, num_online_cpus()) )
+        cpumask_raise_softirq(&cpu_online_map, RCU_SOFTIRQ);
+
+    while ( atomic_read(&cpu_count) )
+    {
+        process_pending_softirqs();
+        cpu_relax();
+    }
 }
 
 /* Is batch a before batch b ? */
@@ -422,6 +426,9 @@ static void rcu_process_callbacks(void)
         rdp->process_callbacks = false;
         __rcu_process_callbacks(&rcu_ctrlblk, rdp);
     }
+
+    if ( atomic_read(&cpu_count) )
+        rcu_barrier_action();
 }
 
 static int __rcu_pending(struct rcu_ctrlblk *rcp, struct rcu_data *rdp)
diff --git a/xen/include/xen/rcupdate.h b/xen/include/xen/rcupdate.h
index 174d058113..87f35b7704 100644
--- a/xen/include/xen/rcupdate.h
+++ b/xen/include/xen/rcupdate.h
@@ -143,7 +143,7 @@ void rcu_check_callbacks(int cpu);
 void call_rcu(struct rcu_head *head, 
               void (*func)(struct rcu_head *head));
 
-int rcu_barrier(void);
+void rcu_barrier(void);
 
 void rcu_idle_enter(unsigned int cpu);
 void rcu_idle_exit(unsigned int cpu);
-- 
2.16.4


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

Re: [Xen-devel] [PATCH 2/2] xen/rcu: don't use stop_machine_run() for rcu_barrier()

[Julien Grall]

Hi Juergen,

On 17/02/2020 07:20, Juergen Gross wrote:
> Today rcu_barrier() is calling stop_machine_run() to synchronize all
> physical cpus in order to ensure all pending rcu calls have finished
> when returning.
> 
> As stop_machine_run() is using tasklets this requires scheduling of
> idle vcpus on all cpus imposing the need to call rcu_barrier() on idle
> cpus only in case of core scheduling being active, as otherwise a
> scheduling deadlock would occur.
> 
> There is no need at all to do the syncing of the cpus in tasklets, as
> rcu activity is started in __do_softirq() called whenever softirq
> activity is allowed. So rcu_barrier() can easily be modified to use
> softirq for synchronization of the cpus no longer requiring any
> scheduling activity.
> 
> As there already is a rcu softirq reuse that for the synchronization.
> 
> Finally switch rcu_barrier() to return void as it now can never fail.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
>   xen/common/rcupdate.c      | 49 ++++++++++++++++++++++++++--------------------
>   xen/include/xen/rcupdate.h |  2 +-
>   2 files changed, 29 insertions(+), 22 deletions(-)
> 
> diff --git a/xen/common/rcupdate.c b/xen/common/rcupdate.c
> index 079ea9d8a1..1f02a804e3 100644
> --- a/xen/common/rcupdate.c
> +++ b/xen/common/rcupdate.c
> @@ -143,47 +143,51 @@ static int qhimark = 10000;
>   static int qlowmark = 100;
>   static int rsinterval = 1000;
>   
> -struct rcu_barrier_data {
> -    struct rcu_head head;
> -    atomic_t *cpu_count;
> -};
> +/*
> + * rcu_barrier() handling:
> + * cpu_count holds the number of cpu required to finish barrier handling.
> + * Cpus are synchronized via softirq mechanism. rcu_barrier() is regarded to
> + * be active if cpu_count is not zero. In case rcu_barrier() is called on
> + * multiple cpus it is enough to check for cpu_count being not zero on entry
> + * and to call process_pending_softirqs() in a loop until cpu_count drops to
> + * zero, as syncing has been requested already and we don't need to sync
> + * multiple times.
> + */
> +static atomic_t cpu_count = ATOMIC_INIT(0);
>   
>   static void rcu_barrier_callback(struct rcu_head *head)
>   {
> -    struct rcu_barrier_data *data = container_of(
> -        head, struct rcu_barrier_data, head);
> -    atomic_inc(data->cpu_count);
> +    atomic_dec(&cpu_count);
>   }
>   
> -static int rcu_barrier_action(void *_cpu_count)
> +static void rcu_barrier_action(void)
>   {
> -    struct rcu_barrier_data data = { .cpu_count = _cpu_count };
> -
> -    ASSERT(!local_irq_is_enabled());
> -    local_irq_enable();
> +    struct rcu_head head;
>   
>       /*
>        * When callback is executed, all previously-queued RCU work on this CPU
>        * is completed. When all CPUs have executed their callback, data.cpu_count
>        * will have been incremented to include every online CPU.
>        */
> -    call_rcu(&data.head, rcu_barrier_callback);
> +    call_rcu(&head, rcu_barrier_callback);
>   
> -    while ( atomic_read(data.cpu_count) != num_online_cpus() )
> +    while ( atomic_read(&cpu_count) )
>       {
>           process_pending_softirqs();
>           cpu_relax();
>       }
> -
> -    local_irq_disable();
> -
> -    return 0;
>   }
>   
> -int rcu_barrier(void)
> +void rcu_barrier(void)
>   {
> -    atomic_t cpu_count = ATOMIC_INIT(0);
> -    return stop_machine_run(rcu_barrier_action, &cpu_count, NR_CPUS);
> +    if ( !atomic_cmpxchg(&cpu_count, 0, num_online_cpus()) )

What does prevent the cpu_online_map to change under your feet? 
Shouldn't you grab the lock via get_cpu_maps()?

> +        cpumask_raise_softirq(&cpu_online_map, RCU_SOFTIRQ);
> +
> +    while ( atomic_read(&cpu_count) )
> +    {
> +        process_pending_softirqs();
> +        cpu_relax();
> +    }
>   }
>   
>   /* Is batch a before batch b ? */
> @@ -422,6 +426,9 @@ static void rcu_process_callbacks(void)
>           rdp->process_callbacks = false;
>           __rcu_process_callbacks(&rcu_ctrlblk, rdp);
>       }
> +
> +    if ( atomic_read(&cpu_count) )
> +        rcu_barrier_action();
>   }
>   
>   static int __rcu_pending(struct rcu_ctrlblk *rcp, struct rcu_data *rdp)
> diff --git a/xen/include/xen/rcupdate.h b/xen/include/xen/rcupdate.h
> index 174d058113..87f35b7704 100644
> --- a/xen/include/xen/rcupdate.h
> +++ b/xen/include/xen/rcupdate.h
> @@ -143,7 +143,7 @@ void rcu_check_callbacks(int cpu);
>   void call_rcu(struct rcu_head *head,
>                 void (*func)(struct rcu_head *head));
>   
> -int rcu_barrier(void);
> +void rcu_barrier(void);
>   
>   void rcu_idle_enter(unsigned int cpu);
>   void rcu_idle_exit(unsigned int cpu);
> 

Cheers,

-- 
Julien Grall

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

Re: [Xen-devel] [PATCH 2/2] xen/rcu: don't use stop_machine_run() for rcu_barrier()

[Jürgen Groß]

On 17.02.20 12:49, Julien Grall wrote:
> Hi Juergen,
> 
> On 17/02/2020 07:20, Juergen Gross wrote:
>> +void rcu_barrier(void)
>>   {
>> -    atomic_t cpu_count = ATOMIC_INIT(0);
>> -    return stop_machine_run(rcu_barrier_action, &cpu_count, NR_CPUS);
>> +    if ( !atomic_cmpxchg(&cpu_count, 0, num_online_cpus()) )
> 
> What does prevent the cpu_online_map to change under your feet? 
> Shouldn't you grab the lock via get_cpu_maps()?

Oh, indeed.

This in turn will require a modification of the logic to detect parallel
calls on multiple cpus.


Juergen

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

Re: [Xen-devel] [PATCH 2/2] xen/rcu: don't use stop_machine_run() for rcu_barrier()

[Roger Pau Monné]

On Mon, Feb 17, 2020 at 01:11:59PM +0100, Jürgen Groß wrote:
> On 17.02.20 12:49, Julien Grall wrote:
> > Hi Juergen,
> > 
> > On 17/02/2020 07:20, Juergen Gross wrote:
> > > +void rcu_barrier(void)
> > >   {
> > > -    atomic_t cpu_count = ATOMIC_INIT(0);
> > > -    return stop_machine_run(rcu_barrier_action, &cpu_count, NR_CPUS);
> > > +    if ( !atomic_cmpxchg(&cpu_count, 0, num_online_cpus()) )
> > 
> > What does prevent the cpu_online_map to change under your feet?
> > Shouldn't you grab the lock via get_cpu_maps()?
> 
> Oh, indeed.
> 
> This in turn will require a modification of the logic to detect parallel
> calls on multiple cpus.

If you pick my patch to turn that into a rw lock you shouldn't worry
about parallel calls I think, but the lock acquisition can still fail
if there's a CPU plug/unplug going on:

https://lists.xenproject.org/archives/html/xen-devel/2020-02/msg00940.html

Roger.

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

Re: [Xen-devel] [PATCH 2/2] xen/rcu: don't use stop_machine_run() for rcu_barrier()

[Igor Druzhinin]

On 17/02/2020 07:20, Juergen Gross wrote:
> Today rcu_barrier() is calling stop_machine_run() to synchronize all
> physical cpus in order to ensure all pending rcu calls have finished
> when returning.
> 
> As stop_machine_run() is using tasklets this requires scheduling of
> idle vcpus on all cpus imposing the need to call rcu_barrier() on idle
> cpus only in case of core scheduling being active, as otherwise a
> scheduling deadlock would occur.
> 
> There is no need at all to do the syncing of the cpus in tasklets, as
> rcu activity is started in __do_softirq() called whenever softirq
> activity is allowed. So rcu_barrier() can easily be modified to use
> softirq for synchronization of the cpus no longer requiring any
> scheduling activity.
> 
> As there already is a rcu softirq reuse that for the synchronization.
> 
> Finally switch rcu_barrier() to return void as it now can never fail.
> 

Would this implementation guarantee progress as previous implementation
guaranteed?

Igor


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

Re: [Xen-devel] [PATCH 2/2] xen/rcu: don't use stop_machine_run() for rcu_barrier()

[Jürgen Groß]

On 17.02.20 13:26, Igor Druzhinin wrote:
> On 17/02/2020 07:20, Juergen Gross wrote:
>> Today rcu_barrier() is calling stop_machine_run() to synchronize all
>> physical cpus in order to ensure all pending rcu calls have finished
>> when returning.
>>
>> As stop_machine_run() is using tasklets this requires scheduling of
>> idle vcpus on all cpus imposing the need to call rcu_barrier() on idle
>> cpus only in case of core scheduling being active, as otherwise a
>> scheduling deadlock would occur.
>>
>> There is no need at all to do the syncing of the cpus in tasklets, as
>> rcu activity is started in __do_softirq() called whenever softirq
>> activity is allowed. So rcu_barrier() can easily be modified to use
>> softirq for synchronization of the cpus no longer requiring any
>> scheduling activity.
>>
>> As there already is a rcu softirq reuse that for the synchronization.
>>
>> Finally switch rcu_barrier() to return void as it now can never fail.
>>
> 
> Would this implementation guarantee progress as previous implementation
> guaranteed?

Yes.


Juergen

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

Re: [Xen-devel] [PATCH 2/2] xen/rcu: don't use stop_machine_run() for rcu_barrier()

[Igor Druzhinin]

On 17/02/2020 12:28, Jürgen Groß wrote:
> On 17.02.20 13:26, Igor Druzhinin wrote:
>> On 17/02/2020 07:20, Juergen Gross wrote:
>>> Today rcu_barrier() is calling stop_machine_run() to synchronize all
>>> physical cpus in order to ensure all pending rcu calls have finished
>>> when returning.
>>>
>>> As stop_machine_run() is using tasklets this requires scheduling of
>>> idle vcpus on all cpus imposing the need to call rcu_barrier() on idle
>>> cpus only in case of core scheduling being active, as otherwise a
>>> scheduling deadlock would occur.
>>>
>>> There is no need at all to do the syncing of the cpus in tasklets, as
>>> rcu activity is started in __do_softirq() called whenever softirq
>>> activity is allowed. So rcu_barrier() can easily be modified to use
>>> softirq for synchronization of the cpus no longer requiring any
>>> scheduling activity.
>>>
>>> As there already is a rcu softirq reuse that for the synchronization.
>>>
>>> Finally switch rcu_barrier() to return void as it now can never fail.
>>>
>>
>> Would this implementation guarantee progress as previous implementation
>> guaranteed?
> 
> Yes.

Thanks, I'll put it to test today to see if it solves our use case.

Igor

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

Re: [Xen-devel] [PATCH 2/2] xen/rcu: don't use stop_machine_run() for rcu_barrier()

[Jürgen Groß]

On 17.02.20 13:17, Roger Pau Monné wrote:
> On Mon, Feb 17, 2020 at 01:11:59PM +0100, Jürgen Groß wrote:
>> On 17.02.20 12:49, Julien Grall wrote:
>>> Hi Juergen,
>>>
>>> On 17/02/2020 07:20, Juergen Gross wrote:
>>>> +void rcu_barrier(void)
>>>>    {
>>>> -    atomic_t cpu_count = ATOMIC_INIT(0);
>>>> -    return stop_machine_run(rcu_barrier_action, &cpu_count, NR_CPUS);
>>>> +    if ( !atomic_cmpxchg(&cpu_count, 0, num_online_cpus()) )
>>>
>>> What does prevent the cpu_online_map to change under your feet?
>>> Shouldn't you grab the lock via get_cpu_maps()?
>>
>> Oh, indeed.
>>
>> This in turn will require a modification of the logic to detect parallel
>> calls on multiple cpus.
> 
> If you pick my patch to turn that into a rw lock you shouldn't worry
> about parallel calls I think, but the lock acquisition can still fail
> if there's a CPU plug/unplug going on:
> 
> https://lists.xenproject.org/archives/html/xen-devel/2020-02/msg00940.html

Thanks, but letting rcu_barrier() fail is a no go, so I still need to
handle that case (I mean the case failing to get the lock). And handling
of parallel calls is not needed to be functional correct, but to avoid
not necessary cpu synchronization (each parallel call detected can just
wait until the master has finished and then return).

BTW - The recursive spinlock today would allow for e.g. rcu_barrier() to
be called inside a CPU plug/unplug section. Your rwlock is removing that
possibility. Any chance that could be handled?


Juergen

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

Re: [Xen-devel] [PATCH 2/2] xen/rcu: don't use stop_machine_run() for rcu_barrier()

[Roger Pau Monné]

On Mon, Feb 17, 2020 at 01:32:59PM +0100, Jürgen Groß wrote:
> On 17.02.20 13:17, Roger Pau Monné wrote:
> > On Mon, Feb 17, 2020 at 01:11:59PM +0100, Jürgen Groß wrote:
> > > On 17.02.20 12:49, Julien Grall wrote:
> > > > Hi Juergen,
> > > > 
> > > > On 17/02/2020 07:20, Juergen Gross wrote:
> > > > > +void rcu_barrier(void)
> > > > >    {
> > > > > -    atomic_t cpu_count = ATOMIC_INIT(0);
> > > > > -    return stop_machine_run(rcu_barrier_action, &cpu_count, NR_CPUS);
> > > > > +    if ( !atomic_cmpxchg(&cpu_count, 0, num_online_cpus()) )
> > > > 
> > > > What does prevent the cpu_online_map to change under your feet?
> > > > Shouldn't you grab the lock via get_cpu_maps()?
> > > 
> > > Oh, indeed.
> > > 
> > > This in turn will require a modification of the logic to detect parallel
> > > calls on multiple cpus.
> > 
> > If you pick my patch to turn that into a rw lock you shouldn't worry
> > about parallel calls I think, but the lock acquisition can still fail
> > if there's a CPU plug/unplug going on:
> > 
> > https://lists.xenproject.org/archives/html/xen-devel/2020-02/msg00940.html
> 
> Thanks, but letting rcu_barrier() fail is a no go, so I still need to
> handle that case (I mean the case failing to get the lock). And handling
> of parallel calls is not needed to be functional correct, but to avoid
> not necessary cpu synchronization (each parallel call detected can just
> wait until the master has finished and then return).
>
> BTW - The recursive spinlock today would allow for e.g. rcu_barrier() to
> be called inside a CPU plug/unplug section. Your rwlock is removing that
> possibility. Any chance that could be handled?

While this might be interesting for the rcu stuff, it certainly isn't
for other pieces also relying on the cpu maps lock.

Ie: get_cpu_maps must fail when called by send_IPI_mask if there's a
CPU plug/unplug operation going on, even if it's on the same pCPU
that's holding the lock in write mode.

I guess you could add a pCPU variable to record whether the current
pCPU is in the middle of a CPU plug/unplug operation (and hence has
the maps locked in write mode) and avoid taking the lock in that case?

Roger.

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

Re: [Xen-devel] [PATCH 2/2] xen/rcu: don't use stop_machine_run() for rcu_barrier()

[Jürgen Groß]

On 17.02.20 13:49, Roger Pau Monné wrote:
> On Mon, Feb 17, 2020 at 01:32:59PM +0100, Jürgen Groß wrote:
>> On 17.02.20 13:17, Roger Pau Monné wrote:
>>> On Mon, Feb 17, 2020 at 01:11:59PM +0100, Jürgen Groß wrote:
>>>> On 17.02.20 12:49, Julien Grall wrote:
>>>>> Hi Juergen,
>>>>>
>>>>> On 17/02/2020 07:20, Juergen Gross wrote:
>>>>>> +void rcu_barrier(void)
>>>>>>     {
>>>>>> -    atomic_t cpu_count = ATOMIC_INIT(0);
>>>>>> -    return stop_machine_run(rcu_barrier_action, &cpu_count, NR_CPUS);
>>>>>> +    if ( !atomic_cmpxchg(&cpu_count, 0, num_online_cpus()) )
>>>>>
>>>>> What does prevent the cpu_online_map to change under your feet?
>>>>> Shouldn't you grab the lock via get_cpu_maps()?
>>>>
>>>> Oh, indeed.
>>>>
>>>> This in turn will require a modification of the logic to detect parallel
>>>> calls on multiple cpus.
>>>
>>> If you pick my patch to turn that into a rw lock you shouldn't worry
>>> about parallel calls I think, but the lock acquisition can still fail
>>> if there's a CPU plug/unplug going on:
>>>
>>> https://lists.xenproject.org/archives/html/xen-devel/2020-02/msg00940.html
>>
>> Thanks, but letting rcu_barrier() fail is a no go, so I still need to
>> handle that case (I mean the case failing to get the lock). And handling
>> of parallel calls is not needed to be functional correct, but to avoid
>> not necessary cpu synchronization (each parallel call detected can just
>> wait until the master has finished and then return).
>>
>> BTW - The recursive spinlock today would allow for e.g. rcu_barrier() to
>> be called inside a CPU plug/unplug section. Your rwlock is removing that
>> possibility. Any chance that could be handled?
> 
> While this might be interesting for the rcu stuff, it certainly isn't
> for other pieces also relying on the cpu maps lock.
> 
> Ie: get_cpu_maps must fail when called by send_IPI_mask if there's a
> CPU plug/unplug operation going on, even if it's on the same pCPU
> that's holding the lock in write mode.

Sure? How is cpu_down() working then? It is calling stop_machine_run()
which is using send_IPI_mask()...


Juergen

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

Re: [Xen-devel] [PATCH 2/2] xen/rcu: don't use stop_machine_run() for rcu_barrier()

[Roger Pau Monné]

On Mon, Feb 17, 2020 at 02:17:23PM +0100, Jürgen Groß wrote:
> On 17.02.20 13:49, Roger Pau Monné wrote:
> > On Mon, Feb 17, 2020 at 01:32:59PM +0100, Jürgen Groß wrote:
> > > On 17.02.20 13:17, Roger Pau Monné wrote:
> > > > On Mon, Feb 17, 2020 at 01:11:59PM +0100, Jürgen Groß wrote:
> > > > > On 17.02.20 12:49, Julien Grall wrote:
> > > > > > Hi Juergen,
> > > > > > 
> > > > > > On 17/02/2020 07:20, Juergen Gross wrote:
> > > > > > > +void rcu_barrier(void)
> > > > > > >     {
> > > > > > > -    atomic_t cpu_count = ATOMIC_INIT(0);
> > > > > > > -    return stop_machine_run(rcu_barrier_action, &cpu_count, NR_CPUS);
> > > > > > > +    if ( !atomic_cmpxchg(&cpu_count, 0, num_online_cpus()) )
> > > > > > 
> > > > > > What does prevent the cpu_online_map to change under your feet?
> > > > > > Shouldn't you grab the lock via get_cpu_maps()?
> > > > > 
> > > > > Oh, indeed.
> > > > > 
> > > > > This in turn will require a modification of the logic to detect parallel
> > > > > calls on multiple cpus.
> > > > 
> > > > If you pick my patch to turn that into a rw lock you shouldn't worry
> > > > about parallel calls I think, but the lock acquisition can still fail
> > > > if there's a CPU plug/unplug going on:
> > > > 
> > > > https://lists.xenproject.org/archives/html/xen-devel/2020-02/msg00940.html
> > > 
> > > Thanks, but letting rcu_barrier() fail is a no go, so I still need to
> > > handle that case (I mean the case failing to get the lock). And handling
> > > of parallel calls is not needed to be functional correct, but to avoid
> > > not necessary cpu synchronization (each parallel call detected can just
> > > wait until the master has finished and then return).
> > > 
> > > BTW - The recursive spinlock today would allow for e.g. rcu_barrier() to
> > > be called inside a CPU plug/unplug section. Your rwlock is removing that
> > > possibility. Any chance that could be handled?
> > 
> > While this might be interesting for the rcu stuff, it certainly isn't
> > for other pieces also relying on the cpu maps lock.
> > 
> > Ie: get_cpu_maps must fail when called by send_IPI_mask if there's a
> > CPU plug/unplug operation going on, even if it's on the same pCPU
> > that's holding the lock in write mode.
> 
> Sure? How is cpu_down() working then?

send_IPI_mask failing to acquire the cpu maps lock prevents it from
using the APIC shorthand, which is what we want in that case.

> It is calling stop_machine_run()
> which is using send_IPI_mask()...

Xen should avoid using the APIC shorthand in that case, which I don't
think it's happening now, as the lock is recursive.

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] xen/rcu: don't use stop_machine_run() for rcu_barrier()

[Jürgen Groß]

On 17.02.20 14:47, Roger Pau Monné wrote:
> On Mon, Feb 17, 2020 at 02:17:23PM +0100, Jürgen Groß wrote:
>> On 17.02.20 13:49, Roger Pau Monné wrote:
>>> On Mon, Feb 17, 2020 at 01:32:59PM +0100, Jürgen Groß wrote:
>>>> On 17.02.20 13:17, Roger Pau Monné wrote:
>>>>> On Mon, Feb 17, 2020 at 01:11:59PM +0100, Jürgen Groß wrote:
>>>>>> On 17.02.20 12:49, Julien Grall wrote:
>>>>>>> Hi Juergen,
>>>>>>>
>>>>>>> On 17/02/2020 07:20, Juergen Gross wrote:
>>>>>>>> +void rcu_barrier(void)
>>>>>>>>      {
>>>>>>>> -    atomic_t cpu_count = ATOMIC_INIT(0);
>>>>>>>> -    return stop_machine_run(rcu_barrier_action, &cpu_count, NR_CPUS);
>>>>>>>> +    if ( !atomic_cmpxchg(&cpu_count, 0, num_online_cpus()) )
>>>>>>>
>>>>>>> What does prevent the cpu_online_map to change under your feet?
>>>>>>> Shouldn't you grab the lock via get_cpu_maps()?
>>>>>>
>>>>>> Oh, indeed.
>>>>>>
>>>>>> This in turn will require a modification of the logic to detect parallel
>>>>>> calls on multiple cpus.
>>>>>
>>>>> If you pick my patch to turn that into a rw lock you shouldn't worry
>>>>> about parallel calls I think, but the lock acquisition can still fail
>>>>> if there's a CPU plug/unplug going on:
>>>>>
>>>>> https://lists.xenproject.org/archives/html/xen-devel/2020-02/msg00940.html
>>>>
>>>> Thanks, but letting rcu_barrier() fail is a no go, so I still need to
>>>> handle that case (I mean the case failing to get the lock). And handling
>>>> of parallel calls is not needed to be functional correct, but to avoid
>>>> not necessary cpu synchronization (each parallel call detected can just
>>>> wait until the master has finished and then return).
>>>>
>>>> BTW - The recursive spinlock today would allow for e.g. rcu_barrier() to
>>>> be called inside a CPU plug/unplug section. Your rwlock is removing that
>>>> possibility. Any chance that could be handled?
>>>
>>> While this might be interesting for the rcu stuff, it certainly isn't
>>> for other pieces also relying on the cpu maps lock.
>>>
>>> Ie: get_cpu_maps must fail when called by send_IPI_mask if there's a
>>> CPU plug/unplug operation going on, even if it's on the same pCPU
>>> that's holding the lock in write mode.
>>
>> Sure? How is cpu_down() working then?
> 
> send_IPI_mask failing to acquire the cpu maps lock prevents it from
> using the APIC shorthand, which is what we want in that case.
> 
>> It is calling stop_machine_run()
>> which is using send_IPI_mask()...
> 
> Xen should avoid using the APIC shorthand in that case, which I don't
> think it's happening now, as the lock is recursive.

In fact the code area where this is true is much smaller than that
protected by the lock.

Basically only __cpu_disable() and __cpu_up() (on x86) are critical in
this regard.


Juergen

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

Re: [Xen-devel] [PATCH 2/2] xen/rcu: don't use stop_machine_run() for rcu_barrier()

[Igor Druzhinin]

On 17/02/2020 12:30, Igor Druzhinin wrote:
> On 17/02/2020 12:28, Jürgen Groß wrote:
>> On 17.02.20 13:26, Igor Druzhinin wrote:
>>> On 17/02/2020 07:20, Juergen Gross wrote:
>>>> Today rcu_barrier() is calling stop_machine_run() to synchronize all
>>>> physical cpus in order to ensure all pending rcu calls have finished
>>>> when returning.
>>>>
>>>> As stop_machine_run() is using tasklets this requires scheduling of
>>>> idle vcpus on all cpus imposing the need to call rcu_barrier() on idle
>>>> cpus only in case of core scheduling being active, as otherwise a
>>>> scheduling deadlock would occur.
>>>>
>>>> There is no need at all to do the syncing of the cpus in tasklets, as
>>>> rcu activity is started in __do_softirq() called whenever softirq
>>>> activity is allowed. So rcu_barrier() can easily be modified to use
>>>> softirq for synchronization of the cpus no longer requiring any
>>>> scheduling activity.
>>>>
>>>> As there already is a rcu softirq reuse that for the synchronization.
>>>>
>>>> Finally switch rcu_barrier() to return void as it now can never fail.
>>>>
>>>
>>> Would this implementation guarantee progress as previous implementation
>>> guaranteed?
>>
>> Yes.
> 
> Thanks, I'll put it to test today to see if it solves our use case.

Just manually tried it - gives infinite (up to stack size) trace like:

(XEN) [    1.496520]    [<ffff82d08022e435>] F softirq.c#__do_softirq+0x85/0x90
(XEN) [    1.496561]    [<ffff82d08022e475>] F process_pending_softirqs+0x35/0x37
(XEN) [    1.496600]    [<ffff82d080221101>] F rcupdate.c#rcu_process_callbacks+0x1df/0x1f6
(XEN) [    1.496643]    [<ffff82d08022e435>] F softirq.c#__do_softirq+0x85/0x90
(XEN) [    1.496685]    [<ffff82d08022e475>] F process_pending_softirqs+0x35/0x37
(XEN) [    1.496726]    [<ffff82d080221101>] F rcupdate.c#rcu_process_callbacks+0x1df/0x1f6
(XEN) [    1.496766]    [<ffff82d08022e435>] F softirq.c#__do_softirq+0x85/0x90
(XEN) [    1.496806]    [<ffff82d08022e475>] F process_pending_softirqs+0x35/0x37
(XEN) [    1.496847]    [<ffff82d080221101>] F rcupdate.c#rcu_process_callbacks+0x1df/0x1f6
(XEN) [    1.496887]    [<ffff82d08022e435>] F softirq.c#__do_softirq+0x85/0x90
(XEN) [    1.496927]    [<ffff82d08022e475>] F process_pending_softirqs+0x35/0x37

Igor

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

Re: [Xen-devel] [PATCH 2/2] xen/rcu: don't use stop_machine_run() for rcu_barrier()

[Jürgen Groß]

On 17.02.20 15:23, Igor Druzhinin wrote:
> On 17/02/2020 12:30, Igor Druzhinin wrote:
>> On 17/02/2020 12:28, Jürgen Groß wrote:
>>> On 17.02.20 13:26, Igor Druzhinin wrote:
>>>> On 17/02/2020 07:20, Juergen Gross wrote:
>>>>> Today rcu_barrier() is calling stop_machine_run() to synchronize all
>>>>> physical cpus in order to ensure all pending rcu calls have finished
>>>>> when returning.
>>>>>
>>>>> As stop_machine_run() is using tasklets this requires scheduling of
>>>>> idle vcpus on all cpus imposing the need to call rcu_barrier() on idle
>>>>> cpus only in case of core scheduling being active, as otherwise a
>>>>> scheduling deadlock would occur.
>>>>>
>>>>> There is no need at all to do the syncing of the cpus in tasklets, as
>>>>> rcu activity is started in __do_softirq() called whenever softirq
>>>>> activity is allowed. So rcu_barrier() can easily be modified to use
>>>>> softirq for synchronization of the cpus no longer requiring any
>>>>> scheduling activity.
>>>>>
>>>>> As there already is a rcu softirq reuse that for the synchronization.
>>>>>
>>>>> Finally switch rcu_barrier() to return void as it now can never fail.
>>>>>
>>>>
>>>> Would this implementation guarantee progress as previous implementation
>>>> guaranteed?
>>>
>>> Yes.
>>
>> Thanks, I'll put it to test today to see if it solves our use case.
> 
> Just manually tried it - gives infinite (up to stack size) trace like:
> 
> (XEN) [    1.496520]    [<ffff82d08022e435>] F softirq.c#__do_softirq+0x85/0x90
> (XEN) [    1.496561]    [<ffff82d08022e475>] F process_pending_softirqs+0x35/0x37
> (XEN) [    1.496600]    [<ffff82d080221101>] F rcupdate.c#rcu_process_callbacks+0x1df/0x1f6
> (XEN) [    1.496643]    [<ffff82d08022e435>] F softirq.c#__do_softirq+0x85/0x90
> (XEN) [    1.496685]    [<ffff82d08022e475>] F process_pending_softirqs+0x35/0x37
> (XEN) [    1.496726]    [<ffff82d080221101>] F rcupdate.c#rcu_process_callbacks+0x1df/0x1f6
> (XEN) [    1.496766]    [<ffff82d08022e435>] F softirq.c#__do_softirq+0x85/0x90
> (XEN) [    1.496806]    [<ffff82d08022e475>] F process_pending_softirqs+0x35/0x37
> (XEN) [    1.496847]    [<ffff82d080221101>] F rcupdate.c#rcu_process_callbacks+0x1df/0x1f6
> (XEN) [    1.496887]    [<ffff82d08022e435>] F softirq.c#__do_softirq+0x85/0x90
> (XEN) [    1.496927]    [<ffff82d08022e475>] F process_pending_softirqs+0x35/0x37

Interesting I didn't run into this problem. Obviously I managed to
forget handling the case of recursion.


Juergen


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