Re: [Qemu-devel] [virtio-dev] Re: [PATCH RFC] virtio-net: enable configurable tx queue size

[Wei Wang]

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 
> > > > > > > > queue
> > > > > > > > size specified by the user should be power of 2.
> > > > > > > >
> > > > > > > > Setting the tx queue size to be 1024 requires the guest driver to
> > > > > > > > support the VIRTIO_NET_F_MAX_CHAIN_SIZE feature.
> > > > > > > This should be a generic ring feature, not one specific to 
> > > > > > > virtio net.
> > > > > > 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 being
> > set 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 sets
vring.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 machine
with 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.


> - ABI is changed, e.g -M pc-q35-2.10 returns 1024 on 2.11
Didn't get this. Could you please explain more? which ABI would be 
changed, and why it affects q35?


> > > > For live migration, the queue size that is being used will also be 
> > > > transferred
> > > > to 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 vhost
> > > > > > backed 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 driver
will get the new value.

Best,
Wei