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

Wed Aug 31 14:47:34 UTC 2016

On 08/31/2016 10:24 AM, Michal Privoznik wrote:
> 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
>>>        <source network='default'/>
>>>        <target dev='vnet1'/>
>>>        <model type='virtio'/>
>>> -      <b><driver name='vhost' txmode='iothread' ioeventfd='on' event_idx='off' queues='5'>
>>> +      <b><driver name='vhost' txmode='iothread' ioeventfd='on' event_idx='off' queues='5' rxqueuesize='128'>
>>>          <host csum='off' gso='off' tso4='off' tso6='off' ecn='off' ufo='off' mrg_rxbuf='off'/>
>>>          <guest csum='off' tso4='off' tso6='off' ecn='off' ufo='off'/>
>>>        </driver>
>>> @@ -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.
> 

I guess I find it easier to be able to search qemu and libvirt code for
the same strings "if possible". I think we've already nixed allowing "-"
(dash), so those don't match up. I don't have a strong preference either.

>>
>> 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.
> 

As you point out below - "This should be used by expert users" or as the
disk device "ioeventfd" and "event_idx" descriptions indicate in bold
letters" "In general you should leave this option alone, unless you are
very certain you know what you are doing."

>>
>>> +      </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.
> 

As I'm going through your NVDIMM series - it has the same problem - so I
hunted down what I did for luks-disks... See commit id '9bbf0d7e' - it
adds a file link for the tests/qemuxml2xmloutdata/ to the
../qemuxml2argvdata/... I had just copied other examples.

And I also modified qemuxml2xmltest

>>
>> 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.
> 

I'm fine with not adding the max comparison... As I think you could see
left brain and right brain weren't in agreement either!

John