drivers/virtio: Implement LRO and RX buffer merging #1339

mschlumpp · 2024-02-26T13:13:06Z

Prerequisite checklist

Read the contribution guidelines regarding submitting new changes to the project;
Tested your changes against relevant architectures and platforms;
Ran the checkpatch.uk on your commit series before opening this PR;
Updated relevant documentation.

Base target

Architecture(s): [e.g. x86_64 or N/A]
Platform(s): [e.g. kvm, xen or N/A]
Application(s): [e.g. app-python3 or N/A]

Additional configuration

To test this PR you can use the corresponding LWIP PR: unikraft/lib-lwip#53
When receiving large enough payloads via TCP, QEMU/Linux should then pass on packets size much larger than the configured MTU to the VM.

Description of changes

This PR adds LRO support to the virtio net driver and the necessary abstraction in uknetdev. To avoid having to allocate huge buffers it also implements support for the RX buffer merge feature. This feature allows the device to join multiple receive buffers together for packets larger than the buffer size.
Because this feature triggered a bug in the event notification code, it also includes a fix for that problem.

mogasergiu

Hey @mschlumpp 🌵 👋 ! Thank you for the super useful feature 💪! I had quick diagonal look trying to spot what I would describe as nitpicks. I will try to have a deeper look in the next round.

mogasergiu · 2024-03-18T08:28:11Z

drivers/virtio/net/virtio_net.c

 	struct virtio_net_device *vndev;
+	__u64 drv_features = 0;
+	__u64 host_features;
+	int rc = 0;


Hmm, I see in case of success you return 0; regardless. So this early initialization is probably not needed.

mogasergiu · 2024-03-18T08:29:08Z

drivers/virtio/net/virtio_net.c

