Re: [Xen-devel] [PATCH v3 30/47] xen/sched: add support for multiple vcpus per sched unit where missing

[Jürgen Groß <jgross@xxxxxxxx>]

On 24.09.19 17:00, Jan Beulich wrote:
> On 24.09.2019 16:41, Jürgen Groß wrote:
> > On 23.09.19 17:41, Jan Beulich wrote:
> > > On 14.09.2019 10:52, Juergen Gross wrote:
> > > > @@ -1851,7 +1852,7 @@ void sched_context_switched(struct vcpu *vprev, struct 
> > > > vcpu *vnext)
> > > >                while ( atomic_read(&next->rendezvous_out_cnt) )
> > > >                    cpu_relax();
> > > >        }
> > > > -    else if ( vprev != vnext )
> > > > +    else if ( vprev != vnext && sched_granularity == 1 )
> > > >            context_saved(vprev);
> > > >    }
> > > Would you mind helping me with understanding why this call is
> > > needed with a granularity of 1 only?
> > Otherwise it is done just a few lines up (granularity 1 will result
> > in rendezvous_out_cnt being zero).
> I.e. if is rendezvous_out_cnt is zero and vprev != vnext but
> sched_granularity > 1 the call isn't needed? Why? At the end of
I can add an ASSERT() here. This should never happen.

> the series vcpu_context_saved() gets called in all cases; what's
> conditional upon granularity being 1 there is the call to
> unit_context_saved().
vcpu_context_saved() at the end of the series is testing vprev != vnext.

> > > > +    if ( is_idle_unit(unit) )
> > > > +        return true;
> > > > +
> > > > +    for_each_sched_unit_vcpu ( unit, v )
> > > > +        if ( vcpu_runnable(v) )
> > > > +            return true;
> > > Isn't the loop going to yield true anyway for idle units? If so, is
> > > there a particular reason for the special casing of idle units up
> > > front here?
> > Didn't I explain that before?
> Quite possible; a good hint that a code comment wouldn't hurt.
Okay.

> > for_each_sched_unit_vcpu() for an idle
> > unit might end premature when one of the vcpus is running in another
> > unit (idle_vcpu->sched_unit != idle_unit).
> Oh, that (v)->sched_unit == (i) in the construct is clearly unexpected.
> Is this really still needed by the end of the series? I realize that
> _some_ check is needed, but could this perhaps be arranged in a way
> that you'd still hit all vCPU-s when using it on an idle unit, no
> matter whether they're in use as a substitute in a "real" unit?
I could do that by having another linked list in struct vcpu. This way
I can avoid it.

> As to that construct - why is the parameter named "i" rather than "u"?
> And why "e" in for_each_sched_unit()?
"i" like "item" (somehow this survived the big rename). Can change it.
"e" like "element". I think this is another relic. Can change it, too.


Juergen

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