Use `writev` for writing guest outgoing packets to the tap device #2958

bchalios · 2022-04-12T12:48:37Z

Avoid extra memory copy when writing to tap device in virtio-net TX path

Currently, when performing TX, a network frame might be exist in various scattered in various memory regions described by a DescriptorChain that we receive from the guest. In order to write the frame to the tap device we first perform a number of copies from said memory buffers to a single buffer which we then write to the Tap file descriptor.

This PR reverts that, to use scatter-gather IO using the writev system call which avoids the intermediate memory copies.

Fixes #420

Description of Changes

The main blocker for using the writev system call in our device is that we need to be able to inspect the outgoing frame to check if it is destined towards mmds. This requires us to read at least the frame headers which makes the code quite convoluted.

In order to do this in a clean way, I introduce an IoVec struct which is essentially a Vec of std::io::IoSlice. The struct can be created from a Descriptor chain (without performing any copies) and can be directly used to perform the writev system call.

Moreover, it provides methods for reading ranges of bytes from it (which at the moment perform a copy) and this functionality is used to inspect the ranges corresponding to a destination addresses in order to check if the frame is destined for mmds.

This functionality can be added in rust-vmm.

License Acceptance

By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license.

PR Checklist

[Author TODO: Meet these criteria.]
[Reviewer TODO: Verify that these criteria are met. Request changes if not]

All commits in this PR are signed (git commit -s).
The issue which led to this PR has a clear conclusion.
This PR follows the solution outlined in the related issue.
The description of changes is clear and encompassing.
Any required documentation changes (code and docs) are included in this PR.
Any newly added unsafe code is properly documented.
Any API changes follow the Runbook for Firecracker API changes.
Any user-facing changes are mentioned in CHANGELOG.md.
All added/changed functionality is tested.

src/devices/src/virtio/net/iovec.rs

bchalios · 2022-04-19T11:17:11Z

Leaving here for reference some initial performance measurements for the TX path. Baseline, is latest main.
The measurements are percentage improvement comparing to baseline, i.e. (new_version - baseline) / baseline * 100.0).

#VMs	Bandwidth	VMM thread CPU load	kworker CPU load*	Bandwidth per CPU load
1	15.97	-3.70	50.00	19.29
2	13.38	-5.52	0.00	19.35
3	14.14	-2.87	40.00	16.19
4	16.94	-3.40	12.50	20.12
5	13.94	-3.70	0.00	18.08
6	12.41	-4.08	9.09	16.94
7	11.29	-6.16	25.00	17.90
8	7.08	-8.54	23.08	16.49
9	7.31	-10.81	-6.67	20.18
10	1.85	-12.10	6.25	15.59
11	1.56	-13.50	-5.26	17.08
12	0.51	-13.86	23.53	15.76

*kworker load numbers include a lot of noise, since we are measuring the load of all kworker threads on the system.

bchalios · 2022-05-05T13:08:14Z

I also tried modifying the RX path in order to use readv.

This required changing a bit the order in which we do things in the RX path. The order roughly goes like:

We get an event from the tap fd
We try to find an available DescriptorChain from the RX queue
We use readv to read from the tap directly inside the the DescriptorChain
Goto 2 until reading from tap returns EAGAIN (or an error occurs)

The implementation works (here is a prototype), however the performance degrades significantly.

I tried to track down the reason, and I took flamegraphs for executions with the test implementation and comparing it against the version with only vectored-writes enabled (this PR).

The flame graph when using only writev:

and the one when using both writev and readv:

In the second case the majority of the time for receiving one packet (Net::read_from_mmds_or_tap) is consumed by DescriptorChain::checked_new. This is not true in the former case.

This made me realize, that in the original code we do not parse the whole chain if it is not needed. For example if we are receiving an ICMP frame (54 bytes long) we will parse only one descriptor in the chain. With the new workflow we do not know the size of the packet in advance (tap does not provide us fseek or something similar) so we will parse the whole chain when getting the descriptor to write in. These descriptors chains are >= 64K bytes long with our current configuration and consist of many descriptors, which causes the increased time consumed in DescriptorChain::checked_new.

At the moment, I do not see any way to avoid this issue. The only way around this I could think of is trying to ask the tap how many bytes there are available for reading but this functionality doesn't seem to be there.

acatangiu

Awesome work! This is not only more efficient, but I think it also simplifies the emulation code. 💯

leaving a few comments below

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

bchalios · 2022-12-12T09:59:54Z

I used the tool of @zulinx86 to quantify the performance change introduced by this PR on all the platforms we support.

TCP throughput results:

kernel	m5d (Sky lake)	m5d (Cascade lake)	m6i	m6gd
4.14	3.1%	4.4%	7.6%	13.5%
5.10	3.7%	5.2%	7.3%	16.5%

network latency results:

kernel	m5d (Sky lake)	m5d (Cascade lake)	m6i	m6gd
4.14	-9.8%	-1.8%	-0.6%	0.0%
5.10	0.7%	1.0%	0.5%	2.9%

acatangiu

Buy this man a beer! 🍻

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

src/devices/src/virtio/net/iovec.rs

The standard library handles vectored writes and reads using the `IoSlice` and `IoSliceMut` respectively. We introduce a new type, `IoVecBuffer` which essentially is a vector of `IoSlice` and is able to be instantiated from a `DescriptorChain`. `IoVecBuffer` provides us the necessary bits for using `write_vectored` to transmit packets in the tap. Signed-off-by: Babis Chalios <bchalios@amazon.es>

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 <bchalios@amazon.es>

Move the tap mock object from the `Net` device to the `Tap` iteslf and extend said object to allow us to mock as well tap write failures. Also, add a few extra unit-tests in the mmds network stack. Signed-off-by: Babis Chalios <bchalios@amazon.es>

Update baselines to reflect the improvements in the TX path due to vectored writes. Signed-off-by: Babis Chalios <bchalios@amazon.es>

Update baselines to reflect the (small) improvements in the TX path due to vectored writes. Signed-off-by: Babis Chalios <bchalios@amazon.es>

Signed-off-by: Babis Chalios <bchalios@amazon.es>

Added missing entries for 1.3 release - bchalios@'s net scatter-gather improvements (firecracker-microvm#2958) - seccompiler change to make builds reproducible (firecracker-microvm#3445) Signed-off-by: Patrick Roy <roypat@amazon.co.uk>

Added missing entries for 1.3 release - bchalios@'s net scatter-gather improvements (#2958) - seccompiler change to make builds reproducible (#3445) Signed-off-by: Patrick Roy <roypat@amazon.co.uk>

Added missing entries for 1.3 release - bchalios@'s net scatter-gather improvements (firecracker-microvm#2958) - seccompiler change to make builds reproducible (firecracker-microvm#3445) Signed-off-by: Patrick Roy <roypat@amazon.co.uk>

bchalios requested a review from serban300 April 12, 2022 12:48

bchalios added Good first issue Indicates a good issue for first-time contributors Feature: IO Virtualization labels Apr 12, 2022

bchalios marked this pull request as draft April 12, 2022 12:50

bchalios force-pushed the feat_tx_iovec branch from 0c4a745 to 5238c28 Compare April 12, 2022 12:56

bchalios changed the title ~~[RFC] Feat tx iovec~~ [RFC] Use writev for writing guest outgoing packets to the tap device Apr 12, 2022

bchalios self-assigned this Apr 12, 2022

bchalios force-pushed the feat_tx_iovec branch from 5238c28 to 51a1414 Compare April 13, 2022 14:58

serban300 reviewed Apr 14, 2022

View reviewed changes

bchalios force-pushed the feat_tx_iovec branch from 51a1414 to 020f9f1 Compare April 19, 2022 09:16

bchalios force-pushed the feat_tx_iovec branch 4 times, most recently from ecfbb9a to 8bd30c4 Compare April 22, 2022 08:03

bchalios force-pushed the feat_tx_iovec branch from 8bd30c4 to 407c086 Compare May 5, 2022 12:31

bchalios force-pushed the feat_tx_iovec branch from 407c086 to 0acf388 Compare May 9, 2022 07:32

bchalios force-pushed the feat_tx_iovec branch from 0acf388 to 9e102b1 Compare May 18, 2022 09:25

bchalios force-pushed the feat_tx_iovec branch 2 times, most recently from 523d030 to f18b35d Compare May 30, 2022 09:33

acatangiu reviewed May 30, 2022

View reviewed changes

src/devices/src/virtio/net/device.rs Outdated Show resolved Hide resolved

src/devices/src/virtio/net/device.rs Outdated Show resolved Hide resolved

src/devices/src/virtio/net/device.rs Outdated Show resolved Hide resolved

src/devices/src/virtio/net/device.rs Outdated Show resolved Hide resolved

