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

[Gavin Li <gavinl@nvidia.com>]

On 8/6/2022 7:26 AM, Si-Wei Liu wrote:
> External email: Use caution opening links or attachments
>
>
> On 8/5/2022 3:11 PM, Si-Wei Liu wrote:
> > 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.
> >
> > > Â 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.
> >
> > > 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?
> >
> > 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
> ... , but can align with new mtu during .ndo_change_mtu(), too.
ACK but don't know how to do this.
> > ) in struct virtnet_info?
>
> Thanks,
> -Siwei
>
> > > +ÂÂÂ }
> > > +
> > > +ÂÂÂ sg_init_table(rq->sg, sg_num + 2);
> > > ÂÂÂÂÂÂÂ /* page in rq->sg[MAX_SKB_FRAGS + 1] is list tail */
> > Comment doesn't match code.
> > > -ÂÂÂ 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.
> >
> >
> > Thanks,
> > -Siwei
> > > +ÂÂÂ }
> > > ÂÂÂÂÂÂÂ if (virtio_has_feature(vdev, VIRTIO_NET_F_MRG_RXBUF))
> > > ÂÂÂÂÂÂÂÂÂ vi->mergeable_rx_bufs = true;