Re: [PATCH 07/16] virtio-net: support queue reset

From: Jason Wang <jasowang@redhat.com>
To: Kangjie Xu <kangjie.xu@linux.alibaba.com>
Cc: qemu-devel <qemu-devel@nongnu.org>, mst <mst@redhat.com>,
	hengqi@linux.alibaba.com, Xuan Zhuo <xuanzhuo@linux.alibaba.com>
Subject: Re: [PATCH 07/16] virtio-net: support queue reset
Date: Wed, 27 Jul 2022 14:59:00 +0800	[thread overview]
Message-ID: <CACGkMEvkFMtbKJmK7ZVTVjPNxvF7VM-f_uW4cjuuc2PLeRZ+YQ@mail.gmail.com> (raw)
In-Reply-To: <81462777-254b-762c-cbe3-2272ca343b60@linux.alibaba.com>

On Wed, Jul 27, 2022 at 2:25 PM Kangjie Xu <kangjie.xu@linux.alibaba.com> wrote:
>
>
> 在 2022/7/27 13:00, Jason Wang 写道:
> > On Tue, Jul 26, 2022 at 3:02 PM Kangjie Xu <kangjie.xu@linux.alibaba.com> wrote:
> >>
> >> 在 2022/7/26 11:43, Jason Wang 写道:
> >>> 在 2022/7/18 19:17, Kangjie Xu 写道:
> >>>> From: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> >>>>
> >>>> virtio-net implements queue reset. Queued packets in the corresponding
> >>>> queue pair are flushed or purged.
> >>>>
> >>>> Queue reset is currently only implemented for non-vhosts.
> >>>>
> >>>> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> >>>> ---
> >>>>    hw/net/virtio-net.c | 15 +++++++++++++++
> >>>>    1 file changed, 15 insertions(+)
> >>>>
> >>>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> >>>> index 7ad948ee7c..8396e21a67 100644
> >>>> --- a/hw/net/virtio-net.c
> >>>> +++ b/hw/net/virtio-net.c
> >>>> @@ -531,6 +531,19 @@ static RxFilterInfo
> >>>> *virtio_net_query_rxfilter(NetClientState *nc)
> >>>>        return info;
> >>>>    }
> >>>>    +static void virtio_net_queue_reset(VirtIODevice *vdev, uint32_t
> >>>> queue_index)
> >>>> +{
> >>>> +    VirtIONet *n = VIRTIO_NET(vdev);
> >>>> +    NetClientState *nc = qemu_get_subqueue(n->nic, vq2q(queue_index));
> >>>> +
> >>>> +    if (!nc->peer) {
> >>>> +        return;
> >>>> +    }
> >>>> +
> >>>> +    qemu_flush_or_purge_queued_packets(nc->peer, true);
> >>>> +    assert(!virtio_net_get_subqueue(nc)->async_tx.elem);
> >>>
> >>> Let's try to reuse this function in virtio_net_reset().
> >>>
> >> Yeah, I'll fix it.
> >>
> >> Thanks.
> >>
> >>>> +}
> >>>> +
> >>>>    static void virtio_net_reset(VirtIODevice *vdev)
> >>>>    {
> >>>>        VirtIONet *n = VIRTIO_NET(vdev);
> >>>> @@ -741,6 +754,7 @@ static uint64_t
> >>>> virtio_net_get_features(VirtIODevice *vdev, uint64_t features,
> >>>>        }
> >>>>          if (!get_vhost_net(nc->peer)) {
> >>>> +        virtio_add_feature(&features, VIRTIO_F_RING_RESET);
> >>>
> >>> This breaks migration compatibility.
> >>>
> >>> We probably need:
> >>>
> >>> 1) a new command line parameter
> >>> 2) make it disabled for pre-7.2 machine
> >>>
> >>> Thanks
> >>>
> >>>
> >> Sorry, I don't get what is the meaning of "pre-7.2 machine". Could you
> >> please explain it?
> > I meant for pre 7.2 machine type, we should make reset fault off by default.
> >
> > Otherwise we break migration compatibility.
> >
> > Thanks
>
> Sorry, I did not express myself clearly. Is "7.2" the version of a
> system or a module? If it is a system version, what is the system?

It's the machine type to make sure the migration can work. (you can
get the list of those types via qemu -machine ?)

E.g you can start a 7.0 machine on Qemu 7.1 and it guarantees to be
migrated to 7.0 machine on Qemu 7.0.

>
> I did not have backgrond knowledge related to this part and will
> investigate migration issues afterwards.

Git grep hw_compat_7_0 may give you more hints.

Thanks

>
> Thanks.
>
> >> Thanks
> >>
> >>>>            return features;
> >>>>        }
> >>>>    @@ -3766,6 +3780,7 @@ static void virtio_net_class_init(ObjectClass
> >>>> *klass, void *data)
> >>>>        vdc->set_features = virtio_net_set_features;
> >>>>        vdc->bad_features = virtio_net_bad_features;
> >>>>        vdc->reset = virtio_net_reset;
> >>>> +    vdc->queue_reset = virtio_net_queue_reset;
> >>>>        vdc->set_status = virtio_net_set_status;
> >>>>        vdc->guest_notifier_mask = virtio_net_guest_notifier_mask;
> >>>>        vdc->guest_notifier_pending = virtio_net_guest_notifier_pending;
>