Re: [Xen-devel] [PATCH v14 04/10] x86: detect and initialize Platform QoS Monitoring feature

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 28.08.14 at 09:43, <chao.p.peng@xxxxxxxxxxxxxxx> wrote:
> +static void __init parse_pqos_param(char *s)
> +{
> +    char *ss, *val_str;
> +    int val;
> +
> +    do {
> +        ss = strchr(s, ',');
> +        if ( ss )
> +            *ss = '\0';
> +
> +        val = parse_bool(s);
> +        if ( val >= 0 )
> +            opt_pqos = val;
> +        else
> +        {
> +            val_str = strchr(s, ':');
> +            if ( val_str )
> +                *val_str++ = '\0';
> +
> +            if ( val_str && !strcmp(s, "pqos_monitor") &&
> +                 (val = parse_bool(val_str)) >= 0 )
> +                opt_pqos_monitor = val;
> +            else if ( val_str && !strcmp(s, "rmid_max") )
> +                opt_rmid_max = simple_strtoul(val_str, NULL, 0);

Shouldn't both of these imply opt_pqos = 1, so the user can avoid
redundancy like "pqos=yes,pqos_monitor:yes"? I'd even think
"pqos=pqos_monitor" should be sufficient to enable PQoS and the
monitoring.

> +static void __init init_pqos_monitor(unsigned int rmid_max)
> +{
> +    unsigned int eax, ebx, ecx, edx;
> +    unsigned int rmid;
> +
> +    if ( !boot_cpu_has(X86_FEATURE_QOSM) )
> +        return;
> +
> +    cpuid_count(0xf, 0, &eax, &ebx, &ecx, &edx);
> +    if ( !edx )
> +        return;
> +
> +    pqosm = xzalloc(struct pqos_monitor);
> +    if ( !pqosm )
> +        return;
> +
> +    pqosm->qm_features = edx;
> +    pqosm->rmid_mask = ~(~0ull << get_count_order(ebx));
> +    pqosm->rmid_max = min(rmid_max, ebx);

Quoting my comment on v13: "Perhaps guard against this
degenerating to 0xffffffff, making the operations below not what
you intend, but also not fail?" In particular ...

> +    if ( pqosm->qm_features & QOS_MONITOR_TYPE_L3 )
> +    {
> +        cpuid_count(0xf, 1, &eax, &ebx, &ecx, &edx);
> +        pqosm->l3m.upscaling_factor = ebx;
> +        pqosm->l3m.rmid_max = ecx;
> +        pqosm->l3m.l3_features = edx;
> +    }
> +
> +    pqosm->rmid_max = min(rmid_max, pqosm->l3m.rmid_max);
> +    pqosm->rmid_to_dom = xmalloc_array(domid_t, pqosm->rmid_max + 1);

... this is what isn't going to do well.

> +    if ( !pqosm->rmid_to_dom )
> +    {
> +        xfree(pqosm);
> +        return;
> +    }
> +    /* Reserve RMID 0 for all domains not being monitored */
> +    pqosm->rmid_to_dom[0] = DOMID_XEN;
> +    for ( rmid = 1; rmid <= pqosm->rmid_max; rmid++ )
> +        pqosm->rmid_to_dom[rmid] = DOMID_INVALID;
> +
> +    printk(XENLOG_INFO "Platform QoS Monitoring Enabled.\n");
> +}
> +
> +void __init init_platform_qos(void)
> +{
> +    if ( opt_pqos && opt_pqos_monitor && opt_rmid_max )

Actually - what's the purpose of the pqos_monitor sub-option
on the command line with it being possible to disable monitoring
with "rmid_max=0"? Was it that this is solely in preparation of
future other QoS things?

Jan


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