Re: [PATCH v3 13/15] virtio-net: support queue reset

[Jason Wang]

On Tue, Sep 6, 2022 at 12:25 AM Kangjie Xu <kangjie.xu@linux.alibaba.com> wrote:
>
>
> 在 2022/9/5 16:30, Jason Wang 写道:
> >
> > 在 2022/8/25 16:08, Kangjie Xu 写道:
> >> From: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> >>
> >> virtio-net and vhost-kernel implement queue reset.
> >> Queued packets in the corresponding queue pair are flushed
> >> or purged.
> >>
> >> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> >> Signed-off-by: Kangjie Xu <kangjie.xu@linux.alibaba.com>
> >> ---
> >>   hw/net/virtio-net.c | 18 ++++++++++++++++++
> >>   1 file changed, 18 insertions(+)
> >>
> >> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> >> index 27b59c0ad6..d774a3e652 100644
> >> --- a/hw/net/virtio-net.c
> >> +++ b/hw/net/virtio-net.c
> >> @@ -540,6 +540,23 @@ 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;
> >> +    }
> >> +
> >> +    if (get_vhost_net(nc->peer) &&
> >> +        nc->peer->info->type == NET_CLIENT_DRIVER_TAP) {
> >> +        vhost_net_virtqueue_reset(vdev, nc, queue_index);
> >> +    }
> >> +
> >> +    flush_or_purge_queued_packets(nc);
> >
> >
> > But the codes doesn't prevent the usersapce datapath from being used?
> > (e.g vhost=off)
> >
> > E.g vhost_net_start_one() had:
> >
> >     if (net->nc->info->poll) {
> >         net->nc->info->poll(net->nc, false);
> >     }
> I review this part in vhost_net_start/stop_one().
>
> I didn't take the usersapce datapath into consideration.

Because it's used for vhost-net. So I think maybe we should first
implement the rest in the userspace datapath first then try to
implement them in the vhost?

> If we don't
> prevent it, the userspace datapath may access it and causes some
> problems. From this point, we should disable it.
>
> But if we add net->nc->info->poll in vq reset, it can only operate at
> queue-pair level, which obeys "per-virtqueue".
>
> What's your opinion on this point? I think it's ok to add it in vq reset.

See above, I'd suggest to implement the vq reset in qemu usersapce
datapath first. Then we can do vhost part on top.

>
> >
> > And I will wonder if it's better to consider to:
> >
> > 1) factor out the per virtqueue start/stop from
> > vhost_net_start/stop_one()
> >
> > 2) simply use the helper factored out via step 1)
> >
> > Thanks
> >
> I have a different idea about it, vhost_virtqueue_start/stop()(in
> hw/virtio/vhost.c) are already good abstractions of per virtqueue
> start/stop.
>
> These two functions are used in vhost_dev_start. It's just because
> vhost-net needs some configuration, so we add net->nc->info->poll and
> set_backend for it. But for other devices using vhost_dev_start,
> set_backend and net->nc->info->poll may be not necessary.
>
> I think your apporach to abstract per virtqueue start/stop from
> vhost_net_start/stop_one() will break the generality of
> vhost_dev_start(), which is a common interface for different devices.
>
> To conclude my opinions
>
> 1. I think we need to add net->nc->info->poll in our
> vhost_net_virtqueue_reset() and vhost_net_virtqueue_restart()
>
> 2. We still need vhost_net_virtqueue_reset() and
> vhost_net_virtqueue_restart().
>
>      a. vhost_net_virtqueue_reset() is a wrapper for vhost_virtqueue_stop().
>
>      b. vhost_net_virtqueue_restart() is a wrapper for
> vhost_virtqueue_start().

Sounds good, let's do and see.

Thanks

>
> Thanks
>
> >
> >> +}
> >> +
> >>   static void virtio_net_reset(VirtIODevice *vdev)
> >>   {
> >>       VirtIONet *n = VIRTIO_NET(vdev);
> >> @@ -3784,6 +3801,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;
>