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