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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 28.01.14 at 15:09, "Xu, Dongxiao" <dongxiao.xu@xxxxxxxxx> wrote:
>> >>> 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?

-EUSER is certainly fine here, but that wasn't my point. My point was
that alloc_cqm_rmid() should return a proper error code (right now
only -EUSER, but _potentially_ there could be others in the future,
_for example_ -ENOMEM), and that error code should simply get
passed up here.

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

I wasn't saying there is one. I was asking whether you thought
about whether there might be one. After all, from simply looking
at it I get the impression that two racing calls to this function
might end up leaking one of the two RMIDs (second instance
blindly overwriting what the first instance stored).

Jan


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