[PATCH v3 0/5] virtio: drop sizing vqs during init

Michael S. Tsirkin posted 5 patches 3 years, 8 months ago
There is a newer version of this series
arch/um/drivers/virtio_uml.c             |  2 +-
drivers/net/virtio_net.c                 | 42 +++---------------------
drivers/platform/mellanox/mlxbf-tmfifo.c |  1 -
drivers/remoteproc/remoteproc_virtio.c   |  1 -
drivers/s390/virtio/virtio_ccw.c         |  1 -
drivers/virtio/virtio_mmio.c             |  9 ++---
drivers/virtio/virtio_pci_common.c       | 20 +++++------
drivers/virtio/virtio_pci_common.h       |  3 +-
drivers/virtio/virtio_pci_legacy.c       |  6 +---
drivers/virtio/virtio_pci_modern.c       | 17 +++-------
drivers/virtio/virtio_vdpa.c             |  1 -
include/linux/virtio_config.h            | 26 +++------------
12 files changed, 28 insertions(+), 101 deletions(-)
[PATCH v3 0/5] virtio: drop sizing vqs during init
Posted by Michael S. Tsirkin 3 years, 8 months ago
Reporting after I botched up v2 posting. Sorry about the noise.

Supplying size during init does not work for all transports.
In fact for legacy pci doing that causes a memory
corruption which was reported on Google Cloud.

We might get away with changing size to size_hint so it's
safe to ignore and then fixing legacy to ignore the hint.

But the benefit is unclear in any case, so let's revert for now.
Any new version will have to come with
- documentation of performance gains
- performance testing showing existing workflows
  are not harmed materially. especially ones with
  bursty traffic
- report of testing on legacy devices


Huge shout out to Andres Freund for the effort spent reproducing and
debugging!  Thanks to Guenter Roeck for help with testing!


changes from v2
	drop unrelated patches
changes from v1
	revert the ring size api, it's unused now

Michael S. Tsirkin (5):
  virtio_net: Revert "virtio_net: set the default max ring size by
    find_vqs()"
  virtio: Revert "virtio: add helper virtio_find_vqs_ctx_size()"
  virtio-mmio: Revert "virtio_mmio: support the arg sizes of find_vqs()"
  virtio_pci: Revert "virtio_pci: support the arg sizes of find_vqs()"
  virtio: Revert "virtio: find_vqs() add arg sizes"

 arch/um/drivers/virtio_uml.c             |  2 +-
 drivers/net/virtio_net.c                 | 42 +++---------------------
 drivers/platform/mellanox/mlxbf-tmfifo.c |  1 -
 drivers/remoteproc/remoteproc_virtio.c   |  1 -
 drivers/s390/virtio/virtio_ccw.c         |  1 -
 drivers/virtio/virtio_mmio.c             |  9 ++---
 drivers/virtio/virtio_pci_common.c       | 20 +++++------
 drivers/virtio/virtio_pci_common.h       |  3 +-
 drivers/virtio/virtio_pci_legacy.c       |  6 +---
 drivers/virtio/virtio_pci_modern.c       | 17 +++-------
 drivers/virtio/virtio_vdpa.c             |  1 -
 include/linux/virtio_config.h            | 26 +++------------
 12 files changed, 28 insertions(+), 101 deletions(-)

-- 
MST
Re: [PATCH v3 0/5] virtio: drop sizing vqs during init
Posted by Xuan Zhuo 3 years, 8 months ago
Series:
Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>

There is also a commit, I just submitted, about the problem you pointed
out about using container_of(). Can we submit together?


On Mon, 15 Aug 2022 18:00:21 -0400, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> Reporting after I botched up v2 posting. Sorry about the noise.
>
> Supplying size during init does not work for all transports.
> In fact for legacy pci doing that causes a memory
> corruption which was reported on Google Cloud.
>
> We might get away with changing size to size_hint so it's
> safe to ignore and then fixing legacy to ignore the hint.
>
> But the benefit is unclear in any case, so let's revert for now.
> Any new version will have to come with
> - documentation of performance gains
> - performance testing showing existing workflows
>   are not harmed materially. especially ones with
>   bursty traffic
> - report of testing on legacy devices
>
>
> Huge shout out to Andres Freund for the effort spent reproducing and
> debugging!  Thanks to Guenter Roeck for help with testing!
>
>
> changes from v2
> 	drop unrelated patches
> changes from v1
> 	revert the ring size api, it's unused now
>
> Michael S. Tsirkin (5):
>   virtio_net: Revert "virtio_net: set the default max ring size by
>     find_vqs()"
>   virtio: Revert "virtio: add helper virtio_find_vqs_ctx_size()"
>   virtio-mmio: Revert "virtio_mmio: support the arg sizes of find_vqs()"
>   virtio_pci: Revert "virtio_pci: support the arg sizes of find_vqs()"
>   virtio: Revert "virtio: find_vqs() add arg sizes"
>
>  arch/um/drivers/virtio_uml.c             |  2 +-
>  drivers/net/virtio_net.c                 | 42 +++---------------------
>  drivers/platform/mellanox/mlxbf-tmfifo.c |  1 -
>  drivers/remoteproc/remoteproc_virtio.c   |  1 -
>  drivers/s390/virtio/virtio_ccw.c         |  1 -
>  drivers/virtio/virtio_mmio.c             |  9 ++---
>  drivers/virtio/virtio_pci_common.c       | 20 +++++------
>  drivers/virtio/virtio_pci_common.h       |  3 +-
>  drivers/virtio/virtio_pci_legacy.c       |  6 +---
>  drivers/virtio/virtio_pci_modern.c       | 17 +++-------
>  drivers/virtio/virtio_vdpa.c             |  1 -
>  include/linux/virtio_config.h            | 26 +++------------
>  12 files changed, 28 insertions(+), 101 deletions(-)
>
> --
> MST
>
Re: [PATCH v3 0/5] virtio: drop sizing vqs during init
Posted by Linus Torvalds 3 years, 8 months ago
On Mon, Aug 15, 2022 at 3:00 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> But the benefit is unclear in any case, so let's revert for now.

Should I take this patch series directly, or will you be sending a
pull request (preferred)?

             Linus
Re: [PATCH v3 0/5] virtio: drop sizing vqs during init
Posted by Michael S. Tsirkin 3 years, 8 months ago
On Mon, Aug 15, 2022 at 03:24:28PM -0700, Linus Torvalds wrote:
> On Mon, Aug 15, 2022 at 3:00 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > But the benefit is unclear in any case, so let's revert for now.
> 
> Should I take this patch series directly, or will you be sending a
> pull request (preferred)?
> 
>              Linus

I'll be sending a pull request, just not today - I try not to do
this at strange hours of night.

-- 
MST