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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

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