[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCHv2 1/2] conf: add options for disabling segment offloading




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
>>>        &lt;source network='default'/&gt;
>>>        &lt;target dev='vnet1'/&gt;
>>>        &lt;model type='virtio'/&gt;
>>> -      <b>&lt;driver name='vhost' txmode='iothread' ioeventfd='on' event_idx='off' queues='5'/&gt;</b>
>>> +      <b>&lt;driver name='vhost' txmode='iothread' ioeventfd='on' event_idx='off' queues='5'&gt;
>>> +        &lt;host csum='off' gso='off' tso4='off' tso6='off' ecn='off' ufo='off'/&gt;
>>> +        &lt;guest csum='off' tso4='off' tso6='off' ecn='off' ufo='off'/&gt;
>>> +      &lt;/driver&gt;
>>> +      </b>
>>>      &lt;/interface&gt;
>>>    &lt;/devices&gt;
>>>    ...</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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]