+{
+	struct virtio_net_device *vndev;
+	__u64 host_features;
+	int rc = 0;


Ditto? rc does not seem to be set anywhere.

drivers/virtio/net/virtio_net.c

mogasergiu · 2024-03-18T08:38:28Z

drivers/virtio/net/virtio_net.c

+	 *       burden on the allocator).
+	 */
+	if (conf->lro) {
+		if (VIRTIO_FEATURE_HAS(vndev->vdev->features,


Would it be worth adding a likely here? 🤔

Actually let's also reorder the whole section ^^
Then we can remove one level of indentation on the success case.

mariapana

Apart from @mogasergiu 's remarks, it looks good. Moreover, I haven't found any problems during testing. Seems to work as expected 👍

Reviewed-by: Maria Pana maria.pana4@gmail.com

Krechals

Greetings, Marco! Thank you for adding this feature! I only managed to go over it briefly, but I'll look into it more thoroughly in the coming days.

I expect this feature will boost network throughput by a bit. Though I haven't had a chance to test this yet, have you noticed any improvements?

Krechals · 2024-03-31T23:47:22Z

drivers/virtio/net/virtio_net.c

@@ -322,9 +328,9 @@ static int virtio_netdev_rx_fillup(struct virtio_net_device *vndev,
 	 * Because we using 2 descriptor for a single netbuf, our effective


We should update the comment from above.

Krechals · 2024-04-01T00:43:34Z

drivers/virtio/net/virtio_net.c

@@ -322,9 +328,9 @@ static int virtio_netdev_rx_fillup(struct virtio_net_device *vndev,
 	 * Because we using 2 descriptor for a single netbuf, our effective
 	 * queue size is just the half.
 	 */
-	nb_desc = ALIGN_DOWN(nb_desc, 2);
+	nb_desc = ALIGN_DOWN(nb_desc, vndev->buf_descr_count);


Is it guaranteed that vndev->buf_descr_count is a power of 2?

For now yes, but I'll insert an assert in front of it.

Krechals · 2024-04-01T00:51:11Z

drivers/virtio/net/virtio_net.c

-	/* Appending the header buffer to the sglist */
-	uk_sglist_append(sg, rxhdr, virtio_net_hdr_size(vndev));
+	if (vndev->buf_descr_count == VIRTIO_NET_BUF_DESCR_COUNT_INLINE) {
+		rc = uk_netbuf_header(netbuf, virtio_net_hdr_size(vndev));


Could you please help me understand what this branch covers?
I'm a bit lost in understanding the use case and why we are not using the padding from VTNET_HDR_SIZE_PADDED.

Modern virtio devices (or legacy devices with negotiated ANY_LAYOUT, but we don't consider those) do not need to have the header in a separate descriptor. And if we put header and payload into the same descriptor then there must not be any padding between them.
Additionally, if we use mergeable RX buffers the resulting buffers would contain non-continuous payloads for the additional buffers (fixing that up would require memmoves)
Also this usually wastes one less descriptor.

This also allows us to properly keep track of the index we last notified the host of. This is necessary to properly check against the hosts' notification index. Signed-off-by: Marco Schlumpp <marco@unikraft.io>

This allows network stacks to query the LRO support and enable it if supported by the stack. Signed-off-by: Marco Schlumpp <marco@unikraft.io>

mschlumpp · 2024-04-29T07:47:22Z

Updated and rebased the PR

mogasergiu

Hey Marco, had another look 👋

drivers/virtio/net/virtio_net.c

mogasergiu · 2024-05-13T14:01:17Z

drivers/virtio/net/virtio_net.c

+		buf->len = len + (VTNET_HDR_SIZE_PADDED(vndev) -
+			   virtio_net_hdr_size(vndev));
+		/* Shift the position forward to remove the header */
+		rc = uk_netbuf_header(


paranthesis alignment: buf on this line and the second argument on the next.

mogasergiu · 2024-05-13T14:01:30Z

drivers/virtio/net/virtio_net.c

-	rc = uk_netbuf_header(buf, -((__s16)VTNET_HDR_SIZE_PADDED(vndev)));
+	if (vndev->buf_descr_count == VIRTIO_NET_BUF_DESCR_COUNT_INLINE) {
+		buf->len = len;
+		rc = uk_netbuf_header(


mogasergiu · 2024-05-13T14:02:36Z

drivers/virtio/net/virtio_net.c

@@ -1007,6 +1043,16 @@ static int virtio_netdev_probe(struct uk_netdev *n)
 	 */
 	vndev->vdev->features = drv_features;

+	if (vndev->vdev->features & (1ULL << VIRTIO_F_VERSION_1)


I would place paranthesis around both these bitwise &'s just to be double sure 🤷‍♂️.

mogasergiu · 2024-05-13T14:03:51Z

drivers/virtio/net/virtio_net.c

+
+	while (num_buffers > 1) {
+		ret = virtqueue_buffer_dequeue(rxq->vq, (void **) &chain, &len);
+		if (ret < 0) {


unlikely? 🤔

mogasergiu · 2024-05-13T14:08:41Z

drivers/virtio/net/virtio_net.c

+	 */
+	if (conf->lro) {
+		if (unlikely(
+			!VIRTIO_FEATURE_HAS(vndev->vdev->features,


mogasergiu · 2024-05-13T14:09:33Z

drivers/virtio/net/virtio_net.c

+		}
+		if (VIRTIO_FEATURE_HAS(host_features, VIRTIO_NET_F_GUEST_TSO4))
+			VIRTIO_FEATURE_SET(vndev->vdev->features,
+				   VIRTIO_NET_F_GUEST_TSO4);


Paranthesis alignment?

mogasergiu · 2024-05-13T14:09:38Z

drivers/virtio/net/virtio_net.c

+				   VIRTIO_NET_F_GUEST_TSO4);
+		if (VIRTIO_FEATURE_HAS(host_features, VIRTIO_NET_F_GUEST_TSO6))
+			VIRTIO_FEATURE_SET(vndev->vdev->features,
+				   VIRTIO_NET_F_GUEST_TSO6);


mogasergiu · 2024-05-13T14:12:08Z

drivers/virtio/net/virtio_net.c

+	 *       descriptors (needing indirect descriptors and putting a large
+	 *       burden on the allocator).
+	 */
+	if (conf->lro) {


Assert for conf?

drivers/virtio/net/virtio_net.c

This allows delaying the announcement of driver features to the configure step. Signed-off-by: Marco Schlumpp <marco@unikraft.io>

For modern virtio devices the header can be put into the same descriptor as the data. This also now correctly removes the padded header from the packets. The previous code added junk data at the end because it did not consider the header part of the reported data length. Signed-off-by: Marco Schlumpp <marco@unikraft.io>

This allows the device to merge smaller buffers into a larger one when a single buffer is not large enough for a particular packet. Signed-off-by: Marco Schlumpp <marco@unikraft.io>

Expose device support over uknetdev and allow larger packets when one of the LRO features is enabled. Signed-off-by: Marco Schlumpp <marco@unikraft.io>

mschlumpp requested review from a team as code owners February 26, 2024 13:13

github-actions bot added area/lib Internal Unikraft Microlibrary lang/c Issues or PRs to do with C/C++ lib/uknetdev labels Feb 26, 2024

mschlumpp mentioned this pull request Feb 26, 2024

Implement support for LRO unikraft/lib-lwip#53

Open

mschlumpp force-pushed the mschlumpp/feature/virtio-mrg-rxbuf branch 2 times, most recently from 3e61377 to 01bc03c Compare February 26, 2024 13:33

mschlumpp force-pushed the mschlumpp/feature/virtio-mrg-rxbuf branch from 01bc03c to e26cf57 Compare March 5, 2024 15:49

razvand assigned skuenzer Mar 17, 2024

razvand added this to the v0.17.0 (Calypso) milestone Mar 17, 2024

razvand requested review from Krechals, mogasergiu and mariapana and removed request for a team March 17, 2024 18:17

mogasergiu requested changes Mar 18, 2024

View reviewed changes

mschlumpp force-pushed the mschlumpp/feature/virtio-mrg-rxbuf branch 2 times, most recently from 322c5dc to b749bcf Compare March 18, 2024 13:25

mschlumpp requested a review from mogasergiu March 18, 2024 13:32

mschlumpp force-pushed the mschlumpp/feature/virtio-mrg-rxbuf branch from b749bcf to 67fbea9 Compare March 18, 2024 13:44

mariapana approved these changes Mar 30, 2024

View reviewed changes

Krechals reviewed Apr 1, 2024

View reviewed changes

razvand force-pushed the staging branch from 31975b8 to ba192e4 Compare April 2, 2024 11:41

mschlumpp added 2 commits April 29, 2024 09:28

drivers/virtio: Keep event suppression mechanism internal

72dfa74

This also allows us to properly keep track of the index we last notified the host of. This is necessary to properly check against the hosts' notification index. Signed-off-by: Marco Schlumpp <marco@unikraft.io>

lib/uknetdev: Add interface to configure LRO enablement

41c34f2

This allows network stacks to query the LRO support and enable it if supported by the stack. Signed-off-by: Marco Schlumpp <marco@unikraft.io>

mschlumpp force-pushed the mschlumpp/feature/virtio-mrg-rxbuf branch 2 times, most recently from c85be7d to 5e7f311 Compare April 29, 2024 07:46

mogasergiu requested changes May 13, 2024

View reviewed changes

mschlumpp added 3 commits May 22, 2024 14:26

drivers/virtio/net: Split feature probing and negotiation

3b65d37

This allows delaying the announcement of driver features to the configure step. Signed-off-by: Marco Schlumpp <marco@unikraft.io>

drivers/virtio/net: Add support for mergeable RX buffers

8a98a24

This allows the device to merge smaller buffers into a larger one when a single buffer is not large enough for a particular packet. Signed-off-by: Marco Schlumpp <marco@unikraft.io>

mschlumpp force-pushed the mschlumpp/feature/virtio-mrg-rxbuf branch from 5e7f311 to e71299a Compare May 22, 2024 12:26

drivers/virtio/net: Implement support for LRO

b0313e4

Expose device support over uknetdev and allow larger packets when one of the LRO features is enabled. Signed-off-by: Marco Schlumpp <marco@unikraft.io>

mschlumpp force-pushed the mschlumpp/feature/virtio-mrg-rxbuf branch from e71299a to b0313e4 Compare May 22, 2024 12:51

razvand requested review from mogasergiu and Krechals May 24, 2024 06:14

razvand modified the milestones: v0.17.0 (Calypso), v0.18.0 (Helene) May 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

drivers/virtio: Implement LRO and RX buffer merging #1339

drivers/virtio: Implement LRO and RX buffer merging #1339

mschlumpp commented Feb 26, 2024 •

edited

Loading

mogasergiu left a comment

mogasergiu Mar 18, 2024

mogasergiu Mar 18, 2024

mogasergiu Mar 18, 2024

mschlumpp Mar 18, 2024 •

edited

Loading

mariapana left a comment

Krechals left a comment

Krechals Mar 31, 2024

Krechals Apr 1, 2024

mschlumpp Apr 2, 2024

Krechals Apr 1, 2024

mschlumpp Apr 2, 2024

mschlumpp commented Apr 29, 2024

mogasergiu left a comment

mogasergiu May 13, 2024

mogasergiu May 13, 2024

mogasergiu May 13, 2024

mogasergiu May 13, 2024

mogasergiu May 13, 2024

mogasergiu May 13, 2024

mogasergiu May 13, 2024

mogasergiu May 13, 2024

		@@ -322,9 +328,9 @@ static int virtio_netdev_rx_fillup(struct virtio_net_device *vndev,
		* Because we using 2 descriptor for a single netbuf, our effective

drivers/virtio: Implement LRO and RX buffer merging #1339

Are you sure you want to change the base?

drivers/virtio: Implement LRO and RX buffer merging #1339

Conversation

mschlumpp commented Feb 26, 2024 • edited Loading

Prerequisite checklist

Base target

Additional configuration

Description of changes

mogasergiu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mschlumpp Mar 18, 2024 • edited Loading

Choose a reason for hiding this comment

mariapana left a comment

Choose a reason for hiding this comment

Krechals left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mschlumpp commented Apr 29, 2024

mogasergiu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mschlumpp commented Feb 26, 2024 •

edited

Loading

mschlumpp Mar 18, 2024 •

edited

Loading