On Thu, 4 May 2017 17:32:56 +0200
Peter Zijlstra <[email protected]> wrote:
> On Mon, Apr 24, 2017 at 11:47:32AM -0400, Steven Rostedt wrote:
> > static int rto_next_cpu(struct rq *rq)
> > {
> > int cpu;
> >
> > /*
> > + * When starting the IPI RT pushing, the rto_cpu is set to nr_cpu_ids
> > + * or greater. rt_next_cpu() will simply return the first CPU found in
> > + * the rto_mask.
> > + *
> > + * If rto_next_cpu() is called with rto_cpu less than nr_cpu_ids, it
> > + * will return the next CPU found in the rto_mask.
> > + *
> > + * If there are no more CPUs left in the rto_mask, then a check is made
> > + * against rto_loop and rto_loop_next. rto_loop is only updated with
> > + * the rto_lock held, but any CPU may increment the rto_loop_next
> > + * without any locking.
> > */
> > +again:
> > + if (rq->rd->rto_cpu >= nr_cpu_ids) {
> > cpu = cpumask_first(rq->rd->rto_mask);
> > + rq->rd->rto_cpu = cpu;
> > + /* If cpu is nr_cpu_ids, then there is no overloaded rqs */
> > + return cpu;
> > }
> >
> > + cpu = cpumask_next(rq->rd->rto_cpu, rq->rd->rto_mask);
> > + rq->rd->rto_cpu = cpu;
> >
> > + if (cpu < nr_cpu_ids)
> > + return cpu;
> >
> > + if (rq->rd->rto_loop == atomic_read(&rq->rd->rto_loop_next))
> > + return cpu;
> >
> > + rq->rd->rto_loop = atomic_read(&rq->rd->rto_loop_next);
> > + goto again;
> > +}
>
> I think you want to write that as:
>
> struct root_domain *rd = rq->rd;
> int cpu, next;
>
> /* comment */
> for (;;) {
> if (rd->rto_cpu >= nr_cpu_ids) {
If we go with your change, then this needs to be:
if (rd->rto_cpu < 0) {
> cpu = cpumask_first(rd->rto_mask);
> rd->rto_cpu = cpu;
> return cpu;
> }
>
> cpu = cpumask_next(rd->rto_mask);
cpumask_next() requires two parameters.
> rd->rto_cpu = cpu;
>
> if (cpu < nr_cpu_ids)
> break;
>
> // rd->rto_cpu = -1;
>
> /*
> * ACQUIRE ensures we see the @rto_mask changes
> * made prior to the @next value observed.
> *
> * Matches WMB in rt_set_overload().
> */
> next = atomic_read_acquire(&rd->rto_loop_next);
>
> if (rd->rto_loop == next)
> break;
>
> rd->rto_loop = next;
> }
>
> return cpu;
>
> And I don't fully understand the whole rto_cpu >= nr_cpus_ids thing,
> can't you simply reset the thing to -1 and always use cpumask_next()?
> As per the // comment above?
>
> > +static inline bool rto_start_trylock(atomic_t *v)
> > +{
> > + return !atomic_cmpxchg(v, 0, 1);
>
> Arguably this could be: !atomic_cmpxchg_acquire(v, 0, 1);
Yes agreed. But if you remember, at the time I was basing this off of
tip/sched/core, which didn't have atomic_cmpxchg_acquire() available.
>
> > }
> >
> > +static inline void rto_start_unlock(atomic_t *v)
> > +{
> > + atomic_set_release(v, 0);
> > +}
> >
>
> > static void tell_cpu_to_push(struct rq *rq)
> > {
> > + int cpu = nr_cpu_ids;
> >
> > + /* Keep the loop going if the IPI is currently active */
> > + atomic_inc_return(&rq->rd->rto_loop_next);
>
> Since rt_set_overload() already provides a WMB, we don't need an
> ordered primitive here and atomic_inc() is fine.
Agree, I mentioned this in my previous reply. It was leftover from
previous versions of the patch. I believe I also needed a memory
barrier with this and the check for rto_loop_start. Can't remember if
that was the case, but it doesn't matter now as loop_start is now
updated with a cmpxchg.
>
> >
> > + /* Only one CPU can initiate a loop at a time */
> > + if (!rto_start_trylock(&rq->rd->rto_loop_start))
> > return;
> >
> > + raw_spin_lock(&rq->rd->rto_lock);
> > +
> > + /*
> > + * The rto_cpu is updated under the lock, if it has a valid cpu
> > + * then the IPI is still running and will continue due to the
> > + * update to loop_next, and nothing needs to be done here.
> > + * Otherwise it is finishing up and an ipi needs to be sent.
> > + */
> > + if (rq->rd->rto_cpu >= nr_cpu_ids)
> // if (rq->rd->rto_cpu < 0)
This can be done, I was just being a bit more conservative and having
rto_cpu have less states (valid CPU or nr_cpu_ids). With a -1, we have
to manually set it to that. But I'm fine with doing it that way too.
This went through several iterations. There were times where using a -1
wasn't so simple.
>
> > + cpu = rto_next_cpu(rq);
> >
> > + raw_spin_unlock(&rq->rd->rto_lock);
> > +
> > + rto_start_unlock(&rq->rd->rto_loop_start);
> > +
> > + if (cpu < nr_cpu_ids)
> > + irq_work_queue_on(&rq->rd->rto_push_work, cpu);
> > }
> >
> > /* Called from hardirq context */
> > +void rto_push_irq_work_func(struct irq_work *work)
> > {
> > + struct rq *rq;
> > int this_cpu;
> > int cpu;
> >
> > + this_cpu = smp_processor_id();
> > rq = cpu_rq(this_cpu);
>
> rq = this_rq();
Heh, sure. I guess I was just keeping it with the previous logic.
Thanks for the review. I'll spin up a new patch. Unfortunately, I no
longer have access to the behemoth machine. I'll only be testing this
on 4 cores now, or 8 with HT.
-- Steve
>
> >
> > + /*
> > + * We do not need to grab the lock to check for has_pushable_tasks.
> > + * When it gets updated, a check is made if a push is possible.
> > + */
> > if (has_pushable_tasks(rq)) {
> > raw_spin_lock(&rq->lock);
> > + push_rt_tasks(rq);
> > raw_spin_unlock(&rq->lock);
> > }
> >
> > + raw_spin_lock(&rq->rd->rto_lock);
> >
> > + /* Pass the IPI to the next rt overloaded queue */
> > + cpu = rto_next_cpu(rq);
> >
> > + raw_spin_unlock(&rq->rd->rto_lock);
> >
> > if (cpu >= nr_cpu_ids)
> > return;
> >
> > /* Try the next RT overloaded CPU */
> > + irq_work_queue_on(&rq->rd->rto_push_work, cpu);
> > }
