9:31 a.m.
On 09/24/2014 09:44 AM, Ján Tomko wrote:
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.
Yeah - good point. I guess I associated the rest/most with TCP... Of
course you could use "TCP/UDP" instead of just TCP... I'm OK without
the TCP reference though - it was a "extra" suggestion.
>
>> + <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.
Yes - I do agree that by not spelling them out it may cause someone to
"pause" before adding them.
However, of course, we're engineers so we have this "desire" to try
anyway and/or know what these new knobs/things do.
I guess it's one of those things that I don't like - that is seeing an
acronym on a website or in documentation that's not spelled out.
>
>
>> + 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'?
That's fine - it was the "offloads off" that didn't read well.
>
>> + 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.
That's fine - like I said - I didn't see anything wrong with the code -
maybe something for the todo list to reduce the "need" for all the
repetition and silly errors some of us make...
John