bchalios force-pushed the feat_tx_iovec branch 3 times, most recently from 3fd7c59 to ce1c8a3 Compare June 2, 2022 07:46

bchalios marked this pull request as ready for review June 2, 2022 07:46

bchalios changed the title ~~[RFC] Use writev for writing guest outgoing packets to the tap device~~ Use writev for writing guest outgoing packets to the tap device Jun 2, 2022

bchalios force-pushed the feat_tx_iovec branch 2 times, most recently from 606dc38 to 89828d7 Compare June 2, 2022 14:54

bchalios force-pushed the feat_tx_iovec branch from 6570a7a to 51a9289 Compare December 12, 2022 10:03

acatangiu previously approved these changes Dec 12, 2022

View reviewed changes

bchalios dismissed acatangiu’s stale review via a0b43f2 December 12, 2022 10:19

bchalios force-pushed the feat_tx_iovec branch from 51a9289 to a0b43f2 Compare December 12, 2022 10:19

alxiord previously approved these changes Dec 13, 2022

View reviewed changes

resources/seccomp/aarch64-unknown-linux-musl.json Show resolved Hide resolved

src/devices/src/virtio/net/iovec.rs Show resolved Hide resolved

bchalios dismissed alxiord’s stale review via a78c25b December 27, 2022 10:52

bchalios force-pushed the feat_tx_iovec branch 3 times, most recently from 74d0067 to 3de46ac Compare December 28, 2022 10:10

bchalios added 6 commits December 29, 2022 16:11

net: use writev when writing frames to tap

3aa8dd0

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 <bchalios@amazon.es>

net: mock Tap write failures as well

df650be

Move the tap mock object from the `Net` device to the `Tap` iteslf and extend said object to allow us to mock as well tap write failures. Also, add a few extra unit-tests in the mmds network stack. Signed-off-by: Babis Chalios <bchalios@amazon.es>

net: update throughput baselines

922374b

Update baselines to reflect the improvements in the TX path due to vectored writes. Signed-off-by: Babis Chalios <bchalios@amazon.es>

net: update latency baselines

9d24379

Update baselines to reflect the (small) improvements in the TX path due to vectored writes. Signed-off-by: Babis Chalios <bchalios@amazon.es>

chore: update code coverage

76017e9

Signed-off-by: Babis Chalios <bchalios@amazon.es>

bchalios force-pushed the feat_tx_iovec branch from 3de46ac to 76017e9 Compare December 29, 2022 16:21

dianpopa approved these changes Dec 29, 2022

View reviewed changes

pb8o approved these changes Jan 2, 2023

View reviewed changes

bchalios merged commit d2c52b8 into firecracker-microvm:main Jan 2, 2023

bchalios deleted the feat_tx_iovec branch January 2, 2023 11:43

luminitavoicu mentioned this pull request Jan 4, 2023

perf: net: update AMD baselines for TCP throughput #3350

Closed

2 tasks

ihciah mentioned this pull request Aug 21, 2024

refactor(virtio-net): avoid copy on rx #4742

Closed

9 tasks

bchalios mentioned this pull request Oct 2, 2024

Use readv for the RX path of the network device to avoid one memory copy per frame #4799

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Use `writev` for writing guest outgoing packets to the tap device #2958

Use `writev` for writing guest outgoing packets to the tap device #2958

Uh oh!

bchalios commented Apr 12, 2022 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bchalios commented Apr 19, 2022 •

edited

Loading

Uh oh!

bchalios commented May 5, 2022 •

edited

Loading

Uh oh!

acatangiu left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bchalios commented Dec 12, 2022

Uh oh!

acatangiu left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Use writev for writing guest outgoing packets to the tap device #2958

Use writev for writing guest outgoing packets to the tap device #2958

Uh oh!

Conversation

bchalios commented Apr 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Avoid extra memory copy when writing to tap device in virtio-net TX path

Description of Changes

License Acceptance

PR Checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bchalios commented Apr 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bchalios commented May 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

acatangiu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bchalios commented Dec 12, 2022

Uh oh!

acatangiu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Use `writev` for writing guest outgoing packets to the tap device #2958

Use `writev` for writing guest outgoing packets to the tap device #2958

bchalios commented Apr 12, 2022 •

edited

Loading

bchalios commented Apr 19, 2022 •

edited

Loading

bchalios commented May 5, 2022 •

edited

Loading