Xen devel - Apr 2010 - [PATCH 0/4] Add "xl info" command

Hi,

as already mentioned in previous emails, the "info" subcommand is 
missing from the xl tool.
The attached patchset adds support for this, extending libxl on the way 
to provide the necessary info only by using own functions.
On my system the output of xm info and xl info was identical, I omitted 
the recent NUMA additions from xl info for now and will provide the
necessary patches later.

Most of the additions are straightforward, just some words on 
libxl_get_version_info():
The xen_version hypercall uses statically allocated memory for passing 
strings (e.g. 1024 Bytes for commandline). To gather all information, 
one has to hypercall multiple times with different arg values, each 
information also has a different (static) result type.
To make this more user friendly in libxl, I created a structure holding 
all information in dynamically allocated pointers. These will be 
malloced and filled on calling libxl_get_version_info() and have to be 
freed by the caller (with libxl_free_version_info()).
To avoid doing several hypercalls and copying unnecessary memory, I used 
a bitmask to select the pieces of information the caller is interested. 
This is especially useful if one wants to know the Xen version or the 
PAGESIZE only. For xl info there is a LIBXL_VERSION_ALL_MASK to get all 
of them.

Please apply!

Regards,
Andre.

-- 
Andre Przywara
AMD-Operating System Research Center (OSRC), Dresden, Germany
Tel: +49 351 488-3567-12


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

The libxl version of the physinfo sysctl does not contain some
fields like nr_nodes or capabilities needed for xl info output.
Add them to the structure and the retrieving function.

Signed-off-by: Andre Przywara <andre.przywara@amd.com>

-- 
Andre Przywara
AMD-Operating System Research Center (OSRC), Dresden, Germany
Tel: +49 351 488-3567-12


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

To get the name of the currently used scheduler, Xen provides a sched_id 
sysctl.
Add a libxl wrapper around the libxc function to query this.

Signed-off-by: Andre Przywara <andre.przywara@amd.com>

-- 
Andre Przywara
AMD-Operating System Research Center (OSRC), Dresden, Germany
Tel: +49 351 488-3567-12


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Xen provides a xen_version hypercall to query the values of several
interesting things (like hypervisor version, commandline used,
actual changeset, etc.). Create a user-friendly and efficient
wrapper around the libxc function to provide values for xl info output.

Signed-off-by: Andre Przywara <andre.przywara@amd.com>

-- 
Andre Przywara
AMD-Operating System Research Center (OSRC), Dresden, Germany
Tel: +49 351 488-3567-12


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

The info subcommand was missing from the xl tool. Use the new libxl
wrapper functions to create a clone of "xm info". The splitting into
several smaller functions is enspired by the implementation in XendNode.py.

Signed-off-by: Andre Przywara <andre.przywara@amd.com>

-- 
Andre Przywara
AMD-Operating System Research Center (OSRC), Dresden, Germany
Tel: +49 351 488-3567-12


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On 18/04/10 22:23, Andre Przywara wrote:
> The libxl version of the physinfo sysctl does not contain some
> fields like nr_nodes or capabilities needed for xl info output.
> Add them to the structure and the retrieving function.
>
> Signed-off-by: Andre Przywara<andre.przywara@amd.com>
I think, It would be much nicer to provide the phys_caps already decoded 
instead of copying the xc value as is.

so what about something like having phys_cap_hvm and 
phys_cap_hvm_directio field directly in the structure ? If things 
extends, we should add new field in libxl anyway.

-- 
Vincent


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On 18/04/10 22:25, Andre Przywara wrote:
> To get the name of the currently used scheduler, Xen provides a sched_id
> sysctl.
> Add a libxl wrapper around the libxc function to query this.
maybe it should returns an enum libxl_sched_id instead of just a plain int ?

-- 
Vincent

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On 18/04/10 22:26, Andre Przywara wrote:
> Xen provides a xen_version hypercall to query the values of several
> interesting things (like hypervisor version, commandline used,
> actual changeset, etc.). Create a user-friendly and efficient
> wrapper around the libxc function to provide values for xl info output.
instead of this chain of if else, why don''t you just init the structure
to NULL at the beginning of the call, and not do any else ?

also the query capability seems a bit too much; the info call is 
probably unlikely called into a tight loop, and I expect the number
of call to this to be 1 per starting toolstack or 1 per vm (at worst). 
from an API point of view this would make it simpler too.

last thing, is instead of using strdup, you should use 
libxl_sprintf(ctx, "%s", string) so that you don''t have to
worry about
freeing memory at all, and you can just omit the free_version_info call 
completely.

