Paths

Table of Contentst

mbuf(9): Implement a leaf network interface field in the mbuf packet header.
ClosedPublic
Actions

Authored by • hselasky on May 27 2022, 7:44 PM.

Details

Reviewers

jhb
glebius
gallatin
gnn

Group Reviewers

network

Commits

rGc4c5981c14d5: Bump the FreeBSD version after recent mbuf(9) structure changes.
rG30c85a085ed6: mbuf(9): Update comment about pkthdr structure size.
rGb8394039dc25: mbuf(9): Fix size of mbuf for all 32-bit platforms (i386, ARM, PowerPC and…
rG4d88d81c3166: mbuf(9): Implement a leaf network interface field in the mbuf packet header.

Summary

When packets are received they may traverse several network interfaces like
vlan(4) and lagg(9). When doing receive side offloads it is important to
know the first network interface entry point, because that is where all
offloading is taking place. This makes it possible to track receive
side route changes for multiport setups, for example when lagg(9) receives
traffic from more than one port. This avoids having to install multiple
offloading rules for the same connection.

This field works similar to the existing "rcvif" mbuf packet header field.

Submitted by: jhb@
MFC after: 1 week
Sponsored by: NVIDIA Networking
Sponsored by: Netflix

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

• hselasky created this revision.May 27 2022, 7:44 PM

Herald added a subscriber: imp. · View Herald TranscriptMay 27 2022, 7:44 PM

• hselasky requested review of this revision.May 27 2022, 7:44 PM

• hselasky edited the summary of this revision. (Show Details)

I tested this on a Netflix https serving workload, and I do not see any performance regressions from increasing the pkthdr size. That surprised me.

This revision is now accepted and ready to land.May 27 2022, 7:55 PM

Why is it called "leaf"? Wouldn't "physical" or "original" be a better description?

glebius added a reviewer: network.May 27 2022, 9:59 PM

The problem of rcvif being rewritten as a packet travels up the stack has been affecting not only offloading. I subscribed more people to gather more opinions. That's definitely is a useful feature. Let's get it right from the very beginning.

In D35339#801187, @glebius wrote:

Why is it called "leaf"? Wouldn't "physical" or "original" be a better description?

From the use of "leaf node" in graphs / trees in programming. That is, if you view the set of interfaces as a kind of graph where packets enter in a leaf node (and the "top" or root of the tree is the protocol layer). That graph perhaps isn't quite as simple as that as you have interfaces like if_wg that sit higher in between protocols, but that's the origin of the name.

LGTM

@glebius To be clear, I'm not opposed to other names, just explaining why 'leaf' is the name I came up with.

Can you elaborate more on what some other use cases would be? Right now the other patches for KTLS RX only set this field in packets in the mlx5 driver as it is the only driver to do NIC TLS RX. However, if there are other use cases it may make sense to set this more centrally, e.g. in ether_input and possibly other if_input routines (vs having to get set it all the drivers). Or we could define a helper macro perhaps for use in device drivers that sets both pointers that drivers would use.

BTW, one other thing that KTLS currently kind of hacks around in the RX patches is that the vlan ID is "lost" on the way up the stack (vlan_input strips M_ETHER_VLAN_TAG or whatever the flag is). Would other use cases prefer to have other information about the packet's "initial" state preserved further up the stack?

Just my humble opinion, "physical" would describe it better than a "leaf". Just thinking about all kinds of tunneling, and there could be leaves under leaves.

Other use cases that I can image are vlan, lagg, netgraph and other who rewrite rcvif, while upper layers potentially might be interested in what was the physical interface.

In D35339#801825, @glebius wrote:

Just my humble opinion, "physical" would describe it better than a "leaf". Just thinking about all kinds of tunneling, and there could be leaves under leaves.

Other use cases that I can image are vlan, lagg, netgraph and other who rewrite rcvif, while upper layers potentially might be interested in what was the physical interface.

Hmm, by use cases I was more thinking about the "upper layers". KTLS is one such example, but I'm not sure if other use cases want more info (e.g. KTLS would really like the vlan to persist if possible, but I'm not sure if other upper layer use cases would want that as well)

Any more comments? Or are we ready to give this a try?

In D35339#802482, @hselasky wrote:

Any more comments? Or are we ready to give this a try?

No objections from me on the idea as is. We definitely need this information. However, I would ask to declare it open for later modifications.

Apart from the naming matter, that I already raised, I got more thoughts on this. At some point we should probably differentiate between network interface and network device. As we already do in wlan(4) and its underlying devices. Once a Ethernet device becomes a part of lagg(4) it effectively stops to be a network interface but remains a network device. In the scope of this patch we probably want the device to be saved on the received mbuf. So potentially the new pointer shouldn't even be a pointer to struct ifnet.

Again, no objections for the current patch.

Thank you. Will push this later today.

Closed by commit rG4d88d81c3166: mbuf(9): Implement a leaf network interface field in the mbuf packet header. (authored by • hselasky). · Explain WhyJun 7 2022, 11:00 AM

This revision was automatically updated to reflect the committed changes.

• hselasky added a commit: rG4d88d81c3166: mbuf(9): Implement a leaf network interface field in the mbuf packet header..

I just observed a build failure for ARM which appears attributable to this.

--- kern_mbuf.o ---
sys/kern/kern_mbuf.c:343:1: error: static_assert failed due to requirement 'sizeof(struct mbuf) <= 256' "size of mbuf exceeds MSIZE"
_Static_assert(sizeof(struct mbuf) <= MSIZE,
^              ~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Upon reverting the commits associated with D35339 and D32356 the build was successful.

Thank you.

I'll have a look at it.

Exactly which ARM architecture is this?

I tried to build several ARM targets and cannot reproduce the build failure:

make buildkernel TARGET_ARCH=armv6 
make buildkernel TARGET_ARCH=armv7  
make buildkernel TARGET_ARCH=aarch64 
make buildkernel TARGET_ARCH=armv7 KERNCONF=RPI-B

--HPS

In D35339#803382, @hselasky wrote:

Exactly which ARM architecture is this?

Was simply a generic ARM kernel build, no customization simply the latest git had. At this point it looks like D35339/4d88d81c316 was the immediate cause, but that requires reverting cb27627968e, f0fca646180 and fe8c78f0d20 in order to build. I suspect ARM64 and RISC-V builds may also be hit by this, but I haven't yet confirmed.

make buildkernel TARGET=arm

Thank you!

I have a reproduction.

• hselasky added a commit: rGb8394039dc25: mbuf(9): Fix size of mbuf for all 32-bit platforms (i386, ARM, PowerPC and….Jun 7 2022, 8:14 PM

Should be fixed now:
https://cgit.freebsd.org/src/commit/?id=b8394039dc252698711ded30045af20481db79e0

Please verify!

• hselasky added a commit: rG30c85a085ed6: mbuf(9): Update comment about pkthdr structure size..Jun 7 2022, 8:21 PM

I've only got so much computing power so kernel builds take a bit. Appears b8394039dc2 did fix the issue. Also confirmed this effected ARM and i386 (ARM64 and RISC-V were unaffected).

• hselasky added a commit: rGc4c5981c14d5: Bump the FreeBSD version after recent mbuf(9) structure changes..Jun 8 2022, 6:38 AM

I kicked off a universe build.

Looks good so far.