Re: [libvirt] [PATCH 1/3] conf: Add support for virtio-net.rx_queue_size

On 31.08.2016 14:48, John Ferlan wrote:

>> --- a/docs/formatdomain.html.in >> +++ b/docs/formatdomain.html.in >> @@ -4604,7 +4604,7 @@ 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>&lt;driver name='vhost' txmode='iothread' ioeventfd='on' event_idx='off' queues='5' rxqueuesize='128'&gt; >> &lt;host csum='off' gso='off' tso4='off' tso6='off' ecn='off' ufo='off' mrg_rxbuf='off'/&gt; >> &lt;guest csum='off' tso4='off' tso6='off' ecn='off' ufo='off'/&gt; >> &lt;/driver&gt; >> @@ -4720,6 +4720,11 @@ qemu-kvm -net nic,model=? /dev/null >> <span class="since">virtio-net since 1.0.6 (QEMU and KVM only)</span> >> <span class="since">vhost-user since 1.2.17 (QEMU and KVM only)</span> >> </dd> >> + <dt><code>rxqueuesize</code></dt> >> + <dd> >> + The optional <code>rxqueuesize</code> attribute controls >> + the size of virtio ring for each queue as described above. > > Need a <span class="since"> (I assume something similar to queues with > the QEMU and KVM only condition) > > Also why not "rx_queue_size" - there's already event_idx or mrg_rxbuf, > so adding the "_" at least mimics qemu's argument. That's what I wanted to prevent. Copying name from qemu. But I've struggled with the naming too (as can be seen - look how far I got :D). I don't have a strong opinion on either of those, so whatever we feel like I'll stick with that.

> > Furthermore, the bz has quite a bit of discussion regarding an > "appropriate value", so while I don't think it's something we want to > provide (that is suggested values), perhaps we could create a pointer or > at least a few hints. At the very least a this value needs to be a power > of 2 value... If not provided, the QEMU default of 256 is used. A > larger size gives you what advantage. In general some guidance on > setting or where to look could be helpful. Well, as you point out later in the review too, qemu might change these limitation anytime and we would advice untrue. But if I'm careful with wording we might be safe.

> >> + </dd> >> </dl> >> <p> >> Offloading options for the host and guest can be configured using

> e.g. extra space (although I do see the line is at 80 chars... could > change the internal name to rxqsz ;-)). > >> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", >> + _("rxqueuesize has to be a power of two")); > > ^^ hence why I think it should be documented. > > The bz also indicates there's a maximum of 1024. Should we check for > that? Although if the maximum increases we'd have to follow suit. > Naturally there isn't a way to get that maximum. If something larger > than 1024 is passed, then qemu will fail. Ugh, the hazards of adding > these 1-off things that have limits and rules, but we're not given all > the details necessary. Nope. Moreover, this feature is going to be used by expert users who know exactly what they are doing. So I'd say we're okay with just trying to start qemu and see if it fails or not. IOW too much work on our side not worth it. > >> + goto cleanup; >> + } >> } >> >> ret = 0; >> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-net-virtio-rxqueuesize.xml b/tests/qemuxml2argvdata/qemuxml2argv-net-virtio-rxqueuesize.xml >> new file mode 100644 >> index 0000000..e23d3e3 >> --- /dev/null >> +++ b/tests/qemuxml2argvdata/qemuxml2argv-net-virtio-rxqueuesize.xml >> @@ -0,0 +1,29 @@ >> +<domain type='qemu'> >> + <name>QEMUGuest1</name> >> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> >> + <memory unit='KiB'>219100</memory> >> + <currentMemory unit='KiB'>219100</currentMemory> >> + <vcpu placement='static'>1</vcpu> >> + <os> >> + <type arch='i686' machine='pc'>hvm</type> >> + <boot dev='hd'/> >> + </os> >> + <clock offset='utc'/> >> + <on_poweroff>destroy</on_poweroff> >> + <on_reboot>restart</on_reboot> >> + <on_crash>destroy</on_crash> >> + <devices> >> + <emulator>/usr/bin/qemu</emulator> >> + <disk type='block' device='disk'> >> + <source dev='/dev/HostVG/QEMUGuest1'/> >> + <target dev='hda' bus='ide'/> >> + </disk> >> + <controller type='usb' index='0'/> >> + <interface type='user'> >> + <mac address='00:11:22:33:44:55'/> >> + <model type='virtio'/> >> + <driver rxqueuesize='128'/> >> + </interface> >> + <memballoon model='virtio'/> >> + </devices> >> +</domain> >> > > Shouldn't qemuxml2xmltest.c be updated to add this new test? Good catch.

> > Should there be a "FAIL" test with an invalid value? Sure, I can introduce such test. > > I think by rule we have to wait for QEMU to merge this so we cannot push > until then. In the meantime, we should probably find out if it's felt we > should check a maximum of 1024 or not. I'd hate to see that value change > and then we have problems, but the way it is now - qemu would fail with > a 2048 value from libvirt. I don't think we need to. We should probably check whether it is a power of two (this looks like invariant requirement) = who would want a ring size of a non-power of two size? But the maximum/minimum size can change. Currently the limits are [256;1024]. Moreover, qemu fails with sensible message: error: internal error: process exited while connecting to monitor: 2016-08-31T14:23:16.583247Z qemu-system-x86_64: -device virtio-net-pci,mq=on,vectors=42,rx_queue_size=2048,netdev=hostnet1,id=net1,mac=52:54:00:6e:0e:72,bus=pci.2,addr=0x2: Invalid rx_queue_size (= 2048), must be a power of 2 between 256 and 1024. I'll post v2 which will is rebased onto current HEAD and address your findings.