Re: [libvirt] [PATCH] libxl: support <hap> feature

On 02/04/2016 05:54 AM, Joao Martins wrote: > > On 02/04/2016 01:41 AM, Jim Fehlig wrote: >> On 02/03/2016 02:20 PM, Joao Martins wrote: >>> On 02/03/2016 04:12 AM, Jim Fehlig wrote: >>>> Even though the libxl driver advertises <hap> in capabilities, >>>> it is ignored when set in domXML. Enable hap in the >>>> libxl_domain_create_info struct when <hap> is specified in domXML. >>>> >>>> The xm/xl <-> domXML parser/formatter already supports hap but >>>> lacked a test with hap enabled. Add a hap test. >> [...] >>> FWIW looks good and also: >>> >>> Tested-by: Joao Martins <joao.m.martins(a)oracle.com&gt; >> Thanks for reviewing/testing the patch! But after playing with the patch a bit >> more, I'm considering dropping it :-/. >> >> Before the patch, <hap> was ignored by the libxl driver, which left the 'hap' >> field of libxl_domain_create_info struct unset (= LIBXL__DEFBOOL_DEFAULT). For >> HVM guests, libxl will enable hap when libxl_domain_create_info.hap == >> LIBXL__DEFBOOL_DEFAULT. And using hap is good, since using shadow page table is >> less efficient. The downside is there is no way to turn hap off. >> >> After the patch it is possible to turn hap on and off via presence or absence of >> <hap>. However, there is a *lot* of existing config without <hap> that currently >> reaps the benefits of hap nonetheless. After patch, that config would require >> adding <hap>, else shadow page table will be used. (Note that in xm and xl >> config, hap is enabled in the absence of 'hap='.) >> >> I'm starting to question whether this feature should even be exposed. I think >> the only use case is debugging. E.g. turn hap off if suspected hardware bug. >> Otherwise you would certainly want the hypervisor to use hap if it is able to do >> so. What do you think? >> > Hm, good point. There is another field for VIR_TRISTATE which represents when > it's absent (VIR_TRISTATE_SWITCH_ABSENT). So perhaps we would leave the default > (= LIBXL_DEFBOOL_DEFAULT) if it's absent, and only set hap if the user > explicitly switches on/off? But IIUC, absent == off. I.e., there is no way to turn hap off except the absence of <hap>.

One option might be to extend <hap> with an 'enabled=yes|no' attribute. Then the absence of <hap> means hypervisor default. <hap> without the attribute would be the same as <hap enabled='yes'/> for backwards compatibility. And of course <hap enabled='no'/> disables hap.

Note that capabilities currently advertises hap as <hap default='off' toggle='yes'/> If folks agree to this option, the default should be changed to 'on'. > I am not sure if the usecase is just debug, but > given there are certain performance differences compared to shadow paging > (depending on the HVM guest workload) it might be nice to allow the > administrator to choose it. Agreed. I think the above option accomplishes that.

Regards, Jim -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list