Awesome. Thank you!

Can we add some __predict_false() to aid the compiler (and the reader) to know these are rare?

In D26691#595529, @hselasky wrote:

Looks good from mlx5_en_main.c's point of view.
@gallatin: Did you test this with mlx5en?

Thank you for making these changes.

In D26409#591582, @rscheff wrote:

So, how about moving the PCP to become part of the inpcb, and use IPPROTO sockopts instead. Sounds like that may be the better approach; doing this in the socket struct was "just" the easiest way to get a proof-of-concept running for various reasons... While the PCP is part of the (extended) Ethernet Header, there are no setsockopt Protos that deal with data on that level...

Is there a file which is already perfectly style(9) compliant? Is it this file?

What command line arguments, or other clang format configuration was used here?

In D26150#584330, @rlibby wrote:

Is there a possible side effect where if we now never do an M_WAITOK
alloc from zone_jumbop then we may not increase the zone size up to the
limit? As in, should there be a condition in here where we only convert
M_WAITOK to M_NOWAIT if we are actually near the limit? For example
should mb_reclaim arm this?

In D6656#579632, @hselasky wrote:

I mean DMA -> mbuf chain.

Address markj's feedback:

In D6611#580267, @jhb wrote:

I think one question is how can you really identify these packets? You can send a packet into a VM (e.g. via an if_tap interface from a localhost connection) but that VM might itself forward the packet internally to another interface that then heads out onto the wire. You can't really know that before you send the packet into the if_tap, and the VM itself doesn't know which interfaces are internal and which are not.

Do you have an update to BUS_DMA(9)? Sorry if I'm missing it just because there are so many files in the review..

In D6656#579330, @royger wrote:

In D6656#578863, @gallatin wrote:

In D6656#578326, @gallatin wrote:

I'm confused. t_maxsegsize is basically the connection's MSS, eg, what the NIC is supposed to break the TSO into. However if_hw_tsomaxsegsize is the max size of the TSO to be broken up. Eg, on a NIC without oddball limitations, if_hw_tsomaxsegcount might be 64, if_hw_tsomaxsegsize might be 65535. That means that TSO is supposed to send a max of 64K down to the NIC, in a max of 64 packets. If t_maxseg is less than 1000, then we'd have to limit the TSO to 64 packets.

It seems like by restricting if_hw_tsomaxsegsize to t_maxseg, you're essentially turning off TSO, since you're only allowing one MSS to be sent at a time.

Ugh, I'm wrong. I was confusing t_tsomax with t_tsomaxsegsize. I forgot that some NICs are apparently so broken that they limit TSO seg sizes.

So this is correct? Or am I missing something else?

In D6611#579324, @royger wrote:

In D6611#578858, @gallatin wrote:

This will break some NICs (see comments on D6656 -- basically, if you're setting CSUM_TCP then you need to replace the TCP checksum with the pseudohdr checksum, and store the offset of the TCP header's checksum in csum_data).

The important question is: If the packet has already had its checksum checked, then the checksum is correct. Why not update the checksum normally and send the packet without checksum offload enabled?

Right, I think this (likely) an optimization only used by para-virtual nics. The problem here is that the guest receives a packet that has originated in the same box from a different guest, using a para-virtualized nic. Most para-virtualized nic implementations have an option to avoid calculating the checksum, since the actual hardware will calculate the checksum when the packet hits a physical wire.

In the case above however the packet never actually hit the wire, and thus there's no checksum calculated at all. Hence it would be helpful to have a way to internally forward such packets that have a correct checksum but that such checksum is not available anywhere. Having to calculate the checksum would unnecessarily slow things down, as we know the data is correct, just there's no checksum anywhere.

Isn't there a way to forward such kind of packets (correct packets that don't have the actual checksum anywhere)?

According to the mbuf(9) man using CSUM_DATA_VALID | CSUM_PSEUDO_HDR is the correct way to signal that a packet has been checksummed but that such value is not available in csum_data and instead contains 0xffff.

In D6656#578326, @gallatin wrote:

I'm confused. t_maxsegsize is basically the connection's MSS, eg, what the NIC is supposed to break the TSO into. However if_hw_tsomaxsegsize is the max size of the TSO to be broken up. Eg, on a NIC without oddball limitations, if_hw_tsomaxsegcount might be 64, if_hw_tsomaxsegsize might be 65535. That means that TSO is supposed to send a max of 64K down to the NIC, in a max of 64 packets. If t_maxseg is less than 1000, then we'd have to limit the TSO to 64 packets.

It seems like by restricting if_hw_tsomaxsegsize to t_maxseg, you're essentially turning off TSO, since you're only allowing one MSS to be sent at a time.

This will break some NICs (see comments on D6656 -- basically, if you're setting CSUM_TCP then you need to replace the TCP checksum with the pseudohdr checksum, and store the offset of the TCP header's checksum in csum_data).

The TSO conventions for checksum offload is that we store the TCP pseudohdr checksum in the th->th_sum. This is done as an artifact of TCP checksum offload, however some NICs require this. This change will break those NICs unless you setup the checksum properly by

What device is setting up the TSO flags on incoming frames? Shouldn't it be setting up the LRO flags, and shouldn't we be checking those and converting them to TSO in the forwarding path?

I'm confused. t_maxsegsize is basically the connection's MSS, eg, what the NIC is supposed to break the TSO into. However if_hw_tsomaxsegsize is the max size of the TSO to be broken up. Eg, on a NIC without oddball limitations, if_hw_tsomaxsegcount might be 64, if_hw_tsomaxsegsize might be 65535. That means that TSO is supposed to send a max of 64K down to the NIC, in a max of 64 packets. If t_maxseg is less than 1000, then we'd have to limit the TSO to 64 packets.

In D25514#574176, @gbe wrote:

Is there a reason to not import this into base? The isa-l library is BSD-licensed and has a very small footprint.

Why is it safe to just drop the lock?

I added Hans, as he's also got an attempt to fix the same problem (https://reviews.freebsd.org/D24236)

Great work. There are *so many* of these optimizations hiding in this code. I'm really glad you're looking at it.

Its a very worthwhile change. I personally have not had the time, and I have not heard from shurd lately. If you want to take it on, that would be great!

Thank you for making this change!

Can you add context please ? I upload patches using git diff -U9999999 to get the full file to show up.

In D24645#542519, @mav wrote:

I was thinking about something like this many times. It would be nice to skip this often useless work. But each time I am stopped by worry that it may cause CPU under-utilization on a systems with load average about 1 per SMT thread. 10ms interval you mentioned sounds quite large to me, but honestly I can imagine some synchronous irregular load patterns where any chosen interval will cause a pessimization, dramatically increasing execution latency.

Looked mostly at ktls

I'm going to let Navdeep review this..

Just a heads up that i plan to commit this tomorrow.

Fix issues encountered when running mlx5 with hw tls enabled with this patchset:

• • accessing ext_pgs via pointer was missed at compile time, as the driver was using the ext_buf rather than ext_pgs pointer
• the driver was using ext_pgs at the same time as pkthdr

• Fix accounting issue that jhb pointed out in lagg_bcast_start()