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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 26.03.14 at 07:35, <dongxiao.xu@xxxxxxxxx> wrote:
> +static int select_socket_cpu(cpumask_t *cpu_bitmap)
> +{
> +    int i;

unsigned int

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

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.

> +        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?

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

Jan


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