Re: [Qemu-devel] [PATCH] virtio-net: do not start queues that are not enabled by the guest

[Michael S. Tsirkin]

On Thu, Feb 21, 2019 at 10:18:52AM +0200, Yuri Benditovich wrote:
> On Thu, Feb 21, 2019 at 8:49 AM Jason Wang <address@hidden> wrote:
> >
> >
> > On 2019/2/21 下午2:00, Yuri Benditovich wrote:
> > > On Tue, Feb 19, 2019 at 8:27 AM Jason Wang <address@hidden> wrote:
> > >>
> > >> On 2019/2/19 上午7:34, Michael S. Tsirkin wrote:
> > >>> On Mon, Feb 18, 2019 at 10:49:08PM +0200, Yuri Benditovich wrote:
> > >>>> On Mon, Feb 18, 2019 at 6:39 PM Michael S. Tsirkin <address@hidden> 
> > >>>> wrote:
> > >>>>> On Mon, Feb 18, 2019 at 11:58:51AM +0200, Yuri Benditovich wrote:
> > >>>>>> On Mon, Feb 18, 2019 at 5:49 AM Jason Wang <address@hidden> wrote:
> > >>>>>>> On 2019/2/13 下午10:51, Yuri Benditovich wrote:
> > >>>>>>>> https://bugzilla.redhat.com/show_bug.cgi?id=1608226
> > >>>>>>>> On startup/link-up in multiqueue configuration the virtio-net
> > >>>>>>>> tries to starts all the queues, including those that the guest
> > >>>>>>>> will not enable by VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET.
> > >>>>>>>> If the guest driver does not allocate queues that it will not
> > >>>>>>>> use (for example, Windows driver does not) and number of actually
> > >>>>>>>> used queues is less that maximal number supported by the device,
> > >>>>>>> Is this a requirement of e.g NDIS? If not, could we simply allocate 
> > >>>>>>> all
> > >>>>>>> queues in this case. This is usually what normal Linux driver did.
> > >>>>>>>
> > >>>>>>>
> > >>>>>>>> this causes vhost_net_start to fail and actually disables vhost
> > >>>>>>>> for all the queues, reducing the performance.
> > >>>>>>>> Current commit fixes this: initially only first queue is started,
> > >>>>>>>> upon VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET started all the queues
> > >>>>>>>> requested by the guest.
> > >>>>>>>>
> > >>>>>>>> Signed-off-by: Yuri Benditovich <address@hidden>
> > >>>>>>>> ---
> > >>>>>>>>     hw/net/virtio-net.c | 7 +++++--
> > >>>>>>>>     1 file changed, 5 insertions(+), 2 deletions(-)
> > >>>>>>>>
> > >>>>>>>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> > >>>>>>>> index 3f319ef723..d3b1ac6d3a 100644
> > >>>>>>>> --- a/hw/net/virtio-net.c
> > >>>>>>>> +++ b/hw/net/virtio-net.c
> > >>>>>>>> @@ -174,7 +174,7 @@ static void virtio_net_vhost_status(VirtIONet 
> > >>>>>>>> *n, uint8_t status)
> > >>>>>>>>     {
> > >>>>>>>>         VirtIODevice *vdev = VIRTIO_DEVICE(n);
> > >>>>>>>>         NetClientState *nc = qemu_get_queue(n->nic);
> > >>>>>>>> -    int queues = n->multiqueue ? n->max_queues : 1;
> > >>>>>>>> +    int queues = n->multiqueue ? n->curr_queues : 1;
> > >>>>>>>>
> > >>>>>>>>         if (!get_vhost_net(nc->peer)) {
> > >>>>>>>>             return;
> > >>>>>>>> @@ -1016,9 +1016,12 @@ static int virtio_net_handle_mq(VirtIONet 
> > >>>>>>>> *n, uint8_t cmd,
> > >>>>>>>>             return VIRTIO_NET_ERR;
> > >>>>>>>>         }
> > >>>>>>>>
> > >>>>>>>> -    n->curr_queues = queues;
> > >>>>>>>>         /* stop the backend before changing the number of queues 
> > >>>>>>>> to avoid handling a
> > >>>>>>>>          * disabled queue */
> > >>>>>>>> +    virtio_net_set_status(vdev, 0);
> > >>>>>>> Any reason for doing this?
> > >>>>>> I think there are 2 reasons:
> > >>>>>> 1. The spec does not require guest SW to allocate unused queues.
> > >>>>>> 2. We spend guest's physical memory to just make vhost happy when it
> > >>>>>> touches queues that it should not use.
> > >>>>>>
> > >>>>>> Thanks,
> > >>>>>> Yuri Benditovich
> > >>>>> The spec also says:
> > >>>>>           queue_enable The driver uses this to selectively prevent 
> > >>>>> the device from executing requests from this
> > >>>>>           virtqueue. 1 - enabled; 0 - disabled.
> > >>>>>
> > >>>>> While this is not a conformance clause this strongly implies that
> > >>>>> queues which are not enabled are never accessed by device.
> > >>>>>
> > >>>>> Yuri I am guessing you are not enabling these unused queues right?
> > >>>> Of course, we (Windows driver) do not.
> > >>>> The code of virtio-net passes max_queues to vhost and this causes
> > >>>> vhost to try accessing all the queues, fail on unused ones and finally
> > >>>> leave vhost disabled at all.
> > >>> Jason, at least for 1.0 accessing disabled queues looks like a spec
> > >>> violation. What do you think?
> > >>
> > >> Yes, but there's some issues:
> > >>
> > >> - How to detect a disabled queue for 0.9x device? Looks like there's no
> > >> way according to the spec, so device must assume all queues was enabled.
> > > Can you please add several words - what is 0.9 device (probably this
> > > is more about driver) and
> > > what is the problem with it?
> >
> >
> > It's not a net specific issue. 0.9x device is legacy device defined in
> > the spec. We don't have a method to disable and enable a specific queue
> > at that time. Michael said we can assume queue address 0 as disabled,
> > but there's still a question of how to enable it. Spec is unclear and it
> > was too late to add thing for legacy device. For 1.0 device we have
> > queue_enable, but its implementation is incomplete, since it can work
> > with vhost correctly, we probably need to add thing to make it work.
> >
> >
> > >
> > >> - For 1.0, if we depends on queue_enable, we should implement the
> > >> callback for vhost I think. Otherwise it's still buggy.
> > >>
> > >> So it looks tricky to enable and disable queues through set status
> > > If I succeed to modify the patch such a way that it will act only in
> > > 'target' case,
> > > i.e. only if some of queueus are not initialized (at time of
> > > driver_ok), will it be more safe?
> >
> >
> > For 1.0 device, we can fix the queue_enable, but for 0.9x device how do
> > you enable one specific queue in this case? (setting status?)
> >
> 
> Do I understand correctly that for 0.9 device in some cases the device will
> receive feature _MQ set, but will not receive VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET?
> Or the problem is different?

No. For 0.9 it is possible that device adds buffers before DRIVER_OK.


> > A fundamental question is what prevents you from just initialization all
> > queues during driver start? It looks to me this save lots of efforts
> > than allocating queue dynamically.
> >
> 
> This is not so trivial in Windows driver, as it does not have objects for 
> queues
> that it does not use. Linux driver first of all allocates all the
> queues and then
> adds Rx/Tx to those it will use. Windows driver first decides how many queues
> it will use then allocates objects for them and initializes them from zero to
> fully functional state.
> 
> > Thanks
> >
> >
> > >
> > >> Thanks
> > >>
> > >>
> > >>>>>
> > >>>>>>> Thanks
> > >>>>>>>
> > >>>>>>>
> > >>>>>>>> +
> > >>>>>>>> +    n->curr_queues = queues;
> > >>>>>>>> +
> > >>>>>>>>         virtio_net_set_status(vdev, vdev->status);
> > >>>>>>>>         virtio_net_set_queues(n);
> > >>>>>>>>