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
This reverts commit 762faee5a2678559d3dc09d95f8f2c54cd0466a7.
This has been reported to trip up guests on GCP (Google Cloud).
The reason is that virtio_find_vqs_ctx_size is broken on legacy
devices. We can in theory fix virtio_find_vqs_ctx_size but
in fact the patch itself has several other issues:
- It treats unknown speed as < 10G
- It leaves userspace no way to find out the ring size set by hypervisor
- It tests speed when link is down
- It ignores the virtio spec advice:
Both \field{speed} and \field{duplex} can change, thus the driver
is expected to re-read these values after receiving a
configuration change notification.
- It is not clear the performance impact has been tested properly
Revert the patch for now.
Reported-by: Andres Freund <[email protected]>
Link: https://lore.kernel.org/r/20220814212610.GA3690074%40roeck-us.net
Link: https://lore.kernel.org/r/20220815070203.plwjx7b3cyugpdt7%40awork3.anarazel.de
Link: https://lore.kernel.org/r/3df6bb82-1951-455d-a768-e9e1513eb667%40www.fastmail.com
Link: https://lore.kernel.org/r/FCDC5DDE-3CDD-4B8A-916F-CA7D87B547CE%40anarazel.de
Fixes: 762faee5a267 ("virtio_net: set the default max ring size by find_vqs()")
Cc: Xuan Zhuo <[email protected]>
Cc: Jason Wang <[email protected]>
Signed-off-by: Michael S. Tsirkin <[email protected]>
Tested-by: Andres Freund <[email protected]>
Tested-by: From: Guenter Roeck <[email protected]>
---
drivers/net/virtio_net.c | 42 ++++------------------------------------
1 file changed, 4 insertions(+), 38 deletions(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index d934774e9733..ece00b84e3a7 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -3432,29 +3432,6 @@ static unsigned int mergeable_min_buf_len(struct virtnet_info *vi, struct virtqu
(unsigned int)GOOD_PACKET_LEN);
}
-static void virtnet_config_sizes(struct virtnet_info *vi, u32 *sizes)
-{
- u32 i, rx_size, tx_size;
-
- if (vi->speed == SPEED_UNKNOWN || vi->speed < SPEED_10000) {
- rx_size = 1024;
- tx_size = 1024;
-
- } else if (vi->speed < SPEED_40000) {
- rx_size = 1024 * 4;
- tx_size = 1024 * 4;
-
- } else {
- rx_size = 1024 * 8;
- tx_size = 1024 * 8;
- }
-
- for (i = 0; i < vi->max_queue_pairs; i++) {
- sizes[rxq2vq(i)] = rx_size;
- sizes[txq2vq(i)] = tx_size;
- }
-}
-
static int virtnet_find_vqs(struct virtnet_info *vi)
{
vq_callback_t **callbacks;
@@ -3462,7 +3439,6 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
int ret = -ENOMEM;
int i, total_vqs;
const char **names;
- u32 *sizes;
bool *ctx;
/* We expect 1 RX virtqueue followed by 1 TX virtqueue, followed by
@@ -3490,15 +3466,10 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
ctx = NULL;
}
- sizes = kmalloc_array(total_vqs, sizeof(*sizes), GFP_KERNEL);
- if (!sizes)
- goto err_sizes;
-
/* Parameters for control virtqueue, if any */
if (vi->has_cvq) {
callbacks[total_vqs - 1] = NULL;
names[total_vqs - 1] = "control";
- sizes[total_vqs - 1] = 64;
}
/* Allocate/initialize parameters for send/receive virtqueues */
@@ -3513,10 +3484,8 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
ctx[rxq2vq(i)] = true;
}
- virtnet_config_sizes(vi, sizes);
-
- ret = virtio_find_vqs_ctx_size(vi->vdev, total_vqs, vqs, callbacks,
- names, sizes, ctx, NULL);
+ ret = virtio_find_vqs_ctx(vi->vdev, total_vqs, vqs, callbacks,
+ names, ctx, NULL);
if (ret)
goto err_find;
@@ -3536,8 +3505,6 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
err_find:
- kfree(sizes);
-err_sizes:
kfree(ctx);
err_ctx:
kfree(names);
@@ -3897,9 +3864,6 @@ static int virtnet_probe(struct virtio_device *vdev)
vi->curr_queue_pairs = num_online_cpus();
vi->max_queue_pairs = max_queue_pairs;
- virtnet_init_settings(dev);
- virtnet_update_settings(vi);
-
/* Allocate/initialize the rx/tx queues, and invoke find_vqs */
err = init_vqs(vi);
if (err)
@@ -3912,6 +3876,8 @@ static int virtnet_probe(struct virtio_device *vdev)
netif_set_real_num_tx_queues(dev, vi->curr_queue_pairs);
netif_set_real_num_rx_queues(dev, vi->curr_queue_pairs);
+ virtnet_init_settings(dev);
+
if (virtio_has_feature(vdev, VIRTIO_NET_F_STANDBY)) {
vi->failover = net_failover_create(vi->dev);
if (IS_ERR(vi->failover)) {
--
MST
This reverts commit fe3dc04e31aa51f91dc7f741a5f76cc4817eb5b4: the
API is now unused and in fact can't be implemented on top of a legacy
device.
Fixes: fe3dc04e31aa ("virtio: add helper virtio_find_vqs_ctx_size()")
Cc: "Xuan Zhuo" <[email protected]>
Signed-off-by: Michael S. Tsirkin <[email protected]>
---
include/linux/virtio_config.h | 12 ------------
1 file changed, 12 deletions(-)
diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
index 6adff09f7170..888f7e96f0c7 100644
--- a/include/linux/virtio_config.h
+++ b/include/linux/virtio_config.h
@@ -241,18 +241,6 @@ int virtio_find_vqs_ctx(struct virtio_device *vdev, unsigned nvqs,
ctx, desc);
}
-static inline
-int virtio_find_vqs_ctx_size(struct virtio_device *vdev, u32 nvqs,
- struct virtqueue *vqs[],
- vq_callback_t *callbacks[],
- const char * const names[],
- u32 sizes[],
- const bool *ctx, struct irq_affinity *desc)
-{
- return vdev->config->find_vqs(vdev, nvqs, vqs, callbacks, names, sizes,
- ctx, desc);
-}
-
/**
* virtio_synchronize_cbs - synchronize with virtqueue callbacks
* @vdev: the device
--
MST
This reverts commit fbed86abba6e0472d98079790e58060e4332608a.
The API is now unused, let's not carry dead code around.
Fixes: fbed86abba6e ("virtio_mmio: support the arg sizes of find_vqs()")
Signed-off-by: Michael S. Tsirkin <[email protected]>
---
drivers/virtio/virtio_mmio.c | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
index c492a57531c6..dfcecfd7aba1 100644
--- a/drivers/virtio/virtio_mmio.c
+++ b/drivers/virtio/virtio_mmio.c
@@ -360,7 +360,7 @@ static void vm_synchronize_cbs(struct virtio_device *vdev)
static struct virtqueue *vm_setup_vq(struct virtio_device *vdev, unsigned int index,
void (*callback)(struct virtqueue *vq),
- const char *name, u32 size, bool ctx)
+ const char *name, bool ctx)
{
struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vdev);
struct virtio_mmio_vq_info *info;
@@ -395,11 +395,8 @@ static struct virtqueue *vm_setup_vq(struct virtio_device *vdev, unsigned int in
goto error_new_virtqueue;
}
- if (!size || size > num)
- size = num;
-
/* Create the vring */
- vq = vring_create_virtqueue(index, size, VIRTIO_MMIO_VRING_ALIGN, vdev,
+ vq = vring_create_virtqueue(index, num, VIRTIO_MMIO_VRING_ALIGN, vdev,
true, true, ctx, vm_notify, callback, name);
if (!vq) {
err = -ENOMEM;
@@ -503,7 +500,6 @@ static int vm_find_vqs(struct virtio_device *vdev, unsigned int nvqs,
}
vqs[i] = vm_setup_vq(vdev, queue_idx++, callbacks[i], names[i],
- sizes ? sizes[i] : 0,
ctx ? ctx[i] : false);
if (IS_ERR(vqs[i])) {
vm_del_vqs(vdev);
--
MST
On Mon, Aug 15, 2022 at 06:00:25PM -0400, Michael S. Tsirkin wrote:
> This reverts commit 762faee5a2678559d3dc09d95f8f2c54cd0466a7.
>
> This has been reported to trip up guests on GCP (Google Cloud).
> The reason is that virtio_find_vqs_ctx_size is broken on legacy
> devices. We can in theory fix virtio_find_vqs_ctx_size but
> in fact the patch itself has several other issues:
>
> - It treats unknown speed as < 10G
> - It leaves userspace no way to find out the ring size set by hypervisor
> - It tests speed when link is down
> - It ignores the virtio spec advice:
> Both \field{speed} and \field{duplex} can change, thus the driver
> is expected to re-read these values after receiving a
> configuration change notification.
> - It is not clear the performance impact has been tested properly
>
> Revert the patch for now.
>
> Reported-by: Andres Freund <[email protected]>
> Link: https://lore.kernel.org/r/20220814212610.GA3690074%40roeck-us.net
> Link: https://lore.kernel.org/r/20220815070203.plwjx7b3cyugpdt7%40awork3.anarazel.de
> Link: https://lore.kernel.org/r/3df6bb82-1951-455d-a768-e9e1513eb667%40www.fastmail.com
> Link: https://lore.kernel.org/r/FCDC5DDE-3CDD-4B8A-916F-CA7D87B547CE%40anarazel.de
> Fixes: 762faee5a267 ("virtio_net: set the default max ring size by find_vqs()")
> Cc: Xuan Zhuo <[email protected]>
> Cc: Jason Wang <[email protected]>
> Signed-off-by: Michael S. Tsirkin <[email protected]>
> Tested-by: Andres Freund <[email protected]>
> Tested-by: From: Guenter Roeck <[email protected]>
s/From: //
> ---
> drivers/net/virtio_net.c | 42 ++++------------------------------------
> 1 file changed, 4 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index d934774e9733..ece00b84e3a7 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -3432,29 +3432,6 @@ static unsigned int mergeable_min_buf_len(struct virtnet_info *vi, struct virtqu
> (unsigned int)GOOD_PACKET_LEN);
> }
>
> -static void virtnet_config_sizes(struct virtnet_info *vi, u32 *sizes)
> -{
> - u32 i, rx_size, tx_size;
> -
> - if (vi->speed == SPEED_UNKNOWN || vi->speed < SPEED_10000) {
> - rx_size = 1024;
> - tx_size = 1024;
> -
> - } else if (vi->speed < SPEED_40000) {
> - rx_size = 1024 * 4;
> - tx_size = 1024 * 4;
> -
> - } else {
> - rx_size = 1024 * 8;
> - tx_size = 1024 * 8;
> - }
> -
> - for (i = 0; i < vi->max_queue_pairs; i++) {
> - sizes[rxq2vq(i)] = rx_size;
> - sizes[txq2vq(i)] = tx_size;
> - }
> -}
> -
> static int virtnet_find_vqs(struct virtnet_info *vi)
> {
> vq_callback_t **callbacks;
> @@ -3462,7 +3439,6 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
> int ret = -ENOMEM;
> int i, total_vqs;
> const char **names;
> - u32 *sizes;
> bool *ctx;
>
> /* We expect 1 RX virtqueue followed by 1 TX virtqueue, followed by
> @@ -3490,15 +3466,10 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
> ctx = NULL;
> }
>
> - sizes = kmalloc_array(total_vqs, sizeof(*sizes), GFP_KERNEL);
> - if (!sizes)
> - goto err_sizes;
> -
> /* Parameters for control virtqueue, if any */
> if (vi->has_cvq) {
> callbacks[total_vqs - 1] = NULL;
> names[total_vqs - 1] = "control";
> - sizes[total_vqs - 1] = 64;
> }
>
> /* Allocate/initialize parameters for send/receive virtqueues */
> @@ -3513,10 +3484,8 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
> ctx[rxq2vq(i)] = true;
> }
>
> - virtnet_config_sizes(vi, sizes);
> -
> - ret = virtio_find_vqs_ctx_size(vi->vdev, total_vqs, vqs, callbacks,
> - names, sizes, ctx, NULL);
> + ret = virtio_find_vqs_ctx(vi->vdev, total_vqs, vqs, callbacks,
> + names, ctx, NULL);
> if (ret)
> goto err_find;
>
> @@ -3536,8 +3505,6 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
>
>
> err_find:
> - kfree(sizes);
> -err_sizes:
> kfree(ctx);
> err_ctx:
> kfree(names);
> @@ -3897,9 +3864,6 @@ static int virtnet_probe(struct virtio_device *vdev)
> vi->curr_queue_pairs = num_online_cpus();
> vi->max_queue_pairs = max_queue_pairs;
>
> - virtnet_init_settings(dev);
> - virtnet_update_settings(vi);
> -
> /* Allocate/initialize the rx/tx queues, and invoke find_vqs */
> err = init_vqs(vi);
> if (err)
> @@ -3912,6 +3876,8 @@ static int virtnet_probe(struct virtio_device *vdev)
> netif_set_real_num_tx_queues(dev, vi->curr_queue_pairs);
> netif_set_real_num_rx_queues(dev, vi->curr_queue_pairs);
>
> + virtnet_init_settings(dev);
> +
> if (virtio_has_feature(vdev, VIRTIO_NET_F_STANDBY)) {
> vi->failover = net_failover_create(vi->dev);
> if (IS_ERR(vi->failover)) {
> --
> MST
>
Series:
Reviewed-by: Xuan Zhuo <[email protected]>
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" <[email protected]> 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
>