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

["Xu, Dongxiao" <dongxiao.xu@xxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: Monday, January 20, 2014 9:14 PM
> To: Xu, Dongxiao
> Cc: andrew.cooper3@xxxxxxxxxx; dario.faggioli@xxxxxxxxxx;
> Ian.Campbell@xxxxxxxxxx; Ian.Jackson@xxxxxxxxxxxxx;
> stefano.stabellini@xxxxxxxxxxxxx; xen-devel@xxxxxxxxxxxxx;
> konrad.wilk@xxxxxxxxxx; dgdegra@xxxxxxxxxxxxx; keir@xxxxxxx
> Subject: Re: [PATCH v6 2/7] x86: dynamically attach/detach CQM service for a
> guest
> 
> >>> 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).

For the assignment of RMID, I don't think there will be error of ENOMEM, so I 
think EUSER will be better here?

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

Will re-format in following patch.

> 
> > +bool_t system_supports_cqm(void)
> > +{
> > +    return !!cqm;
> 
> So here we go (wrt the remark on patch 1).

Yes

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

Will change it to simple spin_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?

Could you help to elaborate the race condition here?

Per my understanding, there can't be any competition over who owns the entry. 
Besides, setting it
back to DOMID_INVALID won't race with the allocation loop.

Thanks,
Dongxiao

> 
> Jan


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