Re: [Xen-devel] [PATCH v6 2/7] x86: dynamically attach/detach CQM service for a guest

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 05.12.13 at 10:38, Dongxiao Xu <dongxiao.xu@xxxxxxxxx> wrote:
> @@ -1223,6 +1224,45 @@ long arch_do_domctl(
>      }
>      break;
>  
> +    case XEN_DOMCTL_attach_pqos:
> +    {
> +        if ( domctl->u.qos_type.flags & XEN_DOMCTL_pqos_cqm )
> +        {
> +            if ( !system_supports_cqm() )
> +                ret = -ENODEV;
> +            else if ( d->arch.pqos_cqm_rmid > 0 )
> +                ret = -EEXIST;
> +            else
> +            {
> +                ret = alloc_cqm_rmid(d);
> +                if ( ret < 0 )
> +                    ret = -EUSERS;

Why don't you have the function return a sensible error code
(which presumably might also end up being other than -EUSERS,
e.g. -ENOMEM).

> +            }
> +        }
> +        else
> +            ret = -EINVAL;
> +    }
> +    break;
> +
> +    case XEN_DOMCTL_detach_pqos:
> +    {
> +        if ( domctl->u.qos_type.flags & XEN_DOMCTL_pqos_cqm )
> +        {
> +            if ( !system_supports_cqm() )
> +                ret = -ENODEV;
> +            else if ( d->arch.pqos_cqm_rmid > 0 )
> +            {
> +                free_cqm_rmid(d);
> +                ret = 0;
> +            }
> +            else
> +                ret = -ENOENT;
> +        }
> +        else
> +            ret = -EINVAL;
> +    }
> +    break;

For consistency, both of the above would better be changed to a
single series of if()/else if().../else.

> +bool_t system_supports_cqm(void)
> +{
> +    return !!cqm;

So here we go (wrt the remark on patch 1).

> +}
> +
> +int alloc_cqm_rmid(struct domain *d)
> +{
> +    int rc = 0;
> +    unsigned int rmid;
> +    unsigned long flags;
> +
> +    ASSERT(system_supports_cqm());
> +
> +    spin_lock_irqsave(&cqm_lock, flags);

Why not just spin_lock()? Briefly scanning over the following patches
doesn't point out anything that might require this to be an IRQ-safe
lock.

> +    for ( rmid = cqm->min_rmid; rmid <= cqm->max_rmid; rmid++ )
> +    {
> +        if ( cqm->rmid_to_dom[rmid] != DOMID_INVALID)
> +            continue;
> +
> +        cqm->rmid_to_dom[rmid] = d->domain_id;
> +        break;
> +    }
> +    spin_unlock_irqrestore(&cqm_lock, flags);
> +
> +    /* No CQM RMID available, assign RMID=0 by default */
> +    if ( rmid > cqm->max_rmid )
> +    {
> +        rmid = 0;
> +        rc = -1;
> +    }
> +
> +    d->arch.pqos_cqm_rmid = rmid;

Is it really safe to do this and the freeing below outside of the
lock?

Jan


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