Re: [Xen-devel] [PATCH 13/60] xen/sched: move some per-vcpu items to struct sched_unit

[Juergen Gross <jgross@xxxxxxxx>]

On 02.07.19 10:27, Jan Beulich wrote:
> On 02.07.2019 10:14, Juergen Gross wrote:
> > On 02.07.19 09:54, Jan Beulich wrote:
> > > And again - if someone pins every vCPU to a single pCPU, that last
> > > such pinning operation will be what takes long term effect. Aiui all
> > > vCPU-s in the unit will then be pinned to that one pCPU, i.e.
> > > they'll either all compete for the one pCPU's time, or only one of
> > > them will ever get scheduled.
> > No, that's not how it works. Lets say we have a system with the
> > following topology and core scheduling active:
> >
> > cpu0: core 0, thread 0
> > cpu1: core 0, thread 1
> > cpu2: core 1, thread 0
> > cpu3: core 1, thread 1
> >
> > Then any even numbered vcpu will only ever be scheduled on cpu0 or cpu2,
> > while any odd numbered vcpu will only run on cpu1 or cpu3.
> >
> > So virtual cores get scheduled on physical cores. Virtual thread 0 will
> > only run on physical thread 0 and the associated virtual thread 1 will
> > run on the associated physical thread 1 of the same physical core.
> >
> > Pinning a virtual thread 1 to a physical thread 0 is not possible (in
> > reality only the virtual core is being pinned to the physical core).
> But that's what existing guests may be doing. You may want to
> take a look at our old, non-pvops kernels, in particular the
> functionality provided by their drivers/xen/core/domctl.c. Yes,
> {sys,dom}ctl-s aren't supposed to be used by the kernel, but to
> achieve the intended effects I saw no way around (ab)using them.
> (I mean this to be taken as an example only - I realize that the
> code there wouldn't work on modern Xen without updating, due to
> the sysctl/domctl interface version that needs setting.)
First - speaking of "guests" is a little bit misleading here. This is
restricted to dom0.

So when you want to use such a dom0 kernel with Xen 4.13 or later you'd
need to stay with cpu scheduling. Any recent kernel will run just fine
as dom0 with core scheduling active.

> > > > > > > > --- a/xen/include/xen/sched-if.h
> > > > > > > > +++ b/xen/include/xen/sched-if.h
> > > > > > > > @@ -438,11 +438,11 @@ static inline cpumask_t* cpupool_domain_cpumask(struct 
> > > > > > > > domain *d)
> > > > > > > >       * * The hard affinity is not a subset of soft affinity
> > > > > > > >       * * There is an overlap between the soft and hard affinity masks
> > > > > > > >       */
> > > > > > > > -static inline int has_soft_affinity(const struct vcpu *v)
> > > > > > > > +static inline int has_soft_affinity(const struct sched_unit *unit)
> > > > > > > >      {
> > > > > > > > -    return v->soft_aff_effective &&
> > > > > > > > -           !cpumask_subset(cpupool_domain_cpumask(v->domain),
> > > > > > > > -                           v->cpu_soft_affinity);
> > > > > > > > +    return unit->soft_aff_effective &&
> > > > > > > > +           !cpumask_subset(cpupool_domain_cpumask(unit->vcpu->domain),
> > > > > > > > +                           unit->cpu_soft_affinity);
> > > > > > > >      }
> > > > > > > Okay, at the moment there looks to be a 1:1 relationship between sched
> > > > > > > units and vCPU-s. This would - at this point of the series - invalidate most
> > > > > > > my earlier comments. However, in patch 57 I don't see how this unit->vcpu
> > > > > > > mapping would get broken, and I can't seem to identify any other patch
> > > > > > > where this might be happening. Looking at the github branch I also get the
> > > > > > > impression that the struct vcpu * pointer out of struct sched_unit survives
> > > > > > > until the end of the series, which doesn't seem right to me.
> > > > > > It is right. The vcpu pointer in the sched_unit is pointing to the first
> > > > > > vcpu of the unit at the end of the series. Further vcpus are found via
> > > > > > v->next_in_list.
> > > > > I'm afraid this sets us up for misunderstanding and misuse. I don't
> > > > > think there should be a straight struct vcpu * out of struct sched_unit.
> > > > That was the most effective way to do it. What are you suggesting?
> > > An actual list, i.e. with a struct list_head. That'll make obvious that
> > > more than one vCPU might be associated with a unit. That's even more so
> > > that the ability to associate more than one appears only quite late in
> > > the series, i.e. there may be further instances like the code above, and
> > > it would require a careful audit (rather than the compiler finding such
> > > instance) to determine all places where using the first vCPU in a unit
> > > isn't really what was meant.
> > TBH I don't see how this would help at all.
> >
> > Instead of using the vcpu pointer I'd had to use the list head instead.
> > Why is that different to a plain pointer regarding finding the places
> > where using the first vcpu was wrong?
> Take the example above: Is it correct to act on just the first vCPU?
> I guess _here_ it is, but the same pattern could be found elsewhere.
> If, from the beginning, you use a clearly identifiable list construct,
> then it'll be obvious to you as the writer and to reviewers that by
> the end of the series there may be multiple entities that need dealing
> with - we'd see list_first*() or for_each*() constructs right away
> (and you wouldn't be able to circumvent their use in a way that
> wouldn't trigger "don't open-code" comments).
Would you be fine with just renaming the pointer to "vcpu_list"?
This would avoid the need to introduce another vcpu list in struct vcpu
and I could re-use the already existing list as today.

It should be noted that I named the pointer just "vcpu" in order to
avoid lots of reformatting due to longer lines, though.


Juergen

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