Re: [virtio-dev] [PATCH] virtio-net: use mtu size as buffer length for big packets

[Si-Wei Liu <si-wei.liu@oracle.com>]

On 8/8/2022 12:31 AM, Gavin Li wrote:
> On 8/6/2022 6:11 AM, Si-Wei Liu wrote:
> > External email: Use caution opening links or attachments
> >
> >
> > On 8/1/2022 9:45 PM, Gavin Li wrote:
> > > Currently add_recvbuf_big() allocates MAX_SKB_FRAGS segments for big
> > > packets even when GUEST_* offloads are not present on the device.
> > > However, if GSO is not supported,
> > GUEST GSO (virtio term), or GRO HW (netdev core term) it should have
> > been be called.
> ACK
> > > Â it would be sufficient to allocate
> > > segments to cover just up the MTU size and no further. Allocating the
> > > maximum amount of segments results in a large waste of buffer space in
> > > the queue, which limits the number of packets that can be buffered and
> > > can result in reduced performance.
> > >
> > > Therefore, if GSO is not supported,
> > Ditto.
> ACK
> > > use the MTU to calculate the
> > > optimal amount of segments required.
> > >
> > > Below is the iperf TCP test results over a Mellanox NIC, using vDPA for
> > > 1 VQ, queue size 1024, before and after the change, with the iperf
> > > server running over the virtio-net interface.
> > >
> > > MTU(Bytes)/Bandwidth (Gbit/s)
> > > ÂÂÂÂÂÂÂÂÂÂÂÂÂ BeforeÂÂ After
> > > ÂÂ 1500ÂÂÂÂÂÂÂ 22.5ÂÂÂÂ 22.4
> > > ÂÂ 9000ÂÂÂÂÂÂÂ 12.8ÂÂÂÂ 25.9
> > >
> > > Signed-off-by: Gavin Li <gavinl@nvidia.com>
> > > Reviewed-by: Gavi Teitz <gavi@nvidia.com>
> > > Reviewed-by: Parav Pandit <parav@nvidia.com>
> > > ---
> > > Â drivers/net/virtio_net.c | 20 ++++++++++++++++----
> > > Â 1 file changed, 16 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > index ec8e1b3108c3..d36918c1809d 100644
> > > --- a/drivers/net/virtio_net.c
> > > +++ b/drivers/net/virtio_net.c
> > > @@ -222,6 +222,9 @@ struct virtnet_info {
> > > ÂÂÂÂÂ /* I like... big packets and I cannot lie! */
> > > ÂÂÂÂÂ bool big_packets;
> > >
> > > +ÂÂÂÂ /* Indicates GSO support */
> > > +ÂÂÂÂ bool gso_is_supported;
> > > +
> > > ÂÂÂÂÂ /* Host will merge rx buffers for big packets (shake it! shake 
> > > it!) */
> > > ÂÂÂÂÂ bool mergeable_rx_bufs;
> > >
> > > @@ -1312,14 +1315,21 @@ static int add_recvbuf_small(struct 
> > > virtnet_info *vi, struct receive_queue *rq,
> > > Â static int add_recvbuf_big(struct virtnet_info *vi, struct 
> > > receive_queue *rq,
> > > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ gfp_t gfp)
> > > Â {
> > > +ÂÂÂÂ unsigned int sg_num = MAX_SKB_FRAGS;
> > > ÂÂÂÂÂ struct page *first, *list = NULL;
> > > ÂÂÂÂÂ char *p;
> > > ÂÂÂÂÂ int i, err, offset;
> > >
> > > -ÂÂÂÂ sg_init_table(rq->sg, MAX_SKB_FRAGS + 2);
> > > +ÂÂÂÂ if (!vi->gso_is_supported) {
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂ unsigned int mtu = vi->dev->mtu;
> > > +
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂ sg_num = (mtu % PAGE_SIZE) ? mtu / PAGE_SIZE + 1 : mtu 
> > > / PAGE_SIZE;
> > DIV_ROUND_UP() can be used?
> ACK
> > Since this branch slightly adds up cost to the datapath, I wonder if
> > this sg_num can be saved and set only once (generally in virtnet_probe
> > time) in struct virtnet_info?
> Not sure how to do it and align it with align with new mtu during 
> .ndo_change_mtu()---as you mentioned in the following mail. Any idea? 
> ndo_change_mtu might be in vendor specific code and unmanageable. In 
> my case, the mtu can only be changed in the xml of the guest vm.
Nope, for e.g. "ip link dev eth0 set mtu 1500" can be done from guest on 
a virtio-net device with 9000 MTU (as defined in guest xml). Basically 
guest user can set MTU to any valid value lower than the original 
HOST_MTU. In the vendor defined .ndo_change_mtu() op, dev_validate_mtu() 
should have validated the MTU value before coming down to it. And I 
suspect you might want to do virtnet_close() and virtnet_open() 
before/after changing the buffer size on the fly (the netif_running() 
case), implementing .ndo_change_mtu() will be needed anyway.

> > > +ÂÂÂÂ }
> > > +
> > > +ÂÂÂÂ sg_init_table(rq->sg, sg_num + 2);
> > >
> > > ÂÂÂÂÂ /* page in rq->sg[MAX_SKB_FRAGS + 1] is list tail */
> > Comment doesn't match code.
> ACK
> > > -ÂÂÂÂ for (i = MAX_SKB_FRAGS + 1; i > 1; --i) {
> > > +ÂÂÂÂ for (i = sg_num + 1; i > 1; --i) {
> > > ÂÂÂÂÂÂÂÂÂÂÂÂÂ first = get_a_page(rq, gfp);
> > > ÂÂÂÂÂÂÂÂÂÂÂÂÂ if (!first) {
> > > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ if (list)
> > > @@ -1350,7 +1360,7 @@ static int add_recvbuf_big(struct virtnet_info 
> > > *vi, struct receive_queue *rq,
> > >
> > > ÂÂÂÂÂ /* chain first in list head */
> > > ÂÂÂÂÂ first->private = (unsigned long)list;
> > > -ÂÂÂÂ err = virtqueue_add_inbuf(rq->vq, rq->sg, MAX_SKB_FRAGS + 2,
> > > +ÂÂÂÂ err = virtqueue_add_inbuf(rq->vq, rq->sg, sg_num + 2,
> > > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ first, gfp);
> > > ÂÂÂÂÂ if (err < 0)
> > > ÂÂÂÂÂÂÂÂÂÂÂÂÂ give_pages(rq, first);
> > > @@ -3571,8 +3581,10 @@ static int virtnet_probe(struct virtio_device 
> > > *vdev)
> > > ÂÂÂÂÂ if (virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_TSO4) ||
> > > ÂÂÂÂÂÂÂÂÂ virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_TSO6) ||
> > > ÂÂÂÂÂÂÂÂÂ virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_ECN) ||
> > > -ÂÂÂÂÂÂÂÂ virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_UFO))
> > > +ÂÂÂÂÂÂÂÂ virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_UFO)) {
> > > ÂÂÂÂÂÂÂÂÂÂÂÂÂ vi->big_packets = true;
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂ vi->gso_is_supported = true;
> > Please do the same for virtnet_clear_guest_offloads(), and
> > correspondingly virtnet_restore_guest_offloads() as well. Not sure why
> > virtnet_clear_guest_offloads() or the caller doesn't unset big_packet on
> > successful return, seems like a bug to me.
> ACK. The two calls virtnet_set_guest_offloads and 
> virtnet_set_guest_offloads is also called by virtnet_set_features. Do 
> you think if I can do this in virtnet_set_guest_offloads?
I think that it should be fine, though you may want to deal with the XDP 
path not to regress it.

-Siwei

> > Thanks,
> > -Siwei
> > > +ÂÂÂÂ }
> > >
> > > ÂÂÂÂÂ if (virtio_has_feature(vdev, VIRTIO_NET_F_MRG_RXBUF))
> > > ÂÂÂÂÂÂÂÂÂÂÂÂÂ vi->mergeable_rx_bufs = true;