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

[Juergen Gross <jgross@xxxxxxxx>]

On 01.07.19 16:08, Jan Beulich wrote:
> > > > On 28.05.19 at 12:32, <jgross@xxxxxxxx> wrote:
> > @@ -155,8 +156,8 @@ static void nmi_mce_softirq(void)
> >        * Set the tmp value unconditionally, so that the check in the iret
> >        * hypercall works.
> >        */
> > -    cpumask_copy(st->vcpu->cpu_hard_affinity_tmp,
> > -                 st->vcpu->cpu_hard_affinity);
> > +    cpumask_copy(st->vcpu->sched_unit->cpu_hard_affinity_tmp,
> > +                 st->vcpu->sched_unit->cpu_hard_affinity);
> Aiui this affects all vCPU-s in the unit, which is unlikely to be what we
> want here: There's now only one cpu_hard_affinity_tmp for all vCPU-s
> in the unit, yet every vCPU in there may want to make use of the
> field in parallel.
Hmm, yes, we'll need a usage bitmask.

Please note that affecting all vcpus in the unit is per design. With
multiple vcpus of a unit needing this feature in parallel there is no
way they can have different needs regarding temporary affinity.

> I also wonder how the code further down in this function fits with
> the scheduler unit concept. But perhaps that's going to be dealt with
> by later patches...
>
> > --- a/xen/common/domain.c
> > +++ b/xen/common/domain.c
> > @@ -125,11 +125,6 @@ static void vcpu_info_reset(struct vcpu *v)
> >   
> >   static void vcpu_destroy(struct vcpu *v)
> >   {
> > -    free_cpumask_var(v->cpu_hard_affinity);
> > -    free_cpumask_var(v->cpu_hard_affinity_tmp);
> > -    free_cpumask_var(v->cpu_hard_affinity_saved);
> > -    free_cpumask_var(v->cpu_soft_affinity);
> > -
> >       free_vcpu_struct(v);
> >   }
> >   
> > @@ -153,12 +148,6 @@ struct vcpu *vcpu_create(
> >   
> >       grant_table_init_vcpu(v);
> >   
> > -    if ( !zalloc_cpumask_var(&v->cpu_hard_affinity) ||
> > -         !zalloc_cpumask_var(&v->cpu_hard_affinity_tmp) ||
> > -         !zalloc_cpumask_var(&v->cpu_hard_affinity_saved) ||
> > -         !zalloc_cpumask_var(&v->cpu_soft_affinity) )
> > -        goto fail;
> Seeing these, I'm actually having trouble understanding how you mean
> to retain the user visible interface behavior here: If you only store an
> affinity per sched unit, then how are you meaning to honor the vCPU
> granular requests coming in?
With core scheduling it is only possible to set (virtual) core
affinities. Whenever an affinity of a vcpu is being set it will set the
affinity of the whole unit.

> > @@ -557,9 +545,10 @@ void domain_update_node_affinity(struct domain *d)
> >            */
> >           for_each_vcpu ( d, v )
> >           {
> > -            cpumask_or(dom_cpumask, dom_cpumask, v->cpu_hard_affinity);
> > +            cpumask_or(dom_cpumask, dom_cpumask,
> > +                       v->sched_unit->cpu_hard_affinity);
> >               cpumask_or(dom_cpumask_soft, dom_cpumask_soft,
> > -                       v->cpu_soft_affinity);
> > +                       v->sched_unit->cpu_soft_affinity);
> >           }
> There's not going to be a for_each_sched_unit(), is there? It
> would mean less iterations here, and less redundant CPU mask
> operations. Ah, that's the subject of patch 30.
Right.

> > @@ -1226,7 +1215,7 @@ int vcpu_reset(struct vcpu *v)
> >       v->async_exception_mask = 0;
> >       memset(v->async_exception_state, 0, sizeof(v->async_exception_state));
> >   #endif
> > -    cpumask_clear(v->cpu_hard_affinity_tmp);
> > +    cpumask_clear(v->sched_unit->cpu_hard_affinity_tmp);
> Same issue as above - you're affecting other vCPU-s here.
Yes, we'll need a usage bitmask to be tested here.

