Optimize IP version detection during address handle creation

[swjz]

Overview

Currently in user space, ibv_init_ah_from_wc() relies on parsing the Global Routing Header (GRH) payload to determine whether an incoming Unreliable Datagram (UD) packet is IPv4 or IPv6. This mechanism relies on get_grh_header_version(), which introduces two major issues:

1. Correctness: Since some hardware zeroes out the IPv4 header checksum in the receive buffer, the software recalculated checksum fails the comparison against the buffer's zeroed field. This causes valid IPv4 packets to be indeterministically misclassified as IPv6.
2. Performance: It involves an extra 20-byte stack memcpy and a checksum calculation (ipv4_calc_hdr_csum()) on the fast path.

Many modern hardware providers already parse the IP version directly in the hardware Completion Queue Entry (CQE). This PR introduces a mechanism to pass this hardware-extracted network header type up to the core library, bypassing the software fallback.

Changes Introduced

• libibverbs:

  • Introduces IBV_WC_WITH_NETWORK_HDR_TYPE and IBV_WC_NETWORK_HDR_IPV6 flags to enum ibv_wc_flags.
  • Updates ibv_init_ah_from_wc() to evaluate this bitmask first, immediately determining the IP version without touching payload memory or executing the software checksum fallback.

• irdma (Provider):

  • Implements the new flags in irdma_process_cqe() for UD queue pairs.
  • Translates the natively hardware-reported cur_cqe->ipv4 state directly into the generic wc_flags bitmask, ensuring accurate IP version reporting regardless of buffer checksum state.

Impact

• Correctness: Fixes a bug where hardware-zeroed checksums cause IPv4 packets to be misclassified as IPv6. Non-supporting providers safely fall back to the existing GRH parsing logic.
• Performance: Accelerates address handle creation for incoming UD packets on supporting hardware by eliminating redundant checksum calculation.

[rleon]

And here again you are accessing packet header. It looks like that "performance claim" in PR description is not accurate.

[swjz]

Thanks for the catch! I've updated the PR description to reflect the change more accurately.

That said, the issue of a zeroed IPv4 checksum leading to IP version misidentification still stands. It is much safer to rely on explicit information rather than a heuristic. Bypassing get_grh_header_version() also still saves us the memcpy and the checksum computation cycles on the fast path.

[rleon]

This is user API enum. You should find a way to solve intel HW bug without exposing it to the users.

[swjz]

The challenge is that we cannot safely fix the GRH payload inside the provider:

• At poll time (irdma_process_cqe): Even if the provider retrieves the buffer address, it cannot assume the memory is actually CPU-accessible.
• At AH creation (ibv_init_ah_from_wc): The application provides a valid, CPU-accessible grh pointer, but by then we are in the core library. The hardware has already zeroed the checksum during Rx offload, causing the software heuristic to fail.

Since modifying the payload in poll_cq isn't viable, and extending wc_flags is undesirable, may I ask for your advice on how to best resolve this issue, assuming we cannot fix the zero checksum in HW?

[rleon]

I thought that if you know what HW has limitation, you can internally store some mark in the irdma_ucontext/irdma_device and perform right flow based on that mark. If you need some help from the libibverbs, it is fine, we can do it. I just don't want to see this behavior is leaked to the applications.

Your PR is actually doing that conceptually, but implementation needs to be changed.

Thanks

[jgunthorpe]

I think we should call this what it is, you have a device that does not conform to the spec (sorry, modifying the UDP header before delivering it is non-compliant) and you want to figure out how to accommodate it.

It is very hard to see how to do this without exposing it to the application since the API is no longer being followed anymore. Everything that inspects the UDP header data is going to require some kind of fixing.

Carving off a few bits from the ibv_wc to indicate what is stored in the GRH portion of the UD payload seems like a reasonable general concept but I'm also not enthused about user visible API here that only one driver can implement. We have to live with that forever

[rleon]

Carving off a few bits from the ibv_wc to indicate what is stored in the GRH portion of the UD payload seems like a reasonable general concept but I'm also not enthused about user visible API here that only one driver can implement. We have to live with that forever

Given the fact that it is presented as a bug.

[jakemoroni]

Yeah, it's not great. The checksum is incorrectly zeroed by HW for IPv4 GRHs. Fortunately, it hasn't been much of a problem aside from this routine. I guess many applications do not explicitly look at this checksum and just implicitly rely on the GRH to create their response AHs.

that only one driver can implement

I think many could implement it, but this is only based on a quick search of the kernel for drivers which currently set IB_WC_WITH_NETWORK_HDR_TYPE for completions, which is more or less the same thing (except there's a dedicated network_hdr_type field in the kernel completion).

If we want to avoid consuming WC flag bits, another idea may be to add a context specific ibv_init_ah_from_wc() method so that irdma can provide its own implementation and fall into the normal ibv_init_ah_from_wc() if no provider method is set. However, even if we had a provider specific method, we still need a way to smuggle the IP version information from the irdma HW CQE into the ibv_init_ah_from_wc() call. Maybe we could put something into wc->vendor_err since this is undefined for successful completions, but it doesn't feel right.

One advantage of the provider method is that by that point, we have a CPU-accessible GRH pointer so we could perform other fixups if necessary.

[jgunthorpe]

I feel a lot better about this if the proposal is "Implement IB_WC_WITH_NETWORK_HDR_TYPE from the kernel in user space that is already supported by 7 drivers". Find a smart way to encode that in the existing WC, task an AI to try to copy the kernel driver implementations to rdma-core..

[10000TB]

Adding into common API, just to fix for one driver / provider is understandably controversial. It looks like PR author considered a few irdma specific alternatives (such as filling the missing checksum) before settling on current approach. While there is performance gain, that isn't the root of the push back.

(1) Adding a generic first class feature for all providers, which irdma could then utilize, seems like a reasonable path forward.
(2) Alternatively, if there is timeline crunch, perhaps vendor specific fields within cqe (vendor_err, or imm_data) can be creatively used to pass along the hint (?)