Re: [Xen-devel] [PATCH v2 31/48] xen/sched: add support for multiple vcpus per sched unit where missing

[Juergen Gross <jgross@xxxxxxxx>]

On 11.09.19 12:43, Jan Beulich wrote:
> On 09.08.2019 16:58, Juergen Gross wrote:
> > V1:
> > - add special handling for idle unit in unit_runnable() and
> >    unit_runnable_state()
> Why was this done? Isn't vcpu_runnable() going to always return
> true for idle vCPU-s?
The problem is the for_each_sched_unit_vcpu() loop handling.

The loop is ending as soon as vcpu->sched_unit != unit. But this
might be true for idle vcpus in case they are temporarily active
for a normal domain's unit.

> > --- a/xen/common/domain.c
> > +++ b/xen/common/domain.c
> > @@ -1255,7 +1255,10 @@ int vcpu_reset(struct vcpu *v)
> >       v->async_exception_mask = 0;
> >       memset(v->async_exception_state, 0, sizeof(v->async_exception_state));
> >   #endif
> > -    v->affinity_broken = 0;
> > +    if ( v->affinity_broken & VCPU_AFFINITY_OVERRIDE )
> > +        vcpu_temporary_affinity(v, NR_CPUS, VCPU_AFFINITY_OVERRIDE);
> > +    if ( v->affinity_broken & VCPU_AFFINITY_WAIT )
> > +        vcpu_temporary_affinity(v, NR_CPUS, VCPU_AFFINITY_WAIT);
> Shouldn't this live in an earlier patch? I guess the same question
> is applicable to almost everything here, as also already mentioned
> e.g. in the previous patch.
Hmm, this _is_ a missing part for multiple vcpus per unit.

I wouldn't know which other patch would be more suitable.

> >   static inline void sched_set_res(struct sched_unit *unit,
> >                                    struct sched_resource *res)
> >   {
> > -    unit->vcpu_list->processor = res->processor;
> > +    int cpu = cpumask_first(res->cpus);
> unsigned int
Yes.

> >   static inline void sched_set_pause_flags_atomic(struct sched_unit *unit,
> >                                                   unsigned int bit)
> >   {
> > -    set_bit(bit, &unit->vcpu_list->pause_flags);
> > +    struct vcpu *v;
> > +
> > +    for_each_sched_unit_vcpu ( unit, v )
> > +        set_bit(bit, &v->pause_flags);
> >   }
> >   
> >   static inline void sched_clear_pause_flags_atomic(struct sched_unit *unit,
> >                                                     unsigned int bit)
> >   {
> > -    clear_bit(bit, &unit->vcpu_list->pause_flags);
> > +    struct vcpu *v;
> > +
> > +    for_each_sched_unit_vcpu ( unit, v )
> > +        clear_bit(bit, &v->pause_flags);
> >   }
> The atomicity is (necessarily) limited here - it's atomic for an
> individual vCPU, but not atomic for a unit as a whole. If this
> is indeed a useful hybrid, then I think it would be nice if there
> was a comment next to these functions clarifying under what
> conditions use of them is correct without it also being correct
> to use their non-atomic counterparts (e.g. due to use of an
> external lock).
Okay, I'll add that.


Juergen

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