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

[Parav Pandit via Virtualization]

> From: Si-Wei Liu <si-wei....@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);
}
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization