Page MenuHomeFreeBSD

vtnet: Better adjust for ethernet alignment.
Needs ReviewPublic

Authored by imp on Dec 21 2023, 9:27 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, May 9, 3:12 PM
Unknown Object (File)
Thu, May 9, 6:52 AM
Unknown Object (File)
Sun, May 5, 4:40 PM
Unknown Object (File)
May 2 2024, 10:16 AM
Unknown Object (File)
Apr 26 2024, 2:45 AM
Unknown Object (File)
Apr 25 2024, 10:38 PM
Unknown Object (File)
Mar 12 2024, 4:49 PM
Unknown Object (File)
Jan 28 2024, 9:18 AM

Details

Reviewers
markj
Summary

Move adjustment of the mbuf from where we allocate it to where we are
about to queue it to the device. Do this only on those platforms that
require it. This allows us to receive an entire jumbo frame on other
platforms. It also doesn't make the adjustment on subsequent frames when
we queue mulitple mbufs for LRO operations.

For the normal use case on armv7, there's no difference because we only
ever allocate one mbuf. However, for the LRO cases it increases what's
available in LRO. It also ensure that we get enough mbufs in those cases
as well (though I have no ability to test this on a LRO scenario with
armv7).

This has the side effect of reverting 527b62e37e68.

Fixes: 527b62e37e68
Sponsored by: Netflix

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 55076
Build 51965: arc lint + arc unit

Event Timeline

imp requested review of this revision.Dec 21 2023, 9:27 PM
imp created this revision.

I'd like to acknowledge that this just fixes the driver in the same manner other drivers are fixed, but to fully fix it requires packing changes in a lot of places, which introduce performance regressions. This fixes it for the typical case, but maybe not for the VLAN case, etc. Given there's not a complete, agreed upon solution to the general problem, this at least stops the insta-panic on boot in a way that doesn't create other issues.

ronald_klop.ws added inline comments.
sys/dev/virtio/network/if_vtnet.c
1590

Sorry if I'm totally reading this wrong. But shouldn't the adjustment happen if __NO_STRICT_ALIGNMENT is _not_ defined?

Take a look at vtnet_rxq_eof(). When it dequeues an mbuf, it first removes a virtio header that appears before the protocol headers. adjsz mod 4 can be either 0 or 2 depending on the configuration. In particular, sizeof(struct vtnet_rx_header) mod 4 is 2, so when you strip that _and_ the ethernet header off, you get 4-byte alignment, assuming the original cluster was 4-byte aligned. But with this patch we end up misaligning in that case.

In D43161#983669, @imp wrote:

I'd like to acknowledge that this just fixes the driver in the same manner other drivers are fixed, but to fully fix it requires packing changes in a lot of places, which introduce performance regressions. This fixes it for the typical case, but maybe not for the VLAN case, etc. Given there's not a complete, agreed upon solution to the general problem, this at least stops the insta-panic on boot in a way that doesn't create other issues.

You mean, adding __packed annotations to various structures so that the compiler doesn't try to use ldp etc.? Yeah, it's hard to say what the ramifications of that would be given that they're visible to userspace. Though, some structures (e.g., struct ip and struct ip6_hdr) are already packed. VLAN headers are 4 bytes so I think they don't pose a problem here...?

sys/dev/virtio/network/if_vtnet.c
1590

Right, these should all be #ifndef. I note that only x86 defines __NO_STRICT_ALIGNMENT, which isn't really right. arm64 at least doesn't suffer from the problem that this patch is trying to solve.