Subject: Re: [virtio-dev] Re: [Qemu-devel] [virtio-dev] Re: [PATCH RFC] virtio-net: enable configurable tx queue size
On 2017年05月25日 19:50, Wei Wang wrote:
On 05/25/2017 03:49 PM, Jason Wang wrote:On 2017年05月24日 16:18, Wei Wang wrote:On 05/24/2017 11:19 AM, Jason Wang wrote:On 2017年05月23日 18:36, Wei Wang wrote:On 05/23/2017 02:24 PM, Jason Wang wrote:On 2017年05月23日 13:15, Wei Wang wrote:On 05/23/2017 10:04 AM, Jason Wang wrote:On 2017年05月22日 19:52, Wei Wang wrote:On 05/20/2017 04:42 AM, Michael S. Tsirkin wrote:On Fri, May 19, 2017 at 10:32:19AM +0800, Wei Wang wrote:This patch enables the virtio-net tx queue size to be configurable between 256 (the default queue size) and 1024 by the user. The queueThis should be a generic ring feature, not one specific to virtio net.size specified by the user should be power of 2.Setting the tx queue size to be 1024 requires the guest driver tosupport the VIRTIO_NET_F_MAX_CHAIN_SIZE feature.OK. How about making two more changes below: 1) make the default tx queue size = 1024 (instead of 256).As has been pointed out, you need compat the default value too in this case.The driver gets the size info from the device, then would it cause any compatibility issue if we change the default ring size to 1024 in the vhost case? In other words, is there any software (i.e. any virtio-net driver)functions based on the assumption of 256 queue size?I don't know. But is it safe e.g we migrate from 1024 to an older qemu with 256 as its queue size?Yes, I think it is safe, because the default queue size is used when the device is beingset up (e.g. feature negotiation).During migration (the device has already been running), the destination machine will load the device state based on the the queue size that is being used (i.e. vring.num).The default value is not used any more after the setup phase.I haven't checked all cases, but there's two obvious things: - After migration and after a reset, it will go back to 256 on dst.Please let me clarify what we want first: when QEMU boots and it realizes the virtio-net device, if the tx_queue_size is not given by the command line, we want to use 1024 as the queue size, that is, virtio_add_queue(,1024,), which setsvring.num=1024 and vring.num_default=1024.When migration happens, the vring.num variable (has been 1024) is sent to the destination machine, where virtio_load() will assign the destination side vring.num to that value (1024). So, vring.num=1024 continues to work on the destination machinewith old QEMU. I don't see an issue here.If reset happens, I think the device and driver will re-do the initialization steps. So, if they are with the old QEMU, then they use the old qemu realize() function to do virtio_add_queue(,256,), and the driver will re-do the probe() steps and take vring.num=256, then everything works fine.Probably works fine but the size is 256 forever after migration. Instead of using 1024 which work just one time and maybe risky, isn't it better to just use 256 for old machine types?If it migrates to the old QEMU, then I think everything should work in the old QEMU style after reset (not just our virtio-net case). I think this should be something natural and reasonable.
The point is it should behave exactly the same not only after reset but also before.
Why would the change depends on machine types?Didn't get this. Could you please explain more? which ABI would be changed, and why it affects q35?- ABI is changed, e.g -M pc-q35-2.10 returns 1024 on 2.11Nothing specific to q35, just to point out the machine type of 2.10.E.g on 2.10, -M pc-q35-2.10, vring.num is 256; On 2.11 -M pc-q35-2.10 vring.num is 1024.I think it's not related to the machine type. Probably we can use QEMU version to discuss here.Suppose this change will be made to the next version, QEMU 2.10. Then with QEMU 2.10, whenpeople create a virtio-net device as usual: -device virtio-net-pci,netdev=net1,mac=52:54:00:00:00:01 it will creates a device with queue size = 1024. If they use QEMU 2.9, then the queue size = 256. What ABI change did you mean?
Not the case. Old drivers which don't have VIRTIO_NET_F_MAX_CHAIN_SIZE feature support, will not allow the device to use 1024 queue size. So, in that case, the device will use the old, 256, queue size.For live migration, the queue size that is being used will also be transferredto the destination.We can reduce the size (to 256) if the MAX_CHAIN_SIZE feature is not supported by the guest. In this way, people who apply the QEMU patch can directly use the largest queue size(1024) without adding the booting command line.2) The vhost backend does not use writev, so I think when the vhostbacked is used, using 1024 queue size should not depend on the MAX_CHAIN_SIZE feature.But do we need to consider even larger queue size now?Need Michael's feedback on this. Meanwhile, I'll get the next version of code ready and check if larger queue size would cause any corner case.The problem is, do we really need a new config filed for this? Or just introduce a flag which means "I support up to 1024 sgs" is sufficient?For now, it also works without the new config field, max_chain_size, But I would prefer to keep the new config field, because: Without that, the driver will work on an assumed value, 1023.This is the fact, and it's too late to change legacy driver.If the future, QEMU needs to change it to 1022, then how can the new QEMU tell the old driver, which supports the MAX_CHAIN_SIZE feature but works with the old hardcode value 1023?Can config filed help in this case? The problem is similar to ANY_HEADER_SG, the only thing we can is to clarify the limitation for new drivers.I think it helps, because the driver will do virtio_cread_feature(vdev, VIRTIO_NET_F_MAX_CHAIN_SIZE,struct virtio_net_config, max_chain_size, &chain_size); to get the max_chain_size from the device. So when new QEMU has a new value of max_chain_size, old driverwill get the new value.I think we're talking about old drivers, so this won't work. It can only work with assumptions (implicitly).Thanks
I think it's better not tie #sgs to queue size.
The point of using the config field here is, when tomorrow's device is released with a requirement for the driver to use max_chain_size=1022 (not today's 1023), today's driver will naturally support tomorrow's device without any modification, since it reads the max_chain_size from the config field which is filled by the device (either today'sdevice or tomorrow's device with different values).
I'm not saying anything wrong with the config filed you introduced. But you should answer the following question:
Is it useful to support more than 1024? If yes, why? If not, introduce a VIRTIO_F_SG_1024 is more than enough I think.
Best, Wei --------------------------------------------------------------------- To unsubscribe, e-mail: email@example.com For additional commands, e-mail: firstname.lastname@example.org