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

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 20/01/14 13:13, Jan Beulich wrote:
>>>> On 05.12.13 at 10:38, Dongxiao Xu <dongxiao.xu@intel.com> 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).

-EUSERS is correct here.  This failure like this means "all the
available system rmid's are already being used by other domains".

~Andrew

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