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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Wed, 2013-03-13 at 15:02 +0000, Konrad Rzeszutek Wilk wrote:
> 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@xxxxxxxxxx>
> > > ---
> > >  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.

You should probably double check the call chain to make sure it
propagates the way you think. Remember that libc (not libxc) translates
the return value of the underlying ioctl used to make the hypercall from
-Efoo into -1 + errno=Efoo. Some libxc call paths then jump through
hoops to turn this back into -Efoo :-( Some libxc calls do either
depending on the error path they take :-((

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

I prefer -1+errno to -errno myself but libxc is such a mess that the
important thing is that this call really does behave the way you
think...

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

Wouldn't those be per-claim properties? Whereas this call is returning
global claim state?

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

Looking again it seems there is precedent for both ways, so how you have
it should be fine.

BTW libxl_get_claiminfo should probably call the init function before
filling in the values.

Ian.


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