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()

Make cosmetic/style fixes as suggested by jhb

Update to reflect jhb's feedback. We can stop checking the ret value of lagg_enqueue() inside the loop entirely, as it will be overwritten the next time it is used outside the loop, so this simplifies things as well.

• Rebase past r359474
• Move m_ext_pgs down below m_pkthdr in the union, as suggested by Hans

Picture a case where you're asked to refill 128 slots, and fail after 127. Don't you want to at least flush the 127 that you were able to allocate?

Comment changes suggested by jhb

In D24204#532903, @hselasky wrote:

Change looks good.

One comment:
Style is bcopy() and not memcpy() ?

• switch from bcopy to memcpy as suggested by Hans

• Remove ifdef KERN_TLS from locals' declarations and move those into the ifdef'ed block as suggested by scottl
• Add comment regarding where n comes from as suggested by scottl

After discussion with Hans, it turns out that it is the record type, not the seqno, that the Mellanox driver needs in the trailer for hwtls.

In D24213#532734, @kib wrote:

In D24213#532730, @gallatin wrote:

In D24213#532679, @kib wrote:

Don't you want to provide some asserts about the struct mbuf layout ?
Imagine I do not know anything about constraints and want to add a field somewhere.

In kern_mbuf.c, I replaced the constraint that m_ext_pgs was 256b with the constraints that the offset of m_ext in struct mbuf matches the offset of m_ext_pgs.m_ext, as well as a constraint that the size of struct mbuf does not exceed MSIZE. Did you have others in mind? The ones I added caught a few problems on 32-bit platforms which naturally align 8-byte types, for example.

No, I do not, otherwise I would suggest the checks.

BTW, you commented out some asserts, why not remove them if they are not relevant ?

• Restored static asserts on the size of m_ext() that I'd forgotten I'd commented
• Changed copy of seqno to a memcpy

In D24213#532679, @kib wrote:

Don't you want to provide some asserts about the struct mbuf layout ?
Imagine I do not know anything about constraints and want to add a field somewhere.

Committed as r358808. (however I forgot to tag the review in the commit message)

I get it, but that (totally arbitrary) limit is a pet peeve of mine. Its one of those things that you follow up several levels of code to an "XXX" comment and an arbitrary value. Its like reading a mystery novel and finding the butler really did do it.

Funny how the comment right above the assert calls it out as bogus..

Thank you for this cleanup.

Why not just remove the limit, rather than making things even more complex?

Abandoning this in favor of moving selection of an uncongested queue for lacp into the mlx5 driver.

In D23752#521981, @np wrote:

This doesn't look right to me.

Rate limiting a connection doesn't have much to do with tx queue
selection except on mlx5_en but this patch adds this assumption to lagg.
Even in theory this can work only with kernels with RATELIMIT (GENERIC
doesn't have it) and then only with the two NICs that support RATELIMIT
(cxgbe and mlx5_en). In practice this might fix the problem for mlx5_en
but will make it worse for cxgbe.

Scott added rsrv_noflowq to cxgbe in r261558 for this very reason. That
is driver-specific too but at least the change is entirely within the
driver and doesn't pollute sys/net. Can't something like that be done here?

I'm 1/2 joking, but what would you think about not supporting extension headers at all? They are the worst part of IPv6 and make everything complicated and add lots of hairy cases. What benefit are they?
(I'm legitimately curious)

I'd prefer the check be moved inside _iflib_assert(), but that's just a nit.

What we really care about is not re-ordering the load so that we ensure it happens prior to the loop. So I think you want an atomic with acquire semantics:

"the operation must have completed before any subsequent load or store (by program order) is performed"

To expand on this, the issue is that interrupt handlers are removed via intr_event_execute_handlers() when IH_DEAD is set. The thread removing the intr is woken up, and he calls intr_event_update(). When this happens, the ie_hflags are cleared and re-built from all the remaining handlers sharing the event. When the last IH_NET handler is removed, the IH_NET flag will be cleared from ih_hflags (or ie_hflags may still be being rebuilt in a different context), and the ithread_execute_handlers() may return with ie_hflags missing IH_NET. So we can end up in a scenario where IH_NET was present before calling ithread_execute_handlers, and is not present at its return, meaning we must cache the need for epoch locally.

I like the idea of using a different ether_input() for epoch and non-epoch safe drivers to get the tests out of the critical path. However, this is a minor optimization at best, as branch predictors are really good these days on modern CPUs.

Some feedback:

As we discussed in slack, I'm not a huge fan of fixing things with a callout. If the hardware was amenable, I'd much rather leave the ring partially stocked and drop new packets until we were able to allocate mbufs again. However, based on the findings you reported with igb (it wanting a full rx ring to generate interrupts), I'm afraid that might require too much work from hardware drivers. And I'd prefer a fix to a real problem, even if its not something I personally find appealing, to leaving a real bug unfixed and having machines become unreachable.