Re: [Xen-devel] [PATCH 2/6] xl: Implement XENMEM_claim_pages support via 'claim_mode' global config

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Konrad Rzeszutek Wilk writes ("[PATCH 2/6] xl: Implement XENMEM_claim_pages 
support via 'claim_mode' global config"):
> The XENMEM_claim_pages hypercall operates per domain and it should be
> used system wide. As such this patch introduces a global configuration
> option 'claim_mode' that by default is disabled.

This mostly looks good to me.

> +=item B<claim_mode=BOOLEAN>
> +
> +If this option is enabled then when a guest is created there will be an
> +guarantee that there is memory available for the guest. This is an 
> particularly

Sorry to be picky, but can I ask you to wrap this document to 70-75
characters ?  At this width (looks like exactly 80) it inevitably
generates wrap damage when a patch or quoted code is shown in an
80-column window.

> +Note that to enable tmem type guest, one need to provide C<tmem> on the
> +Xen hypervisor argument and as well on the Linux kernel command line.

"to enable tmem type guest" - shouldn't that be "guests" ?  And "one
needs" ?

> +    if (!xlu_cfg_get_long (config, "claim_mode", &l, 0))
> +        global_claim_mode = 1;

This should set global_claim_mode to something depending on l, not 1,
I think ?

And perhaps global_claim_mode should be a libxl_defbool, so that we
can inherit the libxl default more directly ?  At the moment the
default is set in xl and again in libxl, which I think is not ideal.
It's better to try to set the default only in one place.

Also I think you need to call libxl_defbool_setdefault somewhere in
libxl, probably in libxl__domain_create_info_setdefault.

Is there some reason why this variable is called "global_claim_mode"
and not just "claim_mode" ?  Other globals in xl aren't marked in this
way.

Ian.

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