Re: [Xen-devel] [PATCH v2 4/7] x86: add support for COS/CBM manangement

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 19.03.15 at 11:41, <chao.p.peng@xxxxxxxxxxxxxxx> wrote:
> +static unsigned int get_socket_cpu(unsigned int socket)
> +{
> +    unsigned int cpu;
> +
> +    for_each_online_cpu ( cpu )
> +       if ( cpu_to_socket(cpu) == socket )
> +           return cpu;
> +    return nr_cpu_ids;
> +}

This can be a rather long loop for a huge system. I think you need to
find some better solution for this.

> +static void psr_free_cos(struct domain *d)
> +{
> +    unsigned int socket;
> +    unsigned int cos;
> +    struct psr_cat_cbm *map;
> +
> +    if( !d->arch.psr_cos_ids )
> +        return;
> +
> +    for ( socket = 0; socket < opt_num_sockets; socket++)
> +    {
> +        cos = d->arch.psr_cos_ids[socket];
> +        if ( cos == 0 )
> +            continue;
> +
> +        map = cat_socket_info[socket].cos_cbm_map;
> +        if ( map )
> +            map[cos].ref--;

Can map really ever be NULL here? I.e. isn't this rather an
ASSERT() instead of if()?

> @@ -265,6 +474,17 @@ static void cat_cpu_init(unsigned int cpu)
>          info->cbm_len = (eax & 0x1f) + 1;
>          info->cos_max = (edx & 0xffff);
>  
> +        info->cos_cbm_map = xmalloc_array(struct psr_cat_cbm,
> +                                          info->cos_max + 1UL);
> +        if ( !info->cos_cbm_map )
> +            return;
> +
> +        for ( cos = 0; cos <= info->cos_max; cos++ )
> +            info->cos_cbm_map[cos].ref = 0;

So why not simply xzalloc_array()?

> --- a/xen/arch/x86/sysctl.c
> +++ b/xen/arch/x86/sysctl.c
> @@ -171,6 +171,26 @@ long arch_do_sysctl(
>  
>          break;
>  
> +    case XEN_SYSCTL_psr_cat_op:
> +        switch ( sysctl->u.psr_cat_op.cmd )
> +        {
> +        case XEN_SYSCTL_PSR_CAT_get_l3_info:
> +            ret = psr_get_cat_l3_info(sysctl->u.psr_cat_op.target,
> +                                      
> &sysctl->u.psr_cat_op.u.l3_info.cbm_len,
> +                                      
> &sysctl->u.psr_cat_op.u.l3_info.cos_max);
> +            if ( ret )
> +                break;
> +
> +            if ( __copy_to_guest(u_sysctl, sysctl, 1) )
> +                ret = -EFAULT;

Please fold the two if()-s together.

> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -1005,6 +1005,16 @@ struct xen_domctl_psr_cmt_op {
>  typedef struct xen_domctl_psr_cmt_op xen_domctl_psr_cmt_op_t;
>  DEFINE_XEN_GUEST_HANDLE(xen_domctl_psr_cmt_op_t);
>  
> +struct xen_domctl_psr_cat_op {
> +#define XEN_DOMCTL_PSR_CAT_OP_SET_L3_CBM     0
> +#define XEN_DOMCTL_PSR_CAT_OP_GET_L3_CBM     1
> +    uint32_t cmd;       /* IN: XEN_DOMCTL_PSR_CAT_OP_* */
> +    uint32_t target;    /* IN: socket or cpu to be operated on */

How can this be socket _or_ CPU?

Jan


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