-- 
Vincent

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Andre Przywara writes ("[Xen-devel] [PATCH 1/4] libxl: extend physinfo
structure"):
> The libxl version of the physinfo sysctl does not contain some
> fields like nr_nodes or capabilities needed for xl info output.
> Add them to the structure and the retrieving function.
Rather than this:

  --- a/tools/libxl/libxl.h	Thu Apr 15 19:11:16 2010 +0100
  +++ b/tools/libxl/libxl.h	Sun Apr 18 14:34:32 2010 +0200
  +#define PHYS_CAP_HVM          1
  +#define PHYS_CAP_HVM_DIRECTIO 2

I think it would be better to expect libxl callers to include the
public Xen header file sysctl.h.  While libxl callers should not need
to know about information about the underlying libraris, the Xen
interface is a key public interface and is fine to expose - at least
as far as these kind of bitmasks.

That also avoids the possibility of getting these #defines out of step
with the underlying code (or even the possibility of having to write
conversion code!)

Certainly if you _don''t_ convert, you shouldn''t redefine the
bitfield
constants.

Vincent Hanquez writes ("Re: [Xen-devel] [PATCH 1/4] libxl: extend physinfo
structure"):
> I think, It would be much nicer to provide the phys_caps already decoded 
> instead of copying the xc value as is.
Is it really worth reprocessing a bitfield like this which is after
all not actually going to be used much by calling code ?
> so what about something like having phys_cap_hvm and 
> phys_cap_hvm_directio field directly in the structure ? If things 
> extends, we should add new field in libxl anyway.
That''s a possibility which I wouldn''t object to but it seems
unnecessary.  In that case it should be a series of 1-bit unsigned
C bitfields rather than a set of flags in a uint32.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Andre Przywara writes ("[Xen-devel] [PATCH 2/4] libxl: add sched_get_id
function"):
> To get the name of the currently used scheduler, Xen provides a sched_id 
> sysctl.
> Add a libxl wrapper around the libxc function to query this.
There should be a doc comemnt in libxl.h saying what the return value
is.  In this case it seems to be one of the values XEN_SCHEDULER_*
from xen/include/public/domctl.h.

Vincent Hanquez writes ("Re: [Xen-devel] [PATCH 2/4] libxl: add
sched_get_id function"):
> On 18/04/10 22:25, Andre Przywara wrote:
> > To get the name of the currently used scheduler, Xen provides a
sched_id
> > sysctl.
> > Add a libxl wrapper around the libxc function to query this.
> 
> maybe it should returns an enum libxl_sched_id instead of just a plain int
?
That would be an alternative but it does involve extra source code
which TBH I think we can do without.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Andre Przywara writes ("[Xen-devel] [PATCH 3/4] libxl: add version_info
function"):
> Xen provides a xen_version hypercall to query the values of several
> interesting things (like hypervisor version, commandline used,
> actual changeset, etc.). Create a user-friendly and efficient
> wrapper around the libxc function to provide values for xl info output.
As Vincent says, I think the query mask is overkill.  I would suggeset
just doing without it, and always acquire and return all the
information.  Ie, do away with query_mask and LIBXL_VERSION_FOO_MASK
and the correspondings ifs.


+#define FREE_IF_NOT_ZERO(ptr) if((ptr) != NULL) {free(ptr); ptr = NULL;}

This is redundant because  free(NULL)  is a legal no-op.  It is also
bad macro practice to include an unterminated if like that - it might
eat subsequent elses.  Furthermore, you should wrap ptr up in parens
to avoid macro precedence problems.

So you should write something like one of these:

+#define FREE_AND_ZERO(ptr)  (free((ptr), (ptr) = 0)
+#define FREE_AND_ZERO(ptr)  do{ free((ptr)); (ptr) = 0; }while(0)

Finally, this macro should be in libxl_internal.h where every part of
libxl can use it.


Vincent Hanquez writes ("Re: [Xen-devel] [PATCH 3/4] libxl: add
version_info function"):
> instead of this chain of if else, why don''t you just init the
structure
> to NULL at the beginning of the call, and not do any else ?
Yes.

Furthermore the documentation comment should make it clear that the
version info structure can be undefined beforehand (and the reader can
therefore conclude that libxl_get_version_info won''t free it).
> last thing, is instead of using strdup, you should use 
> libxl_sprintf(ctx, "%s", string) so that you don''t have
to worry about
> freeing memory at all, and you can just omit the free_version_info call 
> completely.
I don''t think that''s correct because memory allocated by
libxl_sprintf
does not survive return from libxl into the caller.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Andre Przywara writes ("[Xen-devel] [PATCH 4/4] xl: add "xl info"
command"):
> The info subcommand was missing from the xl tool. Use the new libxl
> wrapper functions to create a clone of "xm info". The splitting
into
> several smaller functions is enspired by the implementation in XendNode.py.
> 
> Signed-off-by: Andre Przywara <andre.przywara@amd.com>
This patch is fine in itself, if you make the edits needed to deal
with the changes we have suggested in your libxl patch.

Thanks for your contribution.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On 19/04/10 16:36, Ian Jackson wrote:
> I don''t think that''s correct because memory allocated by
libxl_sprintf
> does not survive return from libxl into the caller.
It survives as long as the context is not ctx_free. i.e. if the data 
need to preserve longer, they will have do be duplicated before freeing 
the context.

This is to get away with having to deal with memory leak as much as 
possible and potential more complex code related to errors handling.

-- 
Vincent

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Vincent Hanquez writes ("Re: [Xen-devel] [PATCH 3/4] libxl: add
version_info function [and 1 more messages]"):
> On 19/04/10 16:36, Ian Jackson wrote:
> > I don''t think that''s correct because memory
allocated by libxl_sprintf
> > does not survive return from libxl into the caller.
> 
> It survives as long as the context is not ctx_free. i.e. if the data 
> need to preserve longer, they will have do be duplicated before freeing 
> the context.
This is sadly not a really tenable behaviour in the long term because
(a) a long-running caller does not want to free the context (and
necessarily reestablish xenstore connections etc. etc.) (b) the caller
may anyway not be able to free the context if doing so frees data they
are still using.

There are not (if I''m not mistaken) currently any uses of the libxl
allocators for data which persists beyond the current libxl entrypoint
and we shouldn''t introduce any now.  If there are any they should be
removed.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On 19/04/10 17:21, Ian Jackson wrote:
> Vincent Hanquez writes ("Re: [Xen-devel] [PATCH 3/4] libxl: add
version_info function [and 1 more messages]"):
>> On 19/04/10 16:36, Ian Jackson wrote:
>>> I don''t think that''s correct because memory
allocated by libxl_sprintf
>>> does not survive return from libxl into the caller.
>>
>> It survives as long as the context is not ctx_free. i.e. if the data
>> need to preserve longer, they will have do be duplicated before freeing
>> the context.
>
> This is sadly not a really tenable behaviour in the long term because
> (a) a long-running caller does not want to free the context (and
> necessarily reestablish xenstore connections etc. etc.) (b) the caller
> may anyway not be able to free the context if doing so frees data they
> are still using.
This is not perfect, and has a (tiny) cost, but it makes writing the 
current ocaml bindings (and the future python bindings) much easier and 
much less memory leak prone.

Limiting the number of possible boxes of error type, will definitely 
help getting XCP ported on libxl.

-- 
Vincent

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Ian Jackson wrote:
> Andre Przywara writes ("[Xen-devel] [PATCH 3/4] libxl: add
version_info function"):
>> Xen provides a xen_version hypercall to query the values of several
>> interesting things (like hypervisor version, commandline used,
>> actual changeset, etc.). Create a user-friendly and efficient
>> wrapper around the libxc function to provide values for xl info output.
> 
> As Vincent says, I think the query mask is overkill.  I would suggeset
> just doing without it, and always acquire and return all the
> information.  Ie, do away with query_mask and LIBXL_VERSION_FOO_MASK
> and the correspondings ifs.
I am not fully convinced of this. There is quite a lot of information 
copied (up to 4KB), and some fields (like pagesize or the version 
numbers) are just plain integers. And this approach just propagates the 
underlying design.
In the xl info implementation I used this feature to just get the 
pagesize. One could think of just querying the version number, too. If 
you don''t like the additional parameter, what about a wrapping macro,
then?
I don''t see the problem with the complexity, though, as it is hidden 
inside the library.
I can use libxl_sprintf function to get rid of the free() function, but 
I personally don''t like the idea of accumulated mallocs much, left
alone
the "hidden" free operation.

I will incorporate your other comments.

Thanks for the review!

Regards,
Andre.

-- 
Andre Przywara
AMD-Operating System Research Center (OSRC), Dresden, Germany
Tel: +49 351 488-3567-12


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On 19/04/10 21:43, Andre Przywara wrote:
> I am not fully convinced of this. There is quite a lot of information
> copied (up to 4KB), and some fields (like pagesize or the version
> numbers) are just plain integers. And this approach just propagates the
> underlying design.
> In the xl info implementation I used this feature to just get the
> pagesize. One could think of just querying the version number, too. If
> you don''t like the additional parameter, what about a wrapping
macro, then?
> I don''t see the problem with the complexity, though, as it is
hidden
> inside the library.
The query mask is exposed to the user, and the structure is partially 
filled. I can imagine that it''s going to create those kind of issues, 
where the wrong mask is passed, or change later, and then the field the 
user was expecting would be left null.

The macro would help in the previous case, but I still feel it''s 
unnecessary. As a user, I would just not bother, and call the function
always with full mask, until I reach the optimisation phase, and its 
appears on the graph.

A couple of K of data copied shouldn''t make a difference, and would be 
completely lost in noise, however if you really think it makes a 
difference, you could have special dedicated info call to get specific 
things like version number.
> I can use libxl_sprintf function to get rid of the free() function, but
> I personally don''t like the idea of accumulated mallocs much, left
alone
> the "hidden" free operation.
There is a real advantage of permitting a fast development of libxl (and 
its bindings).

This was to get started at the beggining; when we reach a point where 
everything is working we could improve or remove the memory system. But 
I''m fairly convinced it''s not going to make a difference for
this kind
of library.

-- 
Vincent

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Vincent Hanquez wrote:
> On 19/04/10 21:43, Andre Przywara wrote:
>> I am not fully convinced of this. There is quite a lot of information
>> copied (up to 4KB), and some fields (like pagesize or the version
>> numbers) are just plain integers. And this approach just propagates the
>> underlying design.
>> In the xl info implementation I used this feature to just get the
>> pagesize. One could think of just querying the version number, too. If
>> you don''t like the additional parameter, what about a wrapping
macro,
>> then?
>> I don''t see the problem with the complexity, though, as it is
hidden
>> inside the library.
> 
> The query mask is exposed to the user, and the structure is partially 
> filled. I can imagine that it''s going to create those kind of
issues,
> where the wrong mask is passed, or change later, and then the field the 
> user was expecting would be left null.
> 
> The macro would help in the previous case, but I still feel it''s 
> unnecessary. As a user, I would just not bother, and call the function
> always with full mask, until I reach the optimisation phase, and its 
> appears on the graph.
> 
> A couple of K of data copied shouldn''t make a difference, and
would be
> completely lost in noise, however if you really think it makes a 
> difference, you could have special dedicated info call to get specific 
> things like version number.
I just wanted to avoid introducing a lot of functions which could be 
actually one. And  if a user passes a wrong mask, then well, he is just 
doing the wrong thing and cannot expect it to work. If Xen provides a 
possibility to query single parts of the information, why should we hide 
this capability from the upper layers?
Anyway, my plan was now to make the current mask function static and 
create three exported functions, one for getting all the information, 
one for getting the pagesize and one for getting the Xen version number 
only. Together with a comment one could later expose the mask function 
again if the need arises.
But when I was looking at the libxl_sprintf thing, I realized that all 
the information in the info structure is static and will never change 
during runtime. Am I right? If so, we can solve this whole thing by not 
only storing the strings in libxl_ctx, but the whole structure. The 
first call to the function would query all the members, subsequent calls 
would just return the pointer. This avoids duplication should the 
function be called multiple times. It would be automatically freed when 
destroying the xl context.

What do you think of this?

Regards,
Andre.


-- 
Andre Przywara
AMD-Operating System Research Center (OSRC), Dresden, Germany
Tel: +49 351 448-3567-12


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On 21/04/10 13:10, Andre Przywara wrote:
> I just wanted to avoid introducing a lot of functions which could be
> actually one. And  if a user passes a wrong mask, then well, he is just
> doing the wrong thing and cannot expect it to work. If Xen provides a
> possibility to query single parts of the information, why should we hide
> this capability from the upper layers?
well that''s just for simplicity.
> But when I was looking at the libxl_sprintf thing, I realized that all
> the information in the info structure is static and will never change
> during runtime. Am I right? If so, we can solve this whole thing by not
> only storing the strings in libxl_ctx, but the whole structure. The
> first call to the function would query all the members, subsequent calls
> would just return the pointer. This avoids duplication should the
> function be called multiple times. It would be automatically freed when
> destroying the xl context.
>
> What do you think of this?
I think that''s quite reasonable and a good idea all in all. the API 
stays simple, and most potential performance issues are just dealt with.

-- 
Vincent

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel