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

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

> -----Original Message-----
> From: Dario Faggioli [mailto:dario.faggioli@xxxxxxxxxx]
> Sent: Tuesday, December 03, 2013 8:17 PM
> To: Xu, Dongxiao
> Cc: xen-devel@xxxxxxxxxxxxx; keir@xxxxxxx; Ian.Campbell@xxxxxxxxxx;
> stefano.stabellini@xxxxxxxxxxxxx; andrew.cooper3@xxxxxxxxxx;
> Ian.Jackson@xxxxxxxxxxxxx; JBeulich@xxxxxxxx; dgdegra@xxxxxxxxxxxxx
> Subject: Re: [Xen-devel] [PATCH v4 7/7] tools: enable Cache QoS Monitoring
> feature for libxl/libxc
> 
> On mar, 2013-12-03 at 16:47 +0800, Dongxiao Xu wrote:
> > Introduced two new xl commands to attach/detach CQM service for a guest
> > $ xl pqos-attach cqm domid
> > $ xl pqos-detach cqm domid
> >
> > Introduce one new xl command to retrive guest CQM information
> > $ xl pqos-list cqm (domid)
> >
> > Signed-off-by: Jiongxi Li <jiongxi.li@xxxxxxxxx>
> > Signed-off-by: Dongxiao Xu <dongxiao.xu@xxxxxxxxx>
> > ---
> >  tools/libxc/xc_domain.c   |   47 +++++++++++++++
> >  tools/libxc/xenctrl.h     |   11 ++++
> >  tools/libxl/Makefile      |    3 +-
> >  tools/libxl/libxl.h       |    5 ++
> >  tools/libxl/libxl_pqos.c  |  108 +++++++++++++++++++++++++++++++++
> >  tools/libxl/xl.h          |    3 +
> >  tools/libxl/xl_cmdimpl.c  |  146
> +++++++++++++++++++++++++++++++++++++++++++++
> >  tools/libxl/xl_cmdtable.c |   15 +++++
> >  8 files changed, 337 insertions(+), 1 deletion(-)
> >  create mode 100644 tools/libxl/libxl_pqos.c
> >
> Would it be possible to split this patch in 3, one for libxc, one for
> libxl and one for xl?

Originally the patch is split (by functional), and later I merged them 
according to Andrew's suggestion.
I think merge is okay since the logic is simple and straightforward.

> 
> 
> > diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> > index c7dceda..fdca92d 100644
> > --- a/tools/libxl/libxl.h
> > +++ b/tools/libxl/libxl.h
> > @@ -285,6 +285,7 @@
> >
> >  #include <libxl_uuid.h>
> >  #include <_libxl_list.h>
> > +#include <xenctrl.h>
> >
> Is this really necessary? I think it shouldn't... <xenctrl.h> is already
> included in "libxl_internal.h", which you are including yourself below,
> so...

libxl.h will reference "sysctl_cqminfo_t", which is defined in xenctrl.h.
I didn't see libxl_internal.h is included in libxl.h, can you help to point it 
out?

> 
> > diff --git a/tools/libxl/libxl_pqos.c b/tools/libxl/libxl_pqos.c
> > new file mode 100644
> > index 0000000..bf7593a
> > --- /dev/null
> > +++ b/tools/libxl/libxl_pqos.c
> > @@ -0,0 +1,108 @@
> > +/*
> > + * Copyright (C) 2013      Intel Corporation
> > + * Author Jiongxi Li <jiongxi.li@xxxxxxxxx>
> > + * Author Dongxiao Xu <dongxiao.xu@xxxxxxxxx>
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU Lesser General Public License as published
> > + * by the Free Software Foundation; version 2.1 only. with the special
> > + * exception on linking described in file LICENSE.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU Lesser General Public License for more details.
> > + */
> > +
> > +#include "libxl_osdeps.h" /* must come before any other headers */
> > +#include "libxl_internal.h"
> 
> > +int libxl_pqos_attach(libxl_ctx *ctx, uint32_t domid, const char * 
> > qos_type)
> > +{
> > +    int rc;
> > +    uint32_t flags = 0;
> > +
> > +    if (!strncmp(qos_type, "cqm", 3))
> > +        flags |= XEN_DOMCTL_pqos_cqm;
> > +    else {
> > +        rc = -EINVAL;
> > +        LIBXL__LOG(ctx, XTL_ERROR, "%s", msg[EINVAL]);
> >
> I think new code should use the LOG() / LOGE() variant of the logging
> macros.

I saw a lot of existing code still uses LIBXL__LOG() function.
Besides, if we use LOGE(), we need to pass another variable "gc" to the 
function...
Is this a guideline that we will stick to LOG()/LOGE() and deprecate calling of 
LIBXL__LOG() in later code?

> 
> > +        return rc;
> > +    }
> >
> libxl functions should return libxl error codes.

Okay.

> 
> > +
> > +    rc = xc_domain_pqos_attach(ctx->xch, domid, flags);
> > +    if (rc < 0) {
> > +        switch(errno) {
> > +        case EINVAL:
> > +        case ENODEV:
> > +        case EEXIST:
> > +        case EUSERS:
> > +        case ESRCH:
> > +            LIBXL__LOG(ctx, XTL_ERROR, "%s", msg[errno]);
> > +            break;
> > +        default:
> > +            LIBXL__LOG(ctx, XTL_ERROR, "errno: %d", errno);
> > +        }
> > +    }
> > +
> Mmm... Isn't there a better place where to do this demultiplexing and
> printing the correct message? Looking around, in libxl, what usually
> happens is printing something generic enough, together with the errno
> value/message (with LOGE), and then rely on lower layers to provide a
> more detailed explanation of what happened.
> 
> This is certainly up to a maintainer to decide, but I think you at least
> (you're doing this twice) should do it in a function rather than
> duplicating the whole switch.

Will abstract the logic into an internal function.

Thanks,
Dongxiao

> 
> Regards,
> Dario
> 
> --
> <<This happens because I choose it to happen!>> (Raistlin Majere)
> -----------------------------------------------------------------
> Dario Faggioli, Ph.D, http://about.me/dario.faggioli
> Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)

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