On Wed, Sep 07, 2022 at 02:08:18PM +0000, Parav Pandit wrote:

quoted

From: Michael S. Tsirkin <mst@redhat.com>
Sent: Wednesday, September 7, 2022 5:27 AM

On Wed, Sep 07, 2022 at 04:08:54PM +0800, Gavin Li wrote:

quoted

On 9/7/2022 1:31 PM, Michael S. Tsirkin wrote:

quoted

External email: Use caution opening links or attachments


On Thu, Sep 01, 2022 at 05:10:38AM +0300, Gavin Li wrote:

quoted

Currently add_recvbuf_big() allocates MAX_SKB_FRAGS segments for
big packets even when GUEST_* offloads are not present on the

device.

quoted

quoted

quoted

However, if guest GSO is not supported, 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.

actually how does this waste space? Is this because your device does not
have INDIRECT?

VQ is 256 entries deep.
Driver posted total of 256 descriptors.
Each descriptor points to a page of 4K.
These descriptors are chained as 4K * 16.

So without indirect then? with indirect each descriptor can
point to 16 entries.

So total packets that can be serviced are 256/16 = 16.
So effective queue depth = 16.

So, when GSO is off, for 9K mtu, packet buffer needed = 3 pages. (12k).
So, 13 descriptors (= 13 x 4K =52K) per packet buffer is wasted.

After this improvement, these 13 descriptors are available, increasing the effective queue depth = 256/3 = 85.

[..]

quoted

quoted

quoted

quoted

MTU(Bytes)/Bandwidth (Gbit/s)
              Before   After
   1500        22.5     22.4
   9000        12.8     25.9

is this buffer space?

Above performance numbers are showing improvement in bandwidth. In Gbps/sec.

quoted

just the overhead of allocating/freeing the buffers?
of using INDIRECT?

The effective queue depth is so small, device cannot receive all the packets at given bw-delay product.

quoted

quoted

quoted

Which configurations were tested?

I tested it with DPDK vDPA + qemu vhost. Do you mean the feature set
of the VM?

The configuration of interest is mtu, not the backend.
Which is different mtu as shown in above perf numbers.

quoted

quoted

quoted

Did you test devices without VIRTIO_NET_F_MTU ?

No.  It will need code changes.

No. It doesn't need any code changes. This is misleading/vague.

This patch doesn't have any relation to a device which doesn't offer VIRTIO_NET_F_MTU.
Just the code restructuring is touching this area, that may require some existing tests.
I assume virtio tree will have some automation tests for such a device?

I have some automated tests but I also expect developer to do testing.

quoted

quoted

quoted

quoted

@@ -3853,12 +3866,10 @@ static int virtnet_probe(struct

virtio_device *vdev)

               dev->mtu = mtu;
               dev->max_mtu = mtu;
-
-             /* TODO: size buffers correctly in this case. */
-             if (dev->mtu > ETH_DATA_LEN)
-                     vi->big_packets = true;
       }

+     virtnet_set_big_packets_fields(vi, mtu);
+

If VIRTIO_NET_F_MTU is off, then mtu is uninitialized.
You should move it to within if () above to fix.

mtu was initialized to 0 at the beginning of probe if VIRTIO_NET_F_MTU
is off.

In this case,  big_packets_num_skbfrags will be set according to guest gso.

If guest gso is supported, it will be set to MAX_SKB_FRAGS else
zero---- do you

think this is a bug to be fixed?

yes I think with no mtu this should behave as it did historically.

Michael is right.
It should behave as today. There is no new bug introduced by this patch.
dev->mtu and dev->max_mtu is set only when VIRTIO_NET_F_MTU is offered with/without this patch.

Please have mtu related fix/change in different patch.

quoted

quoted

quoted

quoted

       if (vi->any_header_sg)
               dev->needed_headroom = vi->hdr_len;

--
2.31.1