RE: [virtio-dev] [PATCH RESEND v2 2/2] virtio-net: use mtu size as buffer length for big packets

[Parav Pandit <parav@nvidia.com>]

> From: Si-Wei Liu <si-wei.liu@oracle.com>
> Sent: Friday, August 26, 2022 2:05 PM
> 

> > +	/* If we can receive ANY GSO packets, we must allocate large ones.
> */
> > +	if (mtu > ETH_DATA_LEN || guest_gso) {
> > +		vi->big_packets = true;
> > +		/* if the guest offload is offered by the device, user can
> modify
> > +		 * offload capability. In such posted buffers may short fall of
> size.
> > +		 * Hence allocate for max size.
> > +		 */
> > +		if (virtio_has_feature(vi->vdev,
> VIRTIO_NET_F_CTRL_GUEST_OFFLOADS))
> > +			vi->big_packets_sg_num = MAX_SKB_FRAGS;
> >> MAX_SKB_FRAGS is needed when any of the guest_gso capability is
> offered. This is per spec regardless if
> VIRTIO_NET_F_CTRL_GUEST_OFFLOADS is negotiated or not. Quoting spec:
> >
> >
> >> If VIRTIO_NET_F_MRG_RXBUF is not negotiated:
> >> If VIRTIO_NET_F_GUEST_TSO4, VIRTIO_NET_F_GUEST_TSO6 or
> VIRTIO_NET_F_GUEST_UFO are negotiated, the driver SHOULD populate the
> receive queue(s) with buffers of at least 65562 bytes.
> >
> > Spec recommendation is good here, but Linux driver knows that such
> offload settings cannot change if the above feature is not offered.
> > So I think we should add the comment and reference to the code to have
> this optimization.
> 
> I don't get what you mean by optimization here. Say if
> VIRTIO_NET_F_GUEST_TSO4 is negotiated while
> VIRTIO_NET_F_CTRL_GUEST_OFFLOADS is not offered, why you consider it
> an optimization to post only MTU sized (with roundup to page) buffers?
> Wouldn't it be an issue if the device is passing up aggregated GSO packets of
> up to 64KB? Noted, GUEST_GSO is implied on when the corresponding
> feature bit is negotiated, regardless the presence of
> VIRTIO_NET_F_CTRL_GUEST_OFFLOADS bit.
> 
You are right.
NETIF_F_GRO_HW setting of the netdev is already guarded by VIRTIO_NET_F_CTRL_GUEST_OFFLOADS bit check.
So, its fine. I missed that code previously.

I agree the condition check should be simpler like below.

if (guest_gso || mtu > ETH_DATA_LEN) {
	vi->big_packets = true;
	vi->big_packets_sg_num = guest_gso ? MAX_SKB_FRAGS : DIV_ROUND_UP(mtu, PAGE_SIZE);
}