Re: [Xen-devel] [PATCH v12 5/9] x86: dynamically attach/detach QoS monitoring service for a guest

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

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: Friday, July 04, 2014 8:06 PM
> To: Xu, Dongxiao
> Cc: andrew.cooper3@xxxxxxxxxx; Ian.Campbell@xxxxxxxxxx;
> George.Dunlap@xxxxxxxxxxxxx; Ian.Jackson@xxxxxxxxxxxxx;
> stefano.stabellini@xxxxxxxxxxxxx; xen-devel@xxxxxxxxxxxxx;
> konrad.wilk@xxxxxxxxxx; dgdegra@xxxxxxxxxxxxx; keir@xxxxxxx
> Subject: Re: [PATCH v12 5/9] x86: dynamically attach/detach QoS monitoring
> service for a guest
> 
> >>> On 04.07.14 at 10:34, <dongxiao.xu@xxxxxxxxx> wrote:
> > +    case XEN_DOMCTL_pqos_monitor_op:
> > +        if ( !pqos_monitor_enabled() )
> > +        {
> > +            ret = -ENODEV;
> > +            break;
> > +        }
> > +
> > +        switch ( domctl->u.pqos_monitor_op.cmd )
> > +        {
> > +        case XEN_DOMCTL_PQOS_MONITOR_OP_ATTACH:
> > +            ret = pqos_monitor_alloc_rmid(d);
> > +            break;
> 
> Wouldn't it make sense to return the allocated rmid from this operation?

The current return type for this function is int, where 0 stands correct, and 
-EXXX stands for error.
However rmid is "unsigned int" type, which leads to type conflict.

The allocated rmid will be stored to domain structure inside the function.

Thanks,
Dongxiao

> 
> > +int pqos_monitor_alloc_rmid(struct domain *d)
> > +{
> > +    int rc = 0;
> > +    unsigned int rmid;
> > +
> > +    ASSERT(pqos_monitor_enabled());
> > +
> > +    if ( d->arch.pqos_rmid > 0 )
> > +    {
> > +        rc = -EEXIST;
> > +        return rc;
> 
> "return -EEXIST" please (unless you're being paid my number of
> lines of code).
> 
> > +    }
> > +
> > +    for ( rmid = pqosm->rmid_min; rmid <= pqosm->rmid_max; rmid++ )
> > +    {
> > +        if ( pqosm->rmid_to_dom[rmid] != DOMID_INVALID)
> > +            continue;
> > +
> > +        pqosm->rmid_to_dom[rmid] = d->domain_id;
> > +        break;
> > +    }
> > +
> > +    /* No RMID available, assign RMID=0 by default */
> > +    if ( rmid > pqosm->rmid_max )
> > +    {
> > +        rmid = 0;
> > +        rc = -EUSERS;
> > +    }
> > +    else
> > +        pqosm->rmid_inuse++;
> > +
> > +    d->arch.pqos_rmid = rmid;
> > +
> > +    return rc;
> > +}
> > +
> > +void pqos_monitor_free_rmid(struct domain *d)
> > +{
> > +    unsigned int rmid;
> > +
> > +    rmid = d->arch.pqos_rmid;
> > +    /* We do not free system reserved "RMID=0" */
> > +    if ( rmid == 0 )
> > +        return;
> > +
> > +    pqosm->rmid_to_dom[rmid] = DOMID_INVALID;
> > +    d->arch.pqos_rmid = 0;
> > +    pqosm->rmid_inuse--;
> > +}
> 
> This pair of functions should get a brief comment added explaining
> why explicit locking here isn't needed.
> 
> > --- a/xen/include/asm-x86/pqos.h
> > +++ b/xen/include/asm-x86/pqos.h
> > @@ -16,6 +16,7 @@
> >  #ifndef __ASM_PQOS_H__
> >  #define __ASM_PQOS_H__
> >
> > +#include <xen/sched.h>
> 
> You'd get away with just xen/types.h here if you forward declared
> struct domain below.
> 
> Jan


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