From mboxrd@z Thu Jan  1 00:00:00 1970
From: Andrew Cooper <andrew.cooper3@citrix.com>
Subject: Re: [PATCH v6 2/7] x86: dynamically attach/detach CQM
 service for a guest
Date: Mon, 20 Jan 2014 13:17:05 +0000
Message-ID: <52DD21D1.5010202@citrix.com>
References: <1386236334-15410-1-git-send-email-dongxiao.xu@intel.com>
	<1386236334-15410-3-git-send-email-dongxiao.xu@intel.com>
	<52DD2F1A02000078001150A4@nat28.tlf.novell.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <52DD2F1A02000078001150A4@nat28.tlf.novell.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Jan Beulich <JBeulich@suse.com>
Cc: keir@xen.org, Ian.Campbell@citrix.com, stefano.stabellini@eu.citrix.com, dario.faggioli@citrix.com, Ian.Jackson@eu.citrix.com, xen-devel@lists.xen.org, Dongxiao Xu <dongxiao.xu@intel.com>, dgdegra@tycho.nsa.gov
List-Id: xen-devel@lists.xenproject.org

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
>