Hardware checksum offloading with partially computed TCP and UDP checksums #12
Conversation
There was a problem hiding this comment.
Hey @skuenzer,
I left you some initial comments. I will come back to this once some of the PR dependencies are resolved.
Cezar
| IF__NETIF_CHECKSUM_ENABLED(nf, NETIF_CHECKSUM_CHECK_IP) { | ||
| uk_pr_info(" IP"); | ||
| } |
There was a problem hiding this comment.
This macro looks okay, but, wouldn't it be better if there was another macro that included the print too?
PRINT_IF__NETIF_CHECKSUM_ENABLED(nf, NETIF_CHECKSUM_CHECK_IP, " IP");
PRINT_IF__NETIF_CHECKSUM_ENABLED(nf, NETIF_CHECKSUM_CHECK_UDP, " UDP");
...It would save a lot of lines as this is repeated a lot below 🙂
There was a problem hiding this comment.
This is a lwip provided macro that I am using here. I find it counter-intuitive to add another macro on top for the printing.
| @@ -317,6 +317,19 @@ void sys_free(void *ptr); | |||
| #define CHECKSUM_CHECK_ICMP6 1 | |||
| #define CHECKSUM_CHECK_TCP 1 | |||
|
|
|||
| /* | |||
| * As long as partial checksummin gis not upstream available, | |||
There was a problem hiding this comment.
checksummin gis -> checksumming is
| IF__NETIF_CHECKSUM_ENABLED(nf, NETIF_CHECKSUM_PARTIAL_UDP) { | ||
| uk_pr_info("[+partial]"); | ||
| } |
There was a problem hiding this comment.
Same suggestion as before/above. This is repeated a lot and I think would sit better inside a macro.
Also, this can be a different define and it can be empty if partial checksumming is disabled:
#if LWIP_CHECKSUM_PARTIAL
#define PRINT_PART_IF__NETIF_CHECKSUM_ENABLED(INTF, NETIF_CHECKSUM_FLAG, MSG) \
IF__NETIF_CHECKSUM_ENABLED(INTF, NETIF_CHECKSUM_FLAG) \
uk_pr_info(MSG)
#else
#define PRINT_PART_IF__NETIF_CHECKSUM_ENABLED(INTF, NETIF_CHECKSUM_FLAG, MSG) \
do { } while (0)
#endif /* LWIP_CHECKSUM_PARTIAL */Something like this, to have fewer #if checks. (you are free to change the suggestion in any way, of course 😉)
skuenzer
force-pushed
the
skuenzer/partial-checksum
branch
2 times, most recently
from
2e60853 to
4480eed
Compare
Latest lib/uknetdev updates make it necessary that new network interfaces need to be probed for feature negotiation before the interface can be used. Signed-off-by: Simon Kuenzer <[email protected]>
During network device initialization, print the hardware address of the network device to the info kernel console. This should simplify first-level of diagnosis. Signed-off-by: Simon Kuenzer <[email protected]>
During network device initialization, print the checksum handling configuration that get set by the underlying uknetdev driver. This should simplify first-level of diagnosis. Signed-off-by: Simon Kuenzer <[email protected]>
The netdev device features were refactored in `lib/uknetdev`. This commit adopts the usage for initializing interrupt and polling mode for a device: `UK_FEATURE_RXQ_INTR_AVAILABLE` is now called `UK_NETDEV_F_RXQ_INTR`. Signed-off-by: Simon Kuenzer <[email protected]>
Like Linux, we enable checking the checksum of the IP header on packet reception and let lwIP drop the packet if it was invalid. Signed-off-by: Simon Kuenzer <[email protected]>
Add more details why we need to copy a packet for transmission. We support asynchronous transmit but not with zero-copy. Problem is that lwIP may modify a packet header for preparing retransmissions while the NIC is still accessing the packet buffer for transmission. Signed-off-by: Simon Kuenzer <[email protected]>
skuenzer
force-pushed
the
skuenzer/partial-checksum
branch
from
4480eed to
4058185
Compare
This commit implements and enables checksum offloading with the Unikraft fork of lwIP and if the underlying uknetdev device announced support for it. For non-supported devices, all checksums are checked and computed. When an lwIP stack version is used that currently does not support checksum offloading, the original behavior is kept. Signed-off-by: Simon Kuenzer <[email protected]>
During netif initialization, the handling of checksums is
printed per device with the info level. This commits adds
the following properties next to the protocols:
- For received traffic
- `[+partial]` Driver and device additionally support
receiving of partial checksummed packets.
- `[+offloaded]` Driver and device additionally skip checksum
validation of already validated packets,
e.g., by a physical NIC on the host.
- For transmitted traffic
- `[partial]` Generated packets will only have a partial
checksum computed in software. The rest of
the checksum is intended to be computed by
a physical NIC on the host.
Signed-off-by: Simon Kuenzer <[email protected]>
Whenever we need to wait for the device/driver to be ready for sending out another packet, we enter a busy-wait loop. With this commit, we call `uk_sched_yield()` while we wait so that other threads in the system can be executed. Signed-off-by: Simon Kuenzer <[email protected]>
skuenzer
force-pushed
the
skuenzer/partial-checksum
branch
from
4058185 to
7d33417
Compare
|
Hi! Can we cherry pick f9e7d14 out of this PR and place it into a new PR? The current stable (v0.8) of Unikraft relies on this change and results in breaking builds with the current version of LwIP. |
|
Actually, the first 3 are in now, so this PR can be rebased |
|
Hmm. I was sure that it built for me, when I tested just putting in the PRs that are now accepted Lets cherry pick the change because I have problems with this patch. |
|
@marcrittinghaus , is there still an issue with curl and this PR? I moved it to 0.11. Is that reasonable? Or should we move it to 0.12? |
This PR enables transmitting and receiving of packets with a partially computed checksum for
lib/uknetdevdevices that support packets with partial checksums. Additionally, this commit activates the ability to skip checking of the checksum for already validated packets, e.g., by physical NIC on the host.This PR bridges the gap between the lwIP implementation with PR #1 and the Unikraft uknetdev implementation with PR #308. Additonally, this PR depends on PR #10. All three PRs need to be merged before this PR can be applied.