Re: [PATCH v3 1/5] virtio_net: Revert "virtio_net: set the default max ring size by find_vqs()"

[Guenter Roeck]

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 <andres@xxxxxxxxxxx>
> 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 <xuanzhuo@xxxxxxxxxxxxxxxxx>
> Cc: Jason Wang <jasowang@xxxxxxxxxx>
> Signed-off-by: Michael S. Tsirkin <mst@xxxxxxxxxx>
> Tested-by: Andres Freund <andres@xxxxxxxxxxx>
> Tested-by: From: Guenter Roeck <linux@xxxxxxxxxxxx>

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
>