On 17.03.20 14:56, Jan Beulich wrote:
On 13.03.2020 14:06, Juergen Gross wrote:
@@ -143,51 +143,85 @@ 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 cpus required to finish barrier handling.
+ * pending_count is initialized to nr_cpus + 1.
+ * Cpus are synchronized via softirq mechanism. rcu_barrier() is regarded to
+ * be active if pending_count is not zero. In case rcu_barrier() is called on
+ * multiple cpus it is enough to check for pending_count being not zero on
entry
+ * and to call process_pending_softirqs() in a loop until pending_count drops
to
+ * zero, before starting the new rcu_barrier() processing.
Everything up to here reads fine, but ...
+ * In order to avoid hangs when rcu_barrier() is called multiple times on the
+ * same cpu in fast sequence and a slave cpu couldn't drop out of the
+ * barrier handling fast enough a second counter pending_count is needed.
+ * The rcu_barrier() invoking cpu will wait until pending_count reaches 1
+ * (meaning that all cpus have finished processing the barrier) and then will
+ * reset pending_count to 0 to enable entering rcu_barrier() again.
... this starts as if pending_count wasn't mentioned before at all,
which might end up being confusing (e.g. suspecting the text having
gone out of sync with the code, as has happened to me).
I'll reword the comment.
+ */
+static atomic_t cpu_count = ATOMIC_INIT(0);
+static atomic_t pending_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);
+ smp_wmb(); /* Make all previous writes visible to other cpus. */
+ atomic_dec(&cpu_count);
In Linux terms, wouldn't this be smp_mb__before_atomic()? If so,
perhaps better if we also introduce this and its "after" sibling.
Okay, will add a patch.
}
-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.
+ * is completed. When all CPUs have executed their callback, cpu_count
+ * will have been decremented to 0.
*/
- 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;
+ atomic_dec(&pending_count);
Isn't there a barrier needed between the atomic_read() and this
atomic_dec()?
Yes, probably.
+void rcu_barrier(void)
{
- atomic_t cpu_count = ATOMIC_INIT(0);
- return stop_machine_run(rcu_barrier_action, &cpu_count, NR_CPUS);
+ unsigned int n_cpus;
+
+ ASSERT(!in_irq() && local_irq_is_enabled());
+
+ for ( ;; )
Nit: Canonically there ought to also be a blank between the two
semicolons.
Okay.
+ {
+ if ( !atomic_read(&pending_count) && get_cpu_maps() )
+ {
+ n_cpus = num_online_cpus();
+
+ if ( atomic_cmpxchg(&pending_count, 0, n_cpus + 1) == 0 )
+ break;
+
+ put_cpu_maps();
+ }
+
+ process_pending_softirqs();
+ cpu_relax();
Is this really needed after having invoked
process_pending_softirqs()?
With no softirq pending this loop might be rather tight. Better to give
a potential other sibling a chance to make progress.
+ }
+
+ atomic_set(&cpu_count, n_cpus);
Isn't there a barrier needed ahead of this, to order it wrt the
cmpxchg?
I'll add one.
+ cpumask_raise_softirq(&cpu_online_map, RCU_SOFTIRQ);
Isn't there another barrier needed ahead of this, to order it wrt
the set?
No, I don't think so. cpumask_raise_softirq() needs to have appropriate
ordering semantics as otherwise the softirq pending bit wouldn't be
guaranteed to be seen by softirq processing.
Juergen
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
