Re: [Xen-devel] [PATCH v10 3/6] x86: collect CQM information from all sockets

["Xu, Dongxiao" <dongxiao.xu@xxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: Wednesday, April 02, 2014 11:36 PM
> To: Xu, Dongxiao
> Cc: andrew.cooper3@xxxxxxxxxx; Ian.Campbell@xxxxxxxxxx;
> Ian.Jackson@xxxxxxxxxxxxx; stefano.stabellini@xxxxxxxxxxxxx;
> xen-devel@xxxxxxxxxxxxx; konrad.wilk@xxxxxxxxxx; dgdegra@xxxxxxxxxxxxx;
> keir@xxxxxxx
> Subject: Re: [PATCH v10 3/6] x86: collect CQM information from all sockets
> 
> >>> On 26.03.14 at 07:35, <dongxiao.xu@xxxxxxxxx> wrote:
> > +static int select_socket_cpu(cpumask_t *cpu_bitmap)
> > +{
> > +    int i;
> 
> unsigned int

Okay.

> 
> > +    unsigned int cpu;
> > +    int socket, socket_curr = cpu_to_socket(smp_processor_id());
> > +    cpumask_var_t sockets;
> > +
> > +    if ( !zalloc_cpumask_var(&sockets) )
> > +        return -ENOMEM;
> > +
> > +    if ( socket_curr >= 0 )
> > +        set_bit(socket_curr, sockets);
> > +
> > +    cpumask_clear(cpu_bitmap);
> > +    for ( i = 0; i < NR_CPUS; i++ )
> > +    {
> > +        socket = cpu_to_socket(i);
> > +        if ( socket < 0 || test_and_set_bit(socket, sockets) )
> > +            continue;
> > +        cpu = cpumask_any(per_cpu(cpu_core_mask, i));
> 
> Is cpu_to_socket() guaranteed to return negative values for all
> offline CPUs at all times? If not, the per_cpu() access may end
> up being invalid.

You are correct. I didn't see the related per_cpu() variable initialized to 
BAD_APICID for all CPUs (offline ones).
Maybe cpu_online_map is a better way here to enumerate all the existing CPUs.

> 
> And of course the test_and_set_bit() above can corrupt memory
> for sparse socket maps (remember in particular that
> alloc_cpumask_var() allocates nr_cpumask_bits bits, not NR_CPUS
> of them).
> 
> > +    case XEN_SYSCTL_getcqminfo:
> > +    {
> > +        struct cpuinfo_x86 *c = &boot_cpu_data;
> 
> This variable appears to be used just once, i.e. is rather pointless.

Okay.

> 
> > +        cpumask_var_t cpu_cqmdata_map;
> > +
> > +        if ( !system_supports_cqm() )
> > +        {
> > +            ret = -ENODEV;
> > +            break;
> > +        }
> > +
> > +        if ( !zalloc_cpumask_var(&cpu_cqmdata_map) )
> > +        {
> > +            ret = -ENOMEM;
> > +            break;
> > +        }
> > +
> > +        ret = select_socket_cpu(cpu_cqmdata_map);
> > +        if ( ret < 0 )
> > +        {
> > +            free_cpumask_var(cpu_cqmdata_map);
> > +            break;
> > +        }
> > +
> > +        get_cqm_info(cpu_cqmdata_map);
> > +
> > +        sysctl->u.getcqminfo.socket_l3c_mfn =
> virt_to_mfn(cqm->socket_l3c_mfn);
> > +        sysctl->u.getcqminfo.rmid_dom_mfn =
> virt_to_mfn(cqm->rmid_to_dom);
> > +        sysctl->u.getcqminfo.nr_rmids = cqm->rmid_max + 1;
> > +        sysctl->u.getcqminfo.nr_sockets =
> cpumask_weight(cpu_cqmdata_map) + 1;
> > +        sysctl->u.getcqminfo.l3c_total = c->x86_cache_size;
> 
> Is this really always the L3 size (i.e. zero if there are only L1 and L2
> caches)? Or does system_supports_cqm() imply this?

I didn't find such statement in the SDM.
I will add the check of CPUID(4) to find whether L3 cache is enabled or not. If 
it is not enabled, CQM will be disabled too. 

> 
> > --- a/xen/include/asm-x86/pqos.h
> > +++ b/xen/include/asm-x86/pqos.h
> > @@ -17,6 +17,8 @@
> >  #ifndef ASM_PQOS_H
> >  #define ASM_PQOS_H
> >  #include <xen/sched.h>
> > +#include <xen/cpumask.h>
> > +#include <public/domctl.h>
> 
> What is this second #include doing here?

Will remove it.

Dongxiao

> 
> Jan


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