[virt-tools-list] 答复： Re: [virt-manager PATCH 1/2] virt-install: add param 'cachemode' for option '--cpu'
Lin Ma
lma at suse.com
Fri Sep 1 15:35:33 UTC 2017
>>> Cole Robinson <crobinso at redhat.com> 2017/8/26 星期六 上午 8:40 >>>
>Firstly I suggest for this instance where the test case is small to just
>squash patch #2 into this one.
ok, test case patch will be merged into this patch.
>On 08/21/2017 06:17 AM, Lin Ma wrote:
>> libvirt supports guest CPU cache by commit df13c0b, So add this feature
>> to virt-install to configure cpu L3 cache mode.
>> GUI support will be added later.
>>
>
>This 'GUI support' comment doesn't really need to go into the commit message.
>Better in the cover letter or after the ---
>
>That said, I need to know more about the feature before I can give my opinion
>on whether it goes in the GUI. We keep the CPU config pretty lax at the
>moment. So maybe you can explain a bit what are the benefits/tradeoffs of the
>different cache configuration values? A pointer to docs is fine too
qemu default behavior about cache mode is: l3-cache=on, host-cache-info=off.
So GUI for 'emulate' and 'disable' are of small significance, But if we provide a
radio to enable cache passthrough, It a easy way to pass 'host-cache-info=on' to
qemu while users use cpu host-passthrough and need host cache passthrough.
>> Currently, The valid value are 'passthrough', 'emulate' or 'disable'.
>> say:
>> --cpu host-passthrough,cachemode=passthrough
>> --cpu $CPU,cachemode=emulate
>> --cpu $CPU,cachemode=disable
>>
>
>Since it seems like the libvirt code could be adapted to handle multiple
><cache> lines at some point, better to make the option name 'cache.mode'. And
>also add 'cache.level' while you are here too
>
ok.
I'll use the option name 'cache.mode' instead of 'cachemode'.
>> Signed-off-by: Lin Ma <lma at suse.com>
>> ---
>> man/virt-install.pod | 4 ++++
>> virtinst/cli.py | 12 ++++++++++--
>> virtinst/cpu.py | 29 +++++++++++++++++++++++++++++
>> 3 files changed, 43 insertions(+), 2 deletions(-)
>>
>> diff --git a/man/virt-install.pod b/man/virt-install.pod
>> index 3482e53..49ee9b8 100644
>> --- a/man/virt-install.pod
>> +++ b/man/virt-install.pod
>> @@ -247,6 +247,10 @@ Example of specifying two NUMA cells. This will generate XML like:
>> </numa>
>> </cpu>
>>
>> +=item B<--cpu host-passthrough,cachemode=passthrough>
>> +
>> +Example of passing through the host cpu's cache information.
>> +
>> =back
>>
>> Use --cpu=? to see a list of all available sub options. Complete details at L<http://libvirt.org/formatdomain.html#elementsCPU>
>> diff --git a/virtinst/cli.py b/virtinst/cli.py
>> index ece9b86..786395f 100644
>> --- a/virtinst/cli.py
>> +++ b/virtinst/cli.py
>> @@ -617,8 +617,9 @@ def vcpu_cli_options(grp, backcompat=True, editexample=False):
>> if editexample:
>> extramsg = "--cpu host-model,clearxml=yes"
>> grp.add_argument("--cpu",
>> - help=_("CPU model and features. Ex:\n"
>> - "--cpu coreduo,+x2apic\n") + extramsg)
>> + help=_("CPU model, L3 cache mode and features. Ex:\n"
>> + "--cpu coreduo,+x2apic\n"
>> + "--cpu host-passthrough,cachemode=passthrough\n") + extramsg)
>>
>
>I don't think it's worth it to add an example of cachemode= to the --help
>output. I try to keep that output slim and generally only targeting common
>usecases or demonstrating weird option syntax
>
>Though I think it's a good idea to add a --cpu host-passthrough example to the
>--help output but that can be its own patch
ok, I'll drop cachemode from help output, and write a patch for adding '--cpu
host-passthrough' example :-)
>> if backcompat:
>> grp.add_argument("--check-cpu", action="store_true",
>> @@ -1467,6 +1468,10 @@ class ParserCPU(VirtCLIParser):
>> else:
>> inst.add_feature(feature_name, policy)
>>
>> + def set_l3_cache_cb(self, inst, val, virtarg):
>> + cpu = inst
>> + cpu.set_l3_cache_mode(cpu, val)
>> +
>> def _parse(self, inst):
>> # Convert +feature, -feature into expected format
>> for key, value in self.optdict.items():
>> @@ -1508,6 +1513,9 @@ ParserCPU.add_arg("cpus", "cell[0-9]*.cpus", can_comma=True,
>> ParserCPU.add_arg("memory", "cell[0-9]*.memory",
>> find_inst_cb=ParserCPU.cell_find_inst_cb)
>>
>> +# Options for CPU.cache
>> +ParserCPU.add_arg(None, "cachemode", cb=ParserCPU.set_l3_cache_cb)
>> +
>>
>> ###################
>> # --vcpus parsing #
>> diff --git a/virtinst/cpu.py b/virtinst/cpu.py
>> index 468853f..ec46452 100644
>> --- a/virtinst/cpu.py
>> +++ b/virtinst/cpu.py
>> @@ -32,6 +32,20 @@ class _CPUCell(XMLBuilder):
>> memory = XMLProperty("./@memory", is_int=True)
>>
>>
>> +class CPUCache(XMLBuilder):
>> + """
>> + Class for generating <cpu> child <cache> XML
>> + """
>> +
>> + MODES = ["passthrough", "emulate", "disable"]
>> +
>> + _XML_ROOT_NAME = "cache"
>> + _XML_PROP_ORDER = ["level", "mode"]
>> +
>> + level = XMLProperty("./@level")
>> + mode = XMLProperty("./@mode")
>> +
>> +
>> class CPUFeature(XMLBuilder):
>> """
>> Class for generating <cpu> child <feature> XML
>> @@ -107,6 +121,21 @@ class CPU(XMLBuilder):
>> self.add_child(obj)
>> return obj
>>
>> + cache = XMLChildProperty(CPUCache)
>> + def set_l3_cache_mode(self, cpu, cache_mode):
>> + cache = CPUCache(self.conn)
>> + if cache_mode not in ["emulate", "passthrough", "disable"]:
>> + raise RuntimeError("valid cache mode: 'passthrough' or "
>> + "'emulate' or 'disable'")
>
>Libvirt will already validate that for us, let's not duplicate it here.
>
ok, I'll drop it.
>> + if cache_mode == "emulate":
>> + cache.level = "3"
>
>Is cache_mode == "emulate" always going to map to level=3? Or is that the only
>valid combination at the moment? If it is potentially going to grow to support
>different level= values in the future, I'd rather require the user to specify
>cache.level=3 explicitly on the command line than trying to get magic about
>it, especially if it's going to be an uncommon operation
Yes, it's the only valid combination so far.
Perhaps qemu will add L4 cache support while some kind of physical processors
add L4 cache in the future, So yes, it maybe grow to support different level values
in the future. Actually in my opinion, We don't need to specify it explicitly, But I'll
add 'cache.level' on command line in next patch.
>> + elif (cache_mode == "passthrough" and
>> + cpu.mode != self.SPECIAL_MODE_HOST_PASSTHROUGH):
>> + raise RuntimeError("cache mode 'passthrough' requires "
>> + "CPU model 'host-passthrough'")
>
>If libvirt validates this for us, let's not duplicate it. If libvirt doesn't
>validate this for us and it's indeed and invalid combination, we should fix it
>there
libvirt validate it already, I'll drop these code.
>
>Thanks,
>Cole
Thanks,
Lin
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/virt-tools-list/attachments/20170901/be3437f4/attachment.htm>
More information about the virt-tools-list
mailing list