8:44 a.m.
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