Re: [PATCH netdev 1/2] virtio: add module option to turn off guest offloads

Re: [PATCH netdev 1/2] virtio: add module option to turn off guest offloads

[Jason Wang]

On 2020/11/12 下午4:11, Xuan Zhuo wrote:
> * VIRTIO_NET_F_GUEST_CSUM
> * VIRTIO_NET_F_GUEST_TSO4
> * VIRTIO_NET_F_GUEST_TSO6
> * VIRTIO_NET_F_GUEST_ECN
> * VIRTIO_NET_F_GUEST_UFO
> * VIRTIO_NET_F_MTU
>
> If these features are negotiated successfully, it may cause virtio-net to
> receive large packages, which will cause xdp to fail to load. And in
> many cases, it cannot be dynamically turned off, so add a module option
> to turn off these features.


Actually we will disable those through control virtqueue. Or does it 
mean your hardware doesn't support control guest offloads?

Module parameters may introduce a lot of burden for management and 
use-ability.

Disabling guest offloads means you may suffer from low RX throughput.

Thanks


>
> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> ---
>   drivers/net/virtio_net.c | 36 +++++++++++++++++++++++++++++++++++-
>   1 file changed, 35 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 21b7114..232a539 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -26,10 +26,11 @@
>   static int napi_weight = NAPI_POLL_WEIGHT;
>   module_param(napi_weight, int, 0444);
>   
> -static bool csum = true, gso = true, napi_tx = true;
> +static bool csum = true, gso = true, napi_tx, guest_offload = true;
>   module_param(csum, bool, 0444);
>   module_param(gso, bool, 0444);
>   module_param(napi_tx, bool, 0644);
> +module_param(guest_offload, bool, 0644);
>   
>   /* FIXME: MTU in config. */
>   #define GOOD_PACKET_LEN (ETH_HLEN + VLAN_HLEN + ETH_DATA_LEN)
> @@ -3245,6 +3246,18 @@ static __maybe_unused int virtnet_restore(struct virtio_device *vdev)
>   	VIRTIO_NET_F_MTU, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS, \
>   	VIRTIO_NET_F_SPEED_DUPLEX, VIRTIO_NET_F_STANDBY
>   
> +#define VIRTNET_FEATURES_WITHOUT_GUEST_OFFLOADS \
> +	VIRTIO_NET_F_CSUM, \
> +	VIRTIO_NET_F_MAC, \
> +	VIRTIO_NET_F_HOST_TSO4, VIRTIO_NET_F_HOST_UFO, VIRTIO_NET_F_HOST_TSO6, \
> +	VIRTIO_NET_F_HOST_ECN, \
> +	VIRTIO_NET_F_MRG_RXBUF, VIRTIO_NET_F_STATUS, VIRTIO_NET_F_CTRL_VQ, \
> +	VIRTIO_NET_F_CTRL_RX, VIRTIO_NET_F_CTRL_VLAN, \
> +	VIRTIO_NET_F_GUEST_ANNOUNCE, VIRTIO_NET_F_MQ, \
> +	VIRTIO_NET_F_CTRL_MAC_ADDR, \
> +	VIRTIO_NET_F_CTRL_GUEST_OFFLOADS, \
> +	VIRTIO_NET_F_SPEED_DUPLEX, VIRTIO_NET_F_STANDBY
> +
>   static unsigned int features[] = {
>   	VIRTNET_FEATURES,
>   };
> @@ -3255,6 +3268,16 @@ static __maybe_unused int virtnet_restore(struct virtio_device *vdev)
>   	VIRTIO_F_ANY_LAYOUT,
>   };
>   
> +static unsigned int features_without_offloads[] = {
> +	VIRTNET_FEATURES_WITHOUT_GUEST_OFFLOADS,
> +};
> +
> +static unsigned int features_without_offloads_legacy[] = {
> +	VIRTNET_FEATURES_WITHOUT_GUEST_OFFLOADS,
> +	VIRTIO_NET_F_GSO,
> +	VIRTIO_F_ANY_LAYOUT,
> +};
> +
>   static struct virtio_driver virtio_net_driver = {
>   	.feature_table = features,
>   	.feature_table_size = ARRAY_SIZE(features),
> @@ -3288,6 +3311,17 @@ static __init int virtio_net_driver_init(void)
>   	if (ret)
>   		goto err_dead;
>   
> +	if (!guest_offload) {
> +		virtio_net_driver.feature_table = features_without_offloads;
> +		virtio_net_driver.feature_table_size =
> +			ARRAY_SIZE(features_without_offloads);
> +
> +		virtio_net_driver.feature_table_legacy =
> +			features_without_offloads_legacy;
> +		virtio_net_driver.feature_table_size_legacy =
> +			ARRAY_SIZE(features_without_offloads_legacy);
> +	}
> +
>           ret = register_virtio_driver(&virtio_net_driver);
>   	if (ret)
>   		goto err_virtio;

Re: [PATCH netdev 2/2] virtio, xdp: Allow xdp to load, even if there is not enough queue

[Jason Wang]

On 2020/11/12 下午5:15, Xuan Zhuo wrote:
> Since XDP_TX uses an independent queue to send data by default, and
> requires a queue for each CPU, this condition is often not met when the
> number of CPUs is relatively large, but XDP_TX is not used in many
> cases. I hope In this case, XDP is allowed to load and a warning message
> is submitted. If the user uses XDP_TX, another error is generated.
>
> This is not a perfect solution, but I still hope to solve some of the
> problems first.
>
> Signed-off-by: Xuan Zhuo<xuanzhuo@linux.alibaba.com>


This leads bad user experiences.

Let's do something like this:

1) When TX queues is sufficient, go as in the past
2) When TX queue is not, use tx lock to synchronize

Thanks

Re: [PATCH netdev 1/2] virtio: add module option to turn off guest offloads

[Jesper Dangaard Brouer]

On Fri, 13 Nov 2020 09:05:19 +0800
Jason Wang <jasowang@redhat.com> wrote:

> On 2020/11/12 下午4:11, Xuan Zhuo wrote:
> > * VIRTIO_NET_F_GUEST_CSUM
> > * VIRTIO_NET_F_GUEST_TSO4
> > * VIRTIO_NET_F_GUEST_TSO6
> > * VIRTIO_NET_F_GUEST_ECN
> > * VIRTIO_NET_F_GUEST_UFO
> > * VIRTIO_NET_F_MTU
> >
> > If these features are negotiated successfully, it may cause virtio-net to
> > receive large packages, which will cause xdp to fail to load. 

I really want the people that implement XDP multi-buffer support, to
think about how this can help virtio-net overcome these limitations.

IHMO XDP need to evolve to support these features for virtio-net.

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer