Re: [Xen-devel] [PATCH v20 10/10] tools: CMDs and APIs for Cache Monitoring Technology

[Wei Liu <wei.liu2@xxxxxxxxxx>]

Thanks for this quick turnaround.

Overall this looks good to me. Just some more questions on one thing I
don't understand.

On Fri, Oct 03, 2014 at 07:55:15PM +0800, Chao Peng wrote:
[...]
> +int libxl__pick_random_socket_cpu(libxl__gc *gc, uint32_t socketid)
> +{

This name is clearer.

But still, why randomization is required?

Does this mean picking arbitrary CPU returns the same result to library
user? If so, why randomization is required?

> +    int i, j, cpu, nr_cpus;
> +    libxl_cputopology *topology;
> +    int *socket_cpus;
> +
> +    topology = libxl_get_cpu_topology(CTX, &nr_cpus);
> +    if (!topology)
> +        return ERROR_FAIL;
> +
> +    socket_cpus = libxl__malloc(gc, sizeof(int) * nr_cpus);
> +
> +    for (i = 0, j = 0; i < nr_cpus; i++)
> +        if (topology[i].socket == socketid)
> +            socket_cpus[j++] = i;
> +
> +    /* load balance among cpus in the same socket. */
> +    cpu = socket_cpus[rand() % j];
> +

There is one bug I can see. If socketid is not a valid id, j is not
incremented. Then here you will have "divided by 0" error.

Although this is internal function, I don't see any sanity check in the
public function that calls into this helper. In some instances it's even
the first helper function that gets called.

My suggestion is that you make sure j is not 0 before dividing; return
ERROR_INVAL otherwise.


> +    libxl_cputopology_list_free(topology, nr_cpus);

(of course, don't forget to call this in error path as well)

Wei.

> +    return cpu;
> +}
> +

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