> > --- a/xen/common/domctl.c
> > +++ b/xen/common/domctl.c
> > @@ -614,6 +614,7 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) 
> > u_domctl)
> >       case XEN_DOMCTL_getvcpuaffinity:
> >       {
> >           struct vcpu *v;
> > +        struct sched_unit *unit;
> const?
>
> > --- a/xen/common/keyhandler.c
> > +++ b/xen/common/keyhandler.c
> > @@ -312,8 +312,8 @@ static void dump_domains(unsigned char key)
> >                   printk("dirty_cpu=%u", v->dirty_cpu);
> >               printk("\n");
> >               printk("    cpu_hard_affinity={%*pbl} 
> > cpu_soft_affinity={%*pbl}\n",
> > -                   nr_cpu_ids, cpumask_bits(v->cpu_hard_affinity),
> > -                   nr_cpu_ids, cpumask_bits(v->cpu_soft_affinity));
> > +                   nr_cpu_ids, cpumask_bits(v->sched_unit->cpu_hard_affinity),
> > +                   nr_cpu_ids, cpumask_bits(v->sched_unit->cpu_soft_affinity));
> I don't see the value of logging the same information multiple times
> (for each vCPU in a sched unit). I think you want to split this up.
Yes, true.

> > --- a/xen/common/wait.c
> > +++ b/xen/common/wait.c
> > @@ -132,7 +132,7 @@ static void __prepare_to_wait(struct waitqueue_vcpu *wqv)
> >   
> >       /* Save current VCPU affinity; force wakeup on *this* CPU only. */
> >       wqv->wakeup_cpu = smp_processor_id();
> > -    cpumask_copy(&wqv->saved_affinity, curr->cpu_hard_affinity);
> > +    cpumask_copy(&wqv->saved_affinity, curr->sched_unit->cpu_hard_affinity);
> >       if ( vcpu_set_hard_affinity(curr, cpumask_of(wqv->wakeup_cpu)) )
> >       {
> >           gdprintk(XENLOG_ERR, "Unable to set vcpu affinity\n");
> > @@ -199,7 +199,7 @@ void check_wakeup_from_wait(void)
> >       {
> >           /* Re-set VCPU affinity and re-enter the scheduler. */
> >           struct vcpu *curr = current;
> > -        cpumask_copy(&wqv->saved_affinity, curr->cpu_hard_affinity);
> > +        cpumask_copy(&wqv->saved_affinity, 
> > curr->sched_unit->cpu_hard_affinity);
> >           if ( vcpu_set_hard_affinity(curr, cpumask_of(wqv->wakeup_cpu)) )
> >           {
> >               gdprintk(XENLOG_ERR, "Unable to set vcpu affinity\n");
> Same problem as above - the consumer of ->saved_affinity will affect
> vCPU-s other than the subject one.
Yes.

> > --- 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.

> In any event, for the purpose here, I think there should be a backlink to
> struct domain in struct sched_unit right away, and it should get used here.
See patch 15.

> > @@ -283,6 +265,22 @@ struct sched_unit {
> >       void                  *priv;      /* scheduler private data */
> >       struct sched_unit     *next_in_list;
> >       struct sched_resource *res;
> > +
> > +    /* Last time when unit has been scheduled out. */
> > +    uint64_t               last_run_time;
> > +
> > +    /* Item needs affinity restored. */
> > +    bool                   affinity_broken;
> > +    /* Does soft affinity actually play a role (given hard affinity)? */
> > +    bool                   soft_aff_effective;
> > +    /* Bitmask of CPUs on which this VCPU may run. */
> > +    cpumask_var_t          cpu_hard_affinity;
> > +    /* Used to change affinity temporarily. */
> > +    cpumask_var_t          cpu_hard_affinity_tmp;
> > +    /* Used to restore affinity across S3. */
> > +    cpumask_var_t          cpu_hard_affinity_saved;
> > +    /* Bitmask of CPUs on which this VCPU prefers to run. */
> > +    cpumask_var_t          cpu_soft_affinity;
> >   };
> The mentions of "VCPU" in the comments here also survive till the end
> of the series, which I also don't think is quite right.
Will modify.

> > @@ -980,7 +978,7 @@ static inline bool is_hvm_vcpu(const struct vcpu *v)
> >   static inline bool is_hwdom_pinned_vcpu(const struct vcpu *v)
> >   {
> >       return (is_hardware_domain(v->domain) &&
> > -            cpumask_weight(v->cpu_hard_affinity) == 1);
> > +            cpumask_weight(v->sched_unit->cpu_hard_affinity) == 1);
> >   }
> Seeing this - how is pinning (by command line option or by Dom0
> doing this on itself transiently) going to work with core scheduling?
In the end only the bit of the first vcpu of a unit will be set in the
affinity masks, affecting all vcpus of the unit.


Juergen

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