On 09/24/2014 01:24 AM, John Ferlan wrote:
On 09/18/2014 10:15 AM, Ján Tomko wrote:
Add options for tuning segment offloading: <driver> <host csum='off' gso='off' tso4='off' tso6='off' ecn='off' ufo='off'/> <guest csum='off' tso4='off' tso6='off' ecn='off' ufo='off'/> </driver> which control the respective host_ and guest_ properties of the virtio-net device. --- docs/formatdomain.html.in | 24 ++- docs/schemas/domaincommon.rng | 44 ++++- src/conf/domain_conf.c | 218 ++++++++++++++++++++- src/conf/domain_conf.h | 15 ++ .../qemuxml2argv-net-virtio-disable-offloads.xml | 35 ++++ tests/qemuxml2xmltest.c | 1 + 6 files changed, 329 insertions(+), 8 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-net-virtio-disable-offloads.xml
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 8c03ebb..5dd70a4 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -3868,7 +3868,11 @@ qemu-kvm -net nic,model=? /dev/null <source network='default'/> <target dev='vnet1'/> <model type='virtio'/> - <b><driver name='vhost' txmode='iothread' ioeventfd='on' event_idx='off' queues='5'/></b> + <b><driver name='vhost' txmode='iothread' ioeventfd='on' event_idx='off' queues='5'> + <host csum='off' gso='off' tso4='off' tso6='off' ecn='off' ufo='off'/> + <guest csum='off' tso4='off' tso6='off' ecn='off' ufo='off'/> + </driver> + </b> </interface> </devices> ...</pre> @@ -3972,6 +3976,24 @@ qemu-kvm -net nic,model=? /dev/null processor, resulting in much higher throughput. <span class="since">Since 1.0.6 (QEMU and KVM only)</span> </dd> + <dt><code>host offloading options</code></dt>
Should this be <code>host</host> offloading options? or host TCP options (in some way to indicate that these are TCP level options). Probably also want your disclaimer use of these should be for only those that know what they are doing...
Well ufo is an UDP option.
+ <dd> + The <code>csum</code>, <code>gso</code>, <code>tso4</code>, + <code>tso6</code>, <code>ecn</code>, <code>ufo</code>
Should read: ecn, and ufo
Should you "spell out" what each translates to?
csum (checksums), gso (generic segmentation offload), tso (tcp segmentation offload v4 and v6), ecn (congestion notification), and ufo (UDP fragment offloads)
I thought not spelling them out was the equivalent of the "only use this if you know what you're doing" disclaimer.
+ attributes with possible values <code>on</code> + and <code>off</code> can be used to turn host offloads off.
s/turn host offloads off/turn off host TCP options/
Saying "offloads off" aloud just doesn't seem right.
+ By default, the supported offloads are enabled by QEMU.
s/the supported offloads/the TCP options/
+ <span class="since">Since 1.2.9 (QEMU only)</span> + </dd> + <dt>guest offloading options</dt>
Similar in here... Does the host setting override the guest value or can the host say "tso4=off" while the guest has "tso4=on"? Curious mainly.
It can say that, but I doubt it'll work.
+ <dd> + The <code>csum</code>, <code>tso4</code>, + <code>tso6</code>, <code>ecn</code>, <code>ufo</code>
same here with the "and" - although I suppose you could just reference the <host> bits "above"...
Another way to say it is guests can use the same options except gso
+ attributes with possible values <code>on</code> + and <code>off</code> can be used to turn guest offloads off.
s/turn guest offloads off/turn off guest TCP options/
How about 'turn off guest offload options'?
+ By default, the supported offloads are enabled by QEMU.
s/the supported offloads/the TCP options/
And of course the usage disclaimer!
+ <span class="since">Since 1.2.9 (QEMU only)</span> + </dd> </dl>
<h5><a name="elementsBackendOptions">Setting network backend-specific options</a></h5>
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 3ccec1c..cdaafa6 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -6897,6 +6897,7 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt, char *ioeventfd = NULL; char *event_idx = NULL; char *queues = NULL; + char *str = NULL; char *filter = NULL; char *internal = NULL; char *devaddr = NULL; @@ -7385,6 +7386,115 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt, } def->driver.virtio.queues = q; }
How about something like this? Not that you have anything wrong, but it avoids the chance that some "change" in the future requires 11 similar changes...
I was worried it wouldn't be clear enough and since we use similar repetition all over domain_conf, it would be better handled separately. Jan