Re: [PATCH 6/6] xl: 'xl list' supports '-c' for global claim information.

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Wed, Mar 13, 2013 at 10:51:18AM +0000, Ian Campbell wrote:
> On Mon, 2013-03-11 at 14:20 +0000, Konrad Rzeszutek Wilk wrote:
> > When guests have XENMEM_claim_pages called, they influence a global
> > counter value - which has the cumulative count of the number of
> > pages requested by all domains. This value is provided via the
> > XENMEM_get_unclaimed_pages hypercall. The value fluctuates quite
> > often so the value is stale once it is provided to the user-space.
> > However it is useful for diagnostic purposes.
> > 
> > [v1: s/unclaimed/outstanding/]
> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > ---
> >  tools/libxl/libxl.c         | 12 ++++++++++++
> >  tools/libxl/libxl.h         |  1 +
> >  tools/libxl/libxl_types.idl |  4 ++++
> >  tools/libxl/xl_cmdimpl.c    | 29 +++++++++++++++++++++++++----
> >  tools/libxl/xl_cmdtable.c   |  4 +++-
> >  5 files changed, 45 insertions(+), 5 deletions(-)
> > 
> > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> > index 0745888..fd5d725 100644
> > --- a/tools/libxl/libxl.c
> > +++ b/tools/libxl/libxl.c
> > @@ -4051,6 +4051,18 @@ libxl_numainfo *libxl_get_numainfo(libxl_ctx *ctx, int *nr)
> >      return ret;
> >  }
> >  
> > +int libxl_get_claiminfo(libxl_ctx *ctx, libxl_claiminfo *claiminfo)
> > +{
> > +    long l;
> > +
> > +    l = xc_domain_get_outstanding_pages(ctx->xch);
> > +    if (l == -ENOSYS)
> 
> Does this function really return -errno and not -1 + set errno on error?

It should be -Exxxx. I need to double check with a hypervisor that does not
have this hypercall implemented to make sure it actually does return
a proper -E value.
> 
> libxc is a bit crap^Wconfused in this respect but I don't see the
> frobbing in the do_memory_op call path to turn from the -1+errno you get
> from ioctl() into -errno which some call chains in libxc have.

I can add it via the errno (in libxc) if you think it would be better?

> 
> > +        return l;
> > +    claiminfo->claimed = l;
> > +
> > +    return 0;
> > +}
> > +
> >  const libxl_version_info* libxl_get_version_info(libxl_ctx *ctx)
> >  {
> >      union {
> > diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> > index 538bf93..8d0ab23 100644
> > --- a/tools/libxl/libxl.h
> > +++ b/tools/libxl/libxl.h
> > @@ -581,6 +581,7 @@ int libxl_wait_for_memory_target(libxl_ctx *ctx, uint32_t domid, int wait_secs);
> >  
> >  /* Parse the claim_mode options */
> >  int libxl_parse_claim_mode(const char *s, unsigned int *flag);
> > +int libxl_get_claiminfo(libxl_ctx *ctx, libxl_claiminfo *claiminfo);
> 
> Most similar interfaces in libxl seem to either return
> libxl_claiminfo->claimed directly or return the struct.
> 
> Is there likely to be other info in this struct in the future?

Yes and no. Originally there were the 'flags' and there were three of them:
on, off, tmem.

There was also suggestion to add NUMA capability to it. But right now the
hypercall expects _no_ flags. This could change in the future.

Do you think it would be better to just slim down this structure and if the
flags get added back, then expand this structure?

I am happy with either solution.

> 
> >  
> >  int libxl_vncviewer_exec(libxl_ctx *ctx, uint32_t domid, int autopass);
> >  int libxl_console_exec(libxl_ctx *ctx, uint32_t domid, int cons_num, libxl_console_type type);
> > diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> > index 0a8b99a..aa71ed8 100644
> > --- a/tools/libxl/libxl_types.idl
> > +++ b/tools/libxl/libxl_types.idl
> > @@ -509,6 +509,10 @@ libxl_cputopology = Struct("cputopology", [
> >      ("node", uint32),
> >      ], dir=DIR_OUT)
> >  
> > +libxl_claiminfo = Struct("claiminfo", [
> > +    ("claimed", uint64),
> 
> What are the units? Should this be a MemKB or ....

pages. Thought that is a bit confusing to users. So MemKB is a better
choice. Let me update that.
> 
> > +    ])
> > +
> >  libxl_sched_credit_params = Struct("sched_credit_params", [
> >      ("tslice_ms", integer),
> >      ("ratelimit_us", integer),
> [...]
> > @@ -4663,18 +4681,21 @@ int main_info(int argc, char **argv)
> >      int opt;
> >      static struct option opts[] = {
> >          {"numa", 0, 0, 'n'},
> > +        {"claim", 0, 0, 'c'},
> 
> Unlike numa this is just one line, I reckon it could be printed
> unconditionally.

OK.
> 
> >          COMMON_LONG_OPTS,
> >          {0, 0, 0, 0}
> >      };
> > -    int numa = 0;
> > +    int numa = 0, claim = 0;
> >  
> > -    SWITCH_FOREACH_OPT(opt, "hn", opts, "info", 0) {
> > +    SWITCH_FOREACH_OPT(opt, "hnc", opts, "info", 0) {
> >      case 'n':
> >          numa = 1;
> >          break;
> > +    case 'c':
> > +        claim = 1;
> >      }
> >  
> > -    print_info(numa);
> > +    print_info(numa, claim);
> >      return 0;
> >  }
> >  
> > diff --git a/tools/libxl/xl_cmdtable.c b/tools/libxl/xl_cmdtable.c
> > index b4a87ca..a48dbb0 100644
> > --- a/tools/libxl/xl_cmdtable.c
> > +++ b/tools/libxl/xl_cmdtable.c
> > @@ -226,7 +226,9 @@ struct cmd_spec cmd_table[] = {
> >      { "info",
> >        &main_info, 0, 0,
> >        "Get information about Xen host",
> > -      "-n, --numa         List host NUMA topology information",
> > +      "[-n|-c]",
> > +      "-n, --numa         List host NUMA topology information\n"
> > +      "-c, --claim        List claim information",
> >      },
> >      { "sharing",
> >        &main_sharing, 0, 0,
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>