Re: [Xen-devel] [PATCH v10 6/6] tools: enable Cache QoS Monitoring feature for libxl/libxc

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

> -----Original Message-----
> From: Ian Campbell [mailto:Ian.Campbell@xxxxxxxxxx]
> Sent: Friday, March 28, 2014 5:44 PM
> To: Xu, Dongxiao
> Cc: xen-devel@xxxxxxxxxxxxx; keir@xxxxxxx; JBeulich@xxxxxxxx;
> Ian.Jackson@xxxxxxxxxxxxx; stefano.stabellini@xxxxxxxxxxxxx;
> andrew.cooper3@xxxxxxxxxx; konrad.wilk@xxxxxxxxxx; dgdegra@xxxxxxxxxxxxx
> Subject: Re: [PATCH v10 6/6] tools: enable Cache QoS Monitoring feature for
> libxl/libxc
> 
> On Fri, 2014-03-28 at 00:53 +0000, Xu, Dongxiao wrote:
> > > -----Original Message-----
> > > From: Ian Campbell [mailto:Ian.Campbell@xxxxxxxxxx]
> > > Sent: Thursday, March 27, 2014 11:14 PM
> > > To: Xu, Dongxiao
> > > Cc: xen-devel@xxxxxxxxxxxxx; keir@xxxxxxx; JBeulich@xxxxxxxx;
> > > Ian.Jackson@xxxxxxxxxxxxx; stefano.stabellini@xxxxxxxxxxxxx;
> > > andrew.cooper3@xxxxxxxxxx; konrad.wilk@xxxxxxxxxx;
> dgdegra@xxxxxxxxxxxxx
> > > Subject: Re: [PATCH v10 6/6] tools: enable Cache QoS Monitoring feature 
> > > for
> > > libxl/libxc
> > >
> > > On Wed, 2014-03-26 at 14:35 +0800, Dongxiao Xu wrote:
> > > [...]
> > > > +int libxl_pqos_attach(libxl_ctx *ctx, uint32_t domid, libxl_pqos_type
> > > qos_type);
> > > > +int libxl_pqos_detach(libxl_ctx *ctx, uint32_t domid, libxl_pqos_type
> > > qos_type);
> > > > +libxl_cqminfo * libxl_getcqminfo(libxl_ctx *ctx, unsigned int 
> > > > *l3c_total,
> > > > +    unsigned int *nr_domains, unsigned int *nr_rmids, unsigned int
> > > *nr_sockets);
> > > [...]
> > > > +libxl_cqminfo = Struct("cqminfo", [
> > > > +    ("valid",         uint32),
> > > > +    ("l3c",           uint64),
> > > > +    ])
> > >
> > > libxl_getcqminfo seems to have a strange mixture of returning an info
> > > struct and returning things via pointers passed as integers. why is
> > > that?
> >
> > The returned info is a unit data structure containing the L3 cache usage 
> > data for
> certain RMID on certain socket.
> > These pointers passed integers tell caller the number of such units are
> returned.
> >
> > This follows one existing function:
> > libxl_dominfo * libxl_list_domain(libxl_ctx *ctx, int *nb_domain_out);
> > where the return value is the unit dominfo data structure, and
> *nb_domain_out tells caller how many data structure units are returned.
> 
> But libxl_getqminfo has three such nr_* pointers as parameters, which
> one is the length of the returned array? What are the other two? How
> does one enumerate the things which the other nr_* refer to?

The size is calculated by nr_domains*nr_sockets. The nr_rmids is to show how 
many RMIDs a system has.

> 
> > > > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> > > > index 8389468..664a34d 100644
> > > > --- a/tools/libxl/xl_cmdimpl.c
> > > > +++ b/tools/libxl/xl_cmdimpl.c
> > > > @@ -7381,6 +7381,115 @@ out:
> > > >      return ret;
> > > >  }
> > > >
> > > > +int main_pqosattach(int argc, char **argv)
> > > > +{
> > > > +    uint32_t domid;
> > > > +    int opt, rc;
> > > > +    libxl_pqos_type qos_type;
> > > > +
> > > > +    SWITCH_FOREACH_OPT(opt, "", NULL, "pqos-attach", 2) {
> > > > +        /* No options */
> > > > +    }
> > > > +
> > > > +    /* libxl_pqos_attach will handle the parameter check. */
> > > > +    libxl_pqos_type_from_string(argv[optind], &qos_type);
> > >
> > > This can fail, I think you need to check the return value.
> >
> > Yes, I am aware of that.
> > The following libxl_pqos_attach() function will check the parameter
> correctness.
> > Actually I put a comment in above (/* libxl_pqos_attach will handle the
> parameter check. */).
> 
> If libxl_pqos_type_from_string fails then qos_type remains undefined, so
> you can't rely on some later function checking for correctness, the
> undefined value may happen to appear valid.

Okay, I will add a check in the function.

Thanks,
Dongxiao

> 
> Ian.

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