Re: [Xen-devel] [PATCH 7/7] xen: sched_rt: print useful affinity info when dumping

[Meng Xu <xumengpanda@xxxxxxxxx>]

Hi Dario,

2015-03-16 13:05 GMT-04:00 Dario Faggioli <dario.faggioli@xxxxxxxxxx>:
> In fact, printing the cpupool's CPU online mask
> for each vCPU is just redundant, as that is the
> same for all the vCPUs of all the domains in the
> same cpupool, while hard affinity is already part
> of the output of dumping domains info.
>
> Instead, print the intersection between hard
> affinity and online CPUs, which is --in case of this
> scheduler-- the effective affinity always used for
> the vCPUs.

Agree.

>
> This change also takes the chance to add a scratch
> cpumask, to avoid having to create one more
> cpumask_var_t on the stack of the dumping routine.

Actually, I have a question about the strength of this design. When we
have a machine with many cpus, we will end up with allocating a
cpumask for each cpu. Is this better than having a cpumask_var_t on
the stack of the dumping routine, since the dumping routine is not in
the hot path?

> Such scratch area can be used to kill most of the
> cpumasks_var_t local variables in other functions
> in the file, but that is *NOT* done in this chage.
>
> Finally, convert the file to use keyhandler scratch,
> instead of open coded string buffers.
>
> Signed-off-by: Dario Faggioli <dario.faggioli@xxxxxxxxxx>
> Cc: George Dunlap <george.dunlap@xxxxxxxxxxxxx>
> Cc: Meng Xu <xumengpanda@xxxxxxxxx>
> Cc: Jan Beulich <JBeulich@xxxxxxxx>
> Cc: Keir Fraser <keir@xxxxxxx>
> ---
>  xen/common/sched_rt.c |   40 ++++++++++++++++++++++++++++++++--------
>  1 file changed, 32 insertions(+), 8 deletions(-)
>
> diff --git a/xen/common/sched_rt.c b/xen/common/sched_rt.c
> index 167a484..7aa3ac8 100644
> --- a/xen/common/sched_rt.c
> +++ b/xen/common/sched_rt.c
> @@ -124,6 +124,12 @@
>  #define TRC_RTDS_BUDGET_REPLENISH TRC_SCHED_CLASS_EVT(RTDS, 4)
>  #define TRC_RTDS_SCHED_TASKLET    TRC_SCHED_CLASS_EVT(RTDS, 5)
>
> + /*
> +  * Useful to avoid too many cpumask_var_t on the stack.
> +  */
> +static cpumask_t **_cpumask_scratch;
> +#define cpumask_scratch _cpumask_scratch[smp_processor_id()]
> +
>  /*
>   * Systme-wide private data, include global RunQueue/DepletedQ
>   * Global lock is referenced by schedule_data.schedule_lock from all
> @@ -218,7 +224,6 @@ __q_elem(struct list_head *elem)
>  static void
>  rt_dump_vcpu(const struct scheduler *ops, const struct rt_vcpu *svc)
>  {
> -    char cpustr[1024];
>      cpumask_t *cpupool_mask;
>
>      ASSERT(svc != NULL);
> @@ -229,10 +234,22 @@ rt_dump_vcpu(const struct scheduler *ops, const struct 
> rt_vcpu *svc)
>          return;
>      }
>
> -    cpumask_scnprintf(cpustr, sizeof(cpustr), svc->vcpu->cpu_hard_affinity);
> +    cpupool_mask = cpupool_scheduler_cpumask(svc->vcpu->domain->cpupool);
> +    /*
> +     * We can't just use 'cpumask_scratch' because the dumping can
> +     * happen from a pCPU outside of this scheduler's cpupool, and
> +     * hence it's not right to use the pCPU's scratch mask (which
> +     * may even not exist!). On the other hand, it is safe to use
> +     * svc->vcpu->processor's own scratch space, since we own the
> +     * runqueue lock.
> +     */
> +    cpumask_and(_cpumask_scratch[svc->vcpu->processor], cpupool_mask,
> +                svc->vcpu->cpu_hard_affinity);
> +    cpulist_scnprintf(keyhandler_scratch, sizeof(keyhandler_scratch),
> +                      _cpumask_scratch[svc->vcpu->processor]);
>      printk("[%5d.%-2u] cpu %u, (%"PRI_stime", %"PRI_stime"),"
>             " cur_b=%"PRI_stime" cur_d=%"PRI_stime" last_start=%"PRI_stime"\n"
> -           " \t\t onQ=%d runnable=%d cpu_hard_affinity=%s ",
> +           " \t\t onQ=%d runnable=%d flags=%x effective hard_affinity=%s\n",
>              svc->vcpu->domain->domain_id,
>              svc->vcpu->vcpu_id,
>              svc->vcpu->processor,
> @@ -243,11 +260,8 @@ rt_dump_vcpu(const struct scheduler *ops, const struct 
> rt_vcpu *svc)
>              svc->last_start,
>              __vcpu_on_q(svc),
>              vcpu_runnable(svc->vcpu),
> -            cpustr);
> -    memset(cpustr, 0, sizeof(cpustr));
> -    cpupool_mask = cpupool_scheduler_cpumask(svc->vcpu->domain->cpupool);
> -    cpumask_scnprintf(cpustr, sizeof(cpustr), cpupool_mask);
> -    printk("cpupool=%s\n", cpustr);
> +            svc->flags,
> +            keyhandler_scratch);
>  }
>
>  static void
> @@ -409,6 +423,10 @@ rt_init(struct scheduler *ops)
>      if ( prv == NULL )
>          return -ENOMEM;
>
> +    _cpumask_scratch = xmalloc_array(cpumask_var_t, nr_cpu_ids);

Is it better to use xzalloc_array?

> +    if ( _cpumask_scratch == NULL )
> +        return -ENOMEM;
> +
>      spin_lock_init(&prv->lock);
>      INIT_LIST_HEAD(&prv->sdom);
>      INIT_LIST_HEAD(&prv->runq);
> @@ -426,6 +444,7 @@ rt_deinit(const struct scheduler *ops)
>  {
>      struct rt_private *prv = rt_priv(ops);
>
> +    xfree(_cpumask_scratch);
>      xfree(prv);
>  }
>
> @@ -443,6 +462,9 @@ rt_alloc_pdata(const struct scheduler *ops, int cpu)
>      per_cpu(schedule_data, cpu).schedule_lock = &prv->lock;
>      spin_unlock_irqrestore(&prv->lock, flags);
>
> +    if ( !alloc_cpumask_var(&_cpumask_scratch[cpu]) )

Is it better to use zalloc_cpumask_var() here?


> +        return NULL;
> +
>      /* 1 indicates alloc. succeed in schedule.c */
>      return (void *)1;
>  }
> @@ -462,6 +484,8 @@ rt_free_pdata(const struct scheduler *ops, void *pcpu, 
> int cpu)
>      sd->schedule_lock = &sd->_lock;
>
>      spin_unlock_irqrestore(&prv->lock, flags);
> +
> +    free_cpumask_var(_cpumask_scratch[cpu]);
>  }
>
>  static void *
>

Others look good to me!

Thank you very much! :-)

Best,

Meng


-----------
Meng Xu
PhD Student in Computer and Information Science
University of Pennsylvania

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel