net: use writev when writing frames to tap

This allows us to avoid an extra user-space copy of the packet we are
transmitting to the tap. We use directly the buffers described by the
`DescriptorChain` by means of `IoVecBuffer`s.

Signed-off-by: Babis Chalios <[email protected]>

Author: bchalios

resources/seccomp/aarch64-unknown-linux-musl.json

@@ -24,6 +24,10 @@
             {
                 "syscall": "write"
             },
+            {
+                "syscall": "writev",
+                "comment": "Used by the VirtIO net device to write to tap"
+            },
             {
                 "syscall": "fsync"
             },

resources/seccomp/x86_64-unknown-linux-musl.json

@@ -24,6 +24,10 @@
             {
                 "syscall": "write"
             },
+            {
+                "syscall": "writev",
+                "comment": "Used by the VirtIO net device to write to tap"
+            },
             {
                 "syscall": "fsync"
             },

src/devices/src/virtio/net/device.rs

@@ -5,13 +5,14 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the THIRD-PARTY file.
 
+use crate::virtio::net::iovec::IoVecBuffer;
 use crate::virtio::net::tap::Tap;
 #[cfg(test)]
 use crate::virtio::net::test_utils::Mocks;
 use crate::virtio::net::Error;
 use crate::virtio::net::NetQueue;
 use crate::virtio::net::Result;
-use crate::virtio::net::{MAX_BUFFER_SIZE, QUEUE_SIZE, QUEUE_SIZES, RX_INDEX, TX_INDEX};
+use crate::virtio::net::{MAX_BUFFER_SIZE, QUEUE_SIZES, RX_INDEX, TX_INDEX};
 use crate::virtio::DescriptorChain;
 use crate::virtio::{
     ActivateResult, DeviceState, IrqTrigger, IrqType, Queue, VirtioDevice, TYPE_NET,
@@ -39,7 +40,12 @@ use virtio_gen::virtio_net::{
     VIRTIO_NET_F_MAC,
 };
 use virtio_gen::virtio_ring::VIRTIO_RING_F_EVENT_IDX;
-use vm_memory::{ByteValued, Bytes, GuestAddress, GuestMemoryError, GuestMemoryMmap};
+use vm_memory::{ByteValued, Bytes, GuestMemoryError, GuestMemoryMmap};
+
+use dumbo::pdu::arp::ETH_IPV4_FRAME_LEN;
+use dumbo::pdu::ethernet::PAYLOAD_OFFSET;
+
+const FRAME_HEADER_MAX_LEN: usize = PAYLOAD_OFFSET + ETH_IPV4_FRAME_LEN;
 
 enum FrontendError {
     AddUsed,
@@ -53,6 +59,13 @@ pub(crate) fn vnet_hdr_len() -> usize {
     mem::size_of::<virtio_net_hdr_v1>()
 }
 
+// This returns the maximum frame header length. This includes the VNET header plus
+// the maximum L2 frame header bytes which includes the ethernet frame header plus
+// the header IPv4 ARP header which is 28 bytes long.
+fn frame_hdr_len() -> usize {
+    vnet_hdr_len() + FRAME_HEADER_MAX_LEN
+}
+
 // Frames being sent/received through the network device model have a VNET header. This
 // function returns a slice which holds the L2 frame bytes without this header.
 fn frame_bytes_from_buf(buf: &[u8]) -> Result<&[u8]> {
@@ -114,7 +127,6 @@ pub struct Net {
     rx_bytes_read: usize,
     rx_frame_buf: [u8; MAX_BUFFER_SIZE],
 
-    tx_iovec: Vec<(GuestAddress, usize)>,
     tx_frame_buf: [u8; MAX_BUFFER_SIZE],
 
     pub(crate) irq_trigger: IrqTrigger,
@@ -189,7 +201,6 @@ impl Net {
             rx_bytes_read: 0,
             rx_frame_buf: [0u8; MAX_BUFFER_SIZE],
             tx_frame_buf: [0u8; MAX_BUFFER_SIZE],
-            tx_iovec: Vec::with_capacity(QUEUE_SIZE as usize),
             irq_trigger: IrqTrigger::new().map_err(Error::EventFd)?,
             device_state: DeviceState::Inactive,
             activate_evt: EventFd::new(libc::EFD_NONBLOCK).map_err(Error::EventFd)?,
@@ -266,24 +277,31 @@ impl Net {
         Ok(())
     }
 
+    // Helper function to consume one op with `size` bytes from a rate limiter
+    fn rate_limiter_consume_op(rate_limiter: &mut RateLimiter, size: u64) -> bool {
+        if !rate_limiter.consume(1, TokenType::Ops) {
+            return false;
+        }
+
+        if !rate_limiter.consume(size, TokenType::Bytes) {
+            rate_limiter.manual_replenish(1, TokenType::Ops);
+            return false;
+        }
+
+        true
+    }
+
+    // Helper function to replenish one operation with `size` bytes from a rate limiter
+    fn rate_limiter_replenish_op(rate_limiter: &mut RateLimiter, size: u64) {
+        rate_limiter.manual_replenish(1, TokenType::Ops);
+        rate_limiter.manual_replenish(size, TokenType::Bytes);
+    }
+
     // Attempts to copy a single frame into the guest if there is enough
     // rate limiting budget.
     // Returns true on successful frame delivery.
     fn rate_limited_rx_single_frame(&mut self) -> bool {
-        // If limiter.consume() fails it means there is no more TokenType::Ops
-        // budget and rate limiting is in effect.
-        if !self.rx_rate_limiter.consume(1, TokenType::Ops) {
-            METRICS.net.rx_rate_limiter_throttled.inc();
-            return false;
-        }
-        // If limiter.consume() fails it means there is no more TokenType::Bytes
-        // budget and rate limiting is in effect.
-        if !self
-            .rx_rate_limiter
-            .consume(self.rx_bytes_read as u64, TokenType::Bytes)
-        {
-            // revert the OPS consume()
-            self.rx_rate_limiter.manual_replenish(1, TokenType::Ops);
+        if !Self::rate_limiter_consume_op(&mut self.rx_rate_limiter, self.rx_bytes_read as u64) {
             METRICS.net.rx_rate_limiter_throttled.inc();
             return false;
         }
@@ -293,12 +311,10 @@ impl Net {
 
         // Undo the tokens consumption if guest delivery failed.
         if !success {
-            // revert the OPS consume()
-            self.rx_rate_limiter.manual_replenish(1, TokenType::Ops);
-            // revert the BYTES consume()
-            self.rx_rate_limiter
-                .manual_replenish(self.rx_bytes_read as u64, TokenType::Bytes);
+            // revert the rate limiting budget consumption
+            Self::rate_limiter_replenish_op(&mut self.rx_rate_limiter, self.rx_bytes_read as u64);
         }
+
         success
     }
 
@@ -409,24 +425,28 @@ impl Net {
     fn write_to_mmds_or_tap(
         mmds_ns: Option<&mut MmdsNetworkStack>,
         rate_limiter: &mut RateLimiter,
-        frame_buf: &[u8],
+        frame_buf: &mut [u8],
+        frame_iovec: &mut IoVecBuffer,
         tap: &mut Tap,
         guest_mac: Option<MacAddr>,
     ) -> Result<bool> {
-        let checked_frame = |frame_buf| {
-            frame_bytes_from_buf(frame_buf).map_err(|e| {
-                error!("VNET header missing in the TX frame.");
-                METRICS.net.tx_malformed_frames.inc();
-                e
-            })
-        };
+        let frame_header = frame_bytes_from_buf(frame_buf).map_err(|e| {
+            error!("VNET header missing in the TX frame.");
+            METRICS.net.tx_malformed_frames.inc();
+            e
+        })?;
+
         if let Some(ns) = mmds_ns {
-            if ns.detour_frame(checked_frame(frame_buf)?) {
+            if ns.is_mmds_frame(frame_header) {
+                // Here it is ok to unwrap. We have already checked that the IoVecBuffer holds
+                // at least the frame headers and we are reading from the beginning of
+                // the IoVecBuffer.
+                let frame_len = frame_iovec.read_at(frame_buf, 0).unwrap();
+                let _ = ns.detour_frame(&frame_buf[..frame_len]);
                 METRICS.mmds.rx_accepted.inc();
 
                 // MMDS frames are not accounted by the rate limiter.
-                rate_limiter.manual_replenish(frame_buf.len() as u64, TokenType::Bytes);
-                rate_limiter.manual_replenish(1, TokenType::Ops);
+                Self::rate_limiter_replenish_op(rate_limiter, frame_iovec.len() as u64);
 
                 // MMDS consumed the frame.
                 return Ok(true);
@@ -437,16 +457,16 @@ impl Net {
 
         // Check for guest MAC spoofing.
         if let Some(mac) = guest_mac {
-            let _ = EthernetFrame::from_bytes(checked_frame(frame_buf)?).map(|eth_frame| {
+            let _ = EthernetFrame::from_bytes(frame_header).map(|eth_frame| {
                 if mac != eth_frame.src_mac() {
                     METRICS.net.tx_spoofed_mac_count.inc();
                 }
             });
         }
 
-        match tap.write(frame_buf) {
+        match tap.write_vectored(frame_iovec) {
             Ok(_) => {
-                METRICS.net.tx_bytes_count.add(frame_buf.len());
+                METRICS.net.tx_bytes_count.add(frame_iovec.len());
                 METRICS.net.tx_packets_count.inc();
                 METRICS.net.tx_count.inc();
             }
@@ -544,79 +564,29 @@ impl Net {
         let tx_queue = &mut self.queues[TX_INDEX];
 
         while let Some(head) = tx_queue.pop_or_enable_notification(mem) {
-            // If limiter.consume() fails it means there is no more TokenType::Ops
-            // budget and rate limiting is in effect.
-            if !self.tx_rate_limiter.consume(1, TokenType::Ops) {
-                // Stop processing the queue and return this descriptor chain to the
-                // avail ring, for later processing.
-                tx_queue.undo_pop();
-                METRICS.net.tx_rate_limiter_throttled.inc();
-                break;
-            }
-
             let head_index = head.index;
-            let mut read_count = 0;
-            let mut next_desc = Some(head);
-
-            self.tx_iovec.clear();
-            while let Some(desc) = next_desc {
-                if desc.is_write_only() {
-                    self.tx_iovec.clear();
-                    break;
-                }
-                self.tx_iovec.push((desc.addr, desc.len as usize));
-                read_count += desc.len as usize;
-                next_desc = desc.next_descriptor();
-            }
-
-            // If limiter.consume() fails it means there is no more TokenType::Bytes
-            // budget and rate limiting is in effect.
-            if !self
-                .tx_rate_limiter
-                .consume(read_count as u64, TokenType::Bytes)
-            {
-                // revert the OPS consume()
-                self.tx_rate_limiter.manual_replenish(1, TokenType::Ops);
-                // Stop processing the queue and return this descriptor chain to the
-                // avail ring, for later processing.
+            // Parse IoVecBuffer from descriptor head
+            let mut buffer = IoVecBuffer::from_descriptor_chain(mem, head).unwrap();
+            if !Self::rate_limiter_consume_op(&mut self.tx_rate_limiter, buffer.len() as u64) {
                 tx_queue.undo_pop();
                 METRICS.net.tx_rate_limiter_throttled.inc();
                 break;
             }
 
-            read_count = 0;
-            // Copy buffer from across multiple descriptors.
-            // TODO(performance - Issue #420): change this to use `writev()` instead of `write()`
-            // and get rid of the intermediate buffer.
-            for (desc_addr, desc_len) in self.tx_iovec.drain(..) {
-                let limit = cmp::min((read_count + desc_len) as usize, self.tx_frame_buf.len());
-
-                let read_result = mem.read_slice(
-                    &mut self.tx_frame_buf[read_count..limit as usize],
-                    desc_addr,
-                );
-                match read_result {
-                    Ok(()) => {
-                        read_count += limit - read_count;
-                        METRICS.net.tx_count.inc();
-                    }
-                    Err(e) => {
-                        error!("Failed to read slice: {:?}", e);
-                        match e {
-                            GuestMemoryError::PartialBuffer { .. } => &METRICS.net.tx_partial_reads,
-                            _ => &METRICS.net.tx_fails,
-                        }
-                        .inc();
-                        read_count = 0;
-                        break;
-                    }
-                }
-            }
+            // Read the frame headers from the IoVecBuffer
+            buffer
+                .read_at(&mut self.tx_frame_buf[..frame_hdr_len()], 0)
+                .ok_or_else(|| {
+                    error!("Frame headers missing in the TX frame.");
+                    METRICS.net.tx_malformed_frames.inc();
+                    DeviceError::MalformedPayload
+                })?;
 
             let frame_consumed_by_mmds = Self::write_to_mmds_or_tap(
                 self.mmds_ns.as_mut(),
                 &mut self.tx_rate_limiter,
-                &self.tx_frame_buf[..read_count],
+                &mut self.tx_frame_buf,
+                &mut buffer,
                 &mut self.tap,
                 self.guest_mac,
             )
@@ -905,13 +875,13 @@ pub mod tests {
 
     #[test]
     fn test_vnet_helpers() {
-        let mut frame_buf = vec![42u8; vnet_hdr_len() - 1];
+        let frame_buf = vec![42u8; vnet_hdr_len() - 1];
         assert_eq!(
             format!("{:?}", frame_bytes_from_buf(&frame_buf)),
             "Err(VnetHeaderMissing)"
         );
         assert_eq!(
-            format!("{:?}", frame_bytes_from_buf_mut(&mut frame_buf)),
+            format!("{:?}", frame_bytes_from_buf_mut(&mut frame_buf.clone())),
             "Err(VnetHeaderMissing)"
         );
 
@@ -1456,6 +1426,10 @@ pub mod tests {
         let dst_ip = Ipv4Addr::new(169, 254, 169, 254);
 
         let (frame_buf, frame_len) = create_arp_request(src_mac, src_ip, dst_mac, dst_ip);
+        let mut buffer = bufferBuffer::from(&frame_buf[..frame_len]);
+
+        let mut headers = vec![0; frame_hdr_len()];
+        buffer.read_at(&mut headers, 0).unwrap();
 
         // Call the code which sends the packet to the host or MMDS.
         // Validate the frame was consumed by MMDS and that the metrics reflect that.
@@ -1465,7 +1439,8 @@ pub mod tests {
             assert!(Net::write_to_mmds_or_tap(
                 net.mmds_ns.as_mut(),
                 &mut net.tx_rate_limiter,
-                &frame_buf[..frame_len],
+                &mut headers,
+                &mut buffer,
                 &mut net.tap,
                 Some(src_mac),
             )
@@ -1491,6 +1466,9 @@ pub mod tests {
         let dst_ip = Ipv4Addr::new(10, 1, 1, 1);
 
         let (frame_buf, frame_len) = create_arp_request(guest_mac, guest_ip, dst_mac, dst_ip);
+        let mut buffer = bufferBuffer::from(&frame_buf[..frame_len]);
+        let mut headers = vec![0; frame_hdr_len()];
+        buffer.read_at(&mut headers, 0).unwrap();
 
         // Check that a legit MAC doesn't affect the spoofed MAC metric.
         check_metric_after_block!(
@@ -1499,7 +1477,8 @@ pub mod tests {
             Net::write_to_mmds_or_tap(
                 net.mmds_ns.as_mut(),
                 &mut net.tx_rate_limiter,
-                &frame_buf[..frame_len],
+                &mut headers,
+                &mut buffer,
                 &mut net.tap,
                 Some(guest_mac),
             )
@@ -1512,7 +1491,8 @@ pub mod tests {
             Net::write_to_mmds_or_tap(
                 net.mmds_ns.as_mut(),
                 &mut net.tx_rate_limiter,
-                &frame_buf[..frame_len],
+                &mut headers,
+                &mut buffer,
                 &mut net.tap,
                 Some(not_guest_mac),
             )

src/devices/src/virtio/net/tap.rs

@@ -7,7 +7,7 @@
 
 use net_gen::ifreq;
 use std::fs::File;
-use std::io::{Error as IoError, Read, Result as IoResult, Write};
+use std::io::{Error as IoError, IoSlice, Read, Result as IoResult, Write};
 use std::os::raw::*;
 use std::os::unix::io::{AsRawFd, FromRawFd, RawFd};
 use utils::ioctl::{ioctl_with_mut_ref, ioctl_with_ref, ioctl_with_val};
@@ -176,6 +176,10 @@ impl Write for Tap {
         self.tap_file.write(&buf)
     }
 
+    fn write_vectored(&mut self, bufs: &[IoSlice<'_>]) -> IoResult<usize> {
+        self.tap_file.write_vectored(bufs)
+    }
+
     fn flush(&mut self) -> IoResult<()> {
         Ok(())
     }