Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Virtio-net: use large buffers when VIRTIO_NET_F_MRG_RXBUF is not negotiated #1058

Closed
wkozaczuk opened this issue Oct 5, 2019 · 1 comment

Comments

@wkozaczuk
Copy link
Collaborator

Per virtio spec:

"5.1.6.3.1 Driver Requirements: Setting Up Receive Buffers
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.
Otherwise, the driver SHOULD populate the receive queue(s) with buffers of at least 1526 bytes.
If VIRTIO_NET_F_MRG_RXBUF is negotiated, each buffer MUST be at greater than the size of the struct virtio_net_hdr"

if VIRTIO_NET_F_MRG_RXBUF is NOT negotiated AND VIRTIO_NET_F_GUEST_TSO4, VIRTIO_NET_F_GUEST_TSO6 or VIRTIO_NET_F_GUEST_UFO we should be using 17-pages large buffers instead of 1-page large currently.

Lack of proper support of this case causes networking to fail on firecracker (which does not support VIRTIO_NET_F_MRG_RXBUF - see issue firecracker-microvm/firecracker#1314) if, for example, one tries to download large files on OSv.

This approximate patch provides a partial bluepronit of how this issue can be addressed:

diff --git a/drivers/virtio-net.cc b/drivers/virtio-net.cc
index e78fb3af..0df45dce 100644
--- a/drivers/virtio-net.cc
+++ b/drivers/virtio-net.cc
@@ -63,6 +63,7 @@ extern int maxnic;
 namespace virtio {
 
 int net::_instance = 0;
+bool net::use_large_buffer = false;
 
 #define net_tag "virtio-net"
 #define net_d(...)   tprintf_d(net_tag, __VA_ARGS__)
@@ -375,6 +376,9 @@ void net::read_config()
     net_i("Features: %s=%d,%s=%d", "Host TSO ECN", _host_tso_ecn, "CSUM", _csum);
     net_i("Features: %s=%d,%s=%d", "Guest_csum", _guest_csum, "guest tso4", _guest_tso4);
     net_i("Features: %s=%d", "host tso4", _host_tso4);
+
+    printf("VIRTIO_NET_F_MRG_RXBUF: %d\n", _mergeable_bufs);
+    use_large_buffer = !_mergeable_bufs;
 }
 
 /**
@@ -473,7 +477,10 @@ void net::receiver()
             // Bad packet/buffer - discard and continue to the next one
             if (len < _hdr_size + ETHER_HDR_LEN) {
                 rx_drops++;
-                memory::free_page(page);
+                if (use_large_buffer)
+                   free(page);
+                else
+                   memory::free_page(page);
 
                 continue;
             }
@@ -581,7 +588,13 @@ void net::free_buffer_and_refcnt(void* buffer, void* refcnt)
 void net::do_free_buffer(void* buffer)
 {
     buffer = align_down(buffer, page_size);
-    memory::free_page(buffer);
+    if (use_large_buffer) {
+        printf("--> Freeing 17 pages: %p\n", buffer);
+        free(buffer);
+    } else {
+        printf("--> Freeing single page: %p\n", buffer);
+        memory::free_page(buffer);
+    }
 }
 
 void net::fill_rx_ring()
@@ -591,12 +604,23 @@ void net::fill_rx_ring()
     vring* vq = _rxq.vqueue;
 
     while (vq->avail_ring_not_empty()) {
-        auto page = memory::alloc_page();
+        void *page;
+        int pages_num = use_large_buffer ? 17 : 1;
+        if (use_large_buffer) {
+            page = aligned_alloc(memory::page_size, pages_num * memory::page_size);
+            printf("--> Allocated 17 pages: %p\n", page);
+        } else {
+            page = memory::alloc_page();
+            printf("--> Allocated single page: %p\n", page);
+        }
 
         vq->init_sg();
-        vq->add_in_sg(page, memory::page_size);
+        vq->add_in_sg(page, pages_num * memory::page_size);
         if (!vq->add_buf(page)) {
-            memory::free_page(page);
+            if (use_large_buffer)
+               free(page);
+            else
+               memory::free_page(page);
             break;
         }
         added++;
diff --git a/drivers/virtio-net.hh b/drivers/virtio-net.hh
index adc93b39..e6725231 100644
--- a/drivers/virtio-net.hh
+++ b/drivers/virtio-net.hh
@@ -220,6 +220,7 @@ public:
     static void free_buffer_and_refcnt(void* buffer, void* refcnt);
     static void free_buffer(iovec iov) { do_free_buffer(iov.iov_base); }
     static void do_free_buffer(void* buffer);
+    static bool use_large_buffer;
 
     bool ack_irq();
 

Please see relevant discussion here.

@wkozaczuk
Copy link
Collaborator Author

Fixed with 3fc7638

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant