Re: [PATCH 4/5] virtio-net: Update virtio-net curr_queue_pairs in vdpa backends

[Si-Wei Liu]

On 8/24/2022 8:05 PM, Jason Wang wrote:
> On Thu, Aug 25, 2022 at 10:53 AM Jason Wang <jasowang@redhat.com> wrote:
> > On Thu, Aug 25, 2022 at 8:38 AM Si-Wei Liu <si-wei.liu@oracle.com> wrote:
> > > On 8/23/2022 9:27 PM, Jason Wang wrote:
> > > > 在 2022/8/20 01:13, Eugenio Pérez 写道:
> > > > > It was returned as error before. Instead of it, simply update the
> > > > > corresponding field so qemu can send it in the migration data.
> > > > >
> > > > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > > > > ---
> > > >
> > > > Looks correct.
> > > >
> > > > Adding Si Wei for double check.
> > > Hmmm, I understand why this change is needed for live migration, but
> > > this would easily cause userspace out of sync with the kernel for other
> > > use cases, such as link down or userspace fallback due to vdpa ioctl
> > > error. Yes, these are edge cases.
> > Considering 7.2 will start, maybe it's time to fix the root cause
> > instead of having a workaround like this?
> Btw, the patch actually tries its best to limit the behaviour, e.g it
> doesn't do the following set_status() stuff. So I think it won't
> trigger the issue you mentioned here?
Well, we can claim we don't support the link down+up case while changing 
queue numbers in between. On the other hand, the error recovery from 
fallback userspace is another story, which would need more attention and 
care on the error path. Yes, if see it from that perspective the change 
is fine. For completeness, please refer to the patch in the other email.

-Siwei

> Thanks
>
> > THanks
> >
> > > Not completely against it, but I
> > > wonder if there's a way we can limit the change scope to live migration
> > > case only?
> > >
> > > -Siwei
> > >
> > > > Thanks
> > > >
> > > >
> > > > >    hw/net/virtio-net.c | 17 ++++++-----------
> > > > >    1 file changed, 6 insertions(+), 11 deletions(-)
> > > > >
> > > > > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> > > > > index dd0d056fde..63a8332cd0 100644
> > > > > --- a/hw/net/virtio-net.c
> > > > > +++ b/hw/net/virtio-net.c
> > > > > @@ -1412,19 +1412,14 @@ static int virtio_net_handle_mq(VirtIONet *n,
> > > > > uint8_t cmd,
> > > > >            return VIRTIO_NET_ERR;
> > > > >        }
> > > > >    -    /* Avoid changing the number of queue_pairs for vdpa device in
> > > > > -     * userspace handler. A future fix is needed to handle the mq
> > > > > -     * change in userspace handler with vhost-vdpa. Let's disable
> > > > > -     * the mq handling from userspace for now and only allow get
> > > > > -     * done through the kernel. Ripples may be seen when falling
> > > > > -     * back to userspace, but without doing it qemu process would
> > > > > -     * crash on a recursive entry to virtio_net_set_status().
> > > > > -     */
> > > > > +    n->curr_queue_pairs = queue_pairs;
> > > > >        if (nc->peer && nc->peer->info->type ==
> > > > > NET_CLIENT_DRIVER_VHOST_VDPA) {
> > > > > -        return VIRTIO_NET_ERR;
> > > > > +        /*
> > > > > +         * Avoid updating the backend for a vdpa device: We're only
> > > > > interested
> > > > > +         * in updating the device model queues.
> > > > > +         */
> > > > > +        return VIRTIO_NET_OK;
> > > > >        }
> > > > > -
> > > > > -    n->curr_queue_pairs = queue_pairs;
> > > > >        /* stop the backend before changing the number of queue_pairs
> > > > > to avoid handling a
> > > > >         * disabled queue */
> > > > >        virtio_net_set_status(vdev, vdev->status);