[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v12 5/9] x86: dynamically attach/detach QoS monitoring service for a guest
> -----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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |