Message-ID: <CACGkMEsnupEVKukgdom85gUwbGoLcHT8bM9OqR_U11DzGdz1rw@mail.gmail.com>
Date:   Wed, 9 Feb 2022 13:39:34 +0800
From:   Jason Wang <jasowang@...hat.com>
To:     Xuan Zhuo <xuanzhuo@...ux.alibaba.com>
Cc:     Jesper Dangaard Brouer <hawk@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        "Michael S. Tsirkin" <mst@...hat.com>,
        netdev <netdev@...r.kernel.org>,
        John Fastabend <john.fastabend@...il.com>,
        Alexei Starovoitov <ast@...nel.org>,
        virtualization <virtualization@...ts.linux-foundation.org>,
        Jakub Kicinski <kuba@...nel.org>, bpf@...r.kernel.org,
        "David S. Miller" <davem@...emloft.net>
Subject: Re: [PATCH v3 00/17] virtio pci support VIRTIO_F_RING_RESET

On Tue, Feb 8, 2022 at 3:56 PM Xuan Zhuo <xuanzhuo@...ux.alibaba.com> wrote:
>
> On Tue, 08 Feb 2022 11:14:45 +0800, Xuan Zhuo <xuanzhuo@...ux.alibaba.com> wrote:
> > On Tue, 8 Feb 2022 10:59:48 +0800, Jason Wang <jasowang@...hat.com> wrote:
> > >
> > > 在 2022/2/7 下午2:02, Xuan Zhuo 写道:
> > > > On Mon, 7 Feb 2022 11:39:36 +0800, Jason Wang <jasowang@...hat.com> wrote:
> > > >> On Wed, Jan 26, 2022 at 3:35 PM Xuan Zhuo <xuanzhuo@...ux.alibaba.com> wrote:
> > > >>> ================================================================================
> > > >>> The virtio spec already supports the virtio queue reset function. This patch set
> > > >>> is to add this function to the kernel. The relevant virtio spec information is
> > > >>> here:
> > > >>>
> > > >>>      https://github.com/oasis-tcs/virtio-spec/issues/124
> > > >>>
> > > >>> Also regarding MMIO support for queue reset, I plan to support it after this
> > > >>> patch is passed.
> > > >>>
> > > >>> #14-#17 is the disable/enable function of rx/tx pair implemented by virtio-net
> > > >>> using the new helper.
> > > >> One thing that came to mind is the steering. E.g if we disable an RX,
> > > >> should we stop steering packets to that queue?
>
> Regarding this spec, if there are multiple queues disabled at the same time, it
> will be a troublesome problem for the backend to select the queue,

I don't understand this, for such a kind of backend or device it can
simply claim not to support this feature.

> so I want to
> directly define that only one queue is allowed to reset at the same time, do you
> think this is appropriate?

This sounds very complicated. E.g how can we define an API that can
reset more than one queues? (currently PCI only support MMIO access).

>
> In terms of the implementation of backend queue reselection, it would be more
> convenient to implement if we drop packets directly. Do you think we must
> implement this reselection function?

Rethink of this. It should be fine and much simpler.

Thanks

>
> Thanks.
>
> > > > Yes, we should reselect a queue.
> > > >
> > > > Thanks.
> > >
> > >
> > > Maybe a spec patch for that?
> >
> > Yes, I also realized this. Although virtio-net's disable/enable is implemented
> > based on queue reset, virtio-net still has to define its own flag and define
> > some more detailed implementations.
> >
> > I'll think about it and post a spec patch.
> >
> > Thanks.
> >
> > >
> > > Thanks
> > >
> > >
> > > >
> > > >> Thanks
> > > >>
> > > >>> This function is not currently referenced by other
> > > >>> functions. It is more to show the usage of the new helper, I not sure if they
> > > >>> are going to be merged together.
> > > >>>
> > > >>> Please review. Thanks.
> > > >>>
> > > >>> v3:
> > > >>>    1. keep vq, irq unreleased
> > > >>>
> > > >>> Xuan Zhuo (17):
> > > >>>    virtio_pci: struct virtio_pci_common_cfg add queue_notify_data
> > > >>>    virtio: queue_reset: add VIRTIO_F_RING_RESET
> > > >>>    virtio: queue_reset: struct virtio_config_ops add callbacks for
> > > >>>      queue_reset
> > > >>>    virtio: queue_reset: add helper
> > > >>>    vritio_ring: queue_reset: extract the release function of the vq ring
> > > >>>    virtio_ring: queue_reset: split: add __vring_init_virtqueue()
> > > >>>    virtio_ring: queue_reset: split: support enable reset queue
> > > >>>    virtio_ring: queue_reset: packed: support enable reset queue
> > > >>>    virtio_ring: queue_reset: add vring_reset_virtqueue()
> > > >>>    virtio_pci: queue_reset: update struct virtio_pci_common_cfg and
> > > >>>      option functions
> > > >>>    virtio_pci: queue_reset: release vq by vp_dev->vqs
> > > >>>    virtio_pci: queue_reset: setup_vq use vring_setup_virtqueue()
> > > >>>    virtio_pci: queue_reset: support VIRTIO_F_RING_RESET
> > > >>>    virtio_net: virtnet_tx_timeout() fix style
> > > >>>    virtio_net: virtnet_tx_timeout() stop ref sq->vq
> > > >>>    virtio_net: split free_unused_bufs()
> > > >>>    virtio_net: support pair disable/enable
> > > >>>
> > > >>>   drivers/net/virtio_net.c               | 220 ++++++++++++++++++++++---
> > > >>>   drivers/virtio/virtio_pci_common.c     |  62 ++++---
> > > >>>   drivers/virtio/virtio_pci_common.h     |  11 +-
> > > >>>   drivers/virtio/virtio_pci_legacy.c     |   5 +-
> > > >>>   drivers/virtio/virtio_pci_modern.c     | 120 +++++++++++++-
> > > >>>   drivers/virtio/virtio_pci_modern_dev.c |  28 ++++
> > > >>>   drivers/virtio/virtio_ring.c           | 144 +++++++++++-----
> > > >>>   include/linux/virtio.h                 |   1 +
> > > >>>   include/linux/virtio_config.h          |  75 ++++++++-
> > > >>>   include/linux/virtio_pci_modern.h      |   2 +
> > > >>>   include/linux/virtio_ring.h            |  42 +++--
> > > >>>   include/uapi/linux/virtio_config.h     |   7 +-
> > > >>>   include/uapi/linux/virtio_pci.h        |   2 +
> > > >>>   13 files changed, 618 insertions(+), 101 deletions(-)
> > > >>>
> > > >>> --
> > > >>> 2.31.0
> > > >>>
> > >
> > _______________________________________________
> > Virtualization mailing list
> > Virtualization@...ts.linux-foundation.org
> > https://lists.linuxfoundation.org/mailman/listinfo/virtualization
>

Powered by blists - more mailing lists