On Thu, Nov 25, 2021 at 03:26:22PM +0800, Jason Wang wrote:

On Thu, Nov 25, 2021 at 3:20 PM Eli Cohen [off-list ref] wrote:

quoted

On Thu, Nov 25, 2021 at 03:15:33PM +0800, Jason Wang wrote:

quoted

On Thu, Nov 25, 2021 at 3:09 PM Eli Cohen [off-list ref] wrote:

quoted

On Thu, Nov 25, 2021 at 02:05:47PM +0800, Jason Wang wrote:

quoted

When VIRTIO_NET_F_MTU feature is not negotiated, we assume a very
large max_mtu. In this case, using small packet mode is not correct
since it may breaks the networking when MTU is grater than
ETH_DATA_LEN.

To have a quick fix, simply enable the big packet mode when
VIRTIO_NET_F_MTU is not negotiated. We can do optimization on top.

Reported-by: Eli Cohen <redacted>
Cc: Eli Cohen <redacted>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 drivers/net/virtio_net.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 7c43bfc1ce44..83ae3ef5eb11 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c

@@ -3200,11 +3200,12 @@ 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;
      }

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

If VIRTIO_NET_F_MTU is provided, then dev->max_mtu is going to equal
ETH_DATA_LEN (will be set in ether_setup()) so I don't think it will set
big_packets to true.

I may miss something, the dev->max_mtu is just assigned to the mtu
value read from the config space in the code block above  (inside the
feature check of VIRTIO_NET_F_MTU).

Sorry, I meant "If VIRTIO_NET_F_MTU is ***NOT*** provided". In that case
dev->max_mtu eauals ETH_DATA_LEN so you won't set vi->big_packets to
true.

I see but in this case, the above assignment:

        /* MTU range: 68 - 65535 */
        dev->min_mtu = MIN_MTU;
        dev->max_mtu = MAX_MTU;

happens after alloc_etherdev_mq() which calls ether_setup(), so we are
probably fine here.

Thanks

Actually the issue with VIRTIO_NET_F_MTU is that devices might be
tempted to expose 9k here simply because they support jumbo frames,
if they also don't support mergeable buffers this will force big
packet mode.

quoted

quoted

Thanks

quoted

quoted

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

--
2.25.1