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 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list