In D15937#337221, @kbowling wrote:

I've deployed a variant of this widely at LLNW on the default stack as well. I think you can garbage collect sbsndptr() from the tree.

I added Hans since it is mostly his code we are factoring out.

if @jch cannot run his real workload, what about running a synthetic test that creates / tears down many connections per second? Eg, something like many copies of netperf -t TCP_CC ?

You might want to go for some middle ground here. 1ms is a bit much (even for us).

Sure.. Those checks for TCP in the TSO case should probably turn into asserts eventually..

If people want to use ethernet entropy harvesting, can we do something to make it more useful in a separate change? I was going to initially suggest the ether dst addr, but that's not very random either.

This is the sparse detect I'm using. Note how it walks the header, and then the TSO by packet on the wire.

Somehow my last comment got lost. With real Netflix traffic, the sparse detection logic is not sufficient. When we have kernel TLS, which inserts 13 and 16 byte mbufs around TLS records, we hit MDDs regularly. I think you need to walk the packet stream, not the DMA descriptors, so that you keep the same segmentation used by the hardware. I re-wrote the sparse detection to do this, and our MDDs went away.

In D15558#328462, @gallatin wrote:

In D15558#328453, @sbruno wrote:

Was this the patch that we discussed on our call? Does this fix your TX checksum problems on IXL?

Yes and Yes.

Unfortunately, I've now proceeded to the "entire box hangs" portion of the game. Even serial console break-to-debugger is ignored. I'm going to try NMIs next..

In D15558#328453, @sbruno wrote:

Was this the patch that we discussed on our call? Does this fix your TX checksum problems on IXL?

Looks good for content to me.

Under a heavy UDP packet flood this helps quite a bit.

Looks awesome. My only issue is that I'd strongly prefer that the STAILQ_HEAD / STAILQ_INIT / STAILQ_ENTRY macros be prefixed with CK_ so that readers realize that these lists are used with epochs, and cannot be safely used with the normal STAILQ macros. I tried to point out most of them, but I may have missed a few.

It would be nice to have this patch together with at least one example of a caller and one example of a NIC driver implementing the API.

I'd just update this review.

As mentioned on the other review, maybe remove the print in the caller, since it is now redundant..

Change to using sched_prio() rather than sched_lend_prio, as suggested by @kib

In D15246#321533, @jhb wrote:

Adding kib@.

It's not clear to me why to prefer PI_NET - 1 over, say, PRI_MIN in this case. There's nothing about powerd that is network specific.

I've replaced the arbitrary PI_NET - 1 to PRI_MIN, as suggested by John

Well thats embarrassing. Thanks for the fix.

I think you'll get a lot less pushback if you serialize the multicast stuff in the stack, rather than the driver framework. This will allow you to put warnings / asserts into all the ioctl entry points above the drivers, so as to lock in the "you can't hold a lock while calling into a driver" rule.

So let me try to sum up in my own words what's going on here:

In D12638#263636, @kib wrote:

I patched this into our netflix tree. I confirmed that, even with rx timestamps enabled, the new functionality does not cause a significant performance loss in our case. There seems to be roughly one or two cache misses per mlx5e_poll_rx_cq() to check the p->priv->clbr_done > 2 condition, and roughly 8x as many for the timestamp handling itself. This pushes the cost of mlx5e_build_rx_mbuf() higher, but not horribly so.

I have not tested it yet, but this looks fantastic. Thank you for this!

In D12615#262156, @glebius wrote:

The change doesn't affect sfbufs at all.

What is LGTM?

I think this is intended to piggyback on the recent iflib change which claims a speedup from chaining the packets. However, I'm afraid that I don't understand where this speedup is coming from. The stated reason to allow chaining in ether_input() is to allow drivers to amortize the release/acquire of the rx lock. However, no decent driver even uses an rx lock anymore, certainly not iflib or mlx5. So is there a benefit? If yes, then can you explain where it is coming from?

We currently disable entropy collection (or hoarding, as you aptly describe it) for NET_ETHER. It looks like if the IFF_ NO_ENTROPY flag was present for our 100G nics, this might be slightly cheaper for us, as it will avoid the function call, and the load of the entropy mask, since if_flags will already be hot in cache from the IFF_UP check. As it is, it will be no worse .

In D11525#251667, @wblock wrote:

Added my probably-weak mdoc suggestions, and added brueffer as a reviewer.

In D11525#251340, @wblock wrote:

This is a bit confusing. The new -I and -X options take the same irq argument as the old -x?

It has been years since I've had to think about this, but I remember that the problem that all drivers fight is that you wind up being called in your ioctl routine with potentially some lock held by something that is calling you, so you cannot sleep. Is that still the issue? Can you remind me what lock is held?

Thanks for the feedback. I'll reach out to wblock about the doc changes. I'm quite weak in man page fu, and this is a slightly odd one at that..

• Feedback from jbh regarding wording

I finally tried this, and sadly it does not seem to speed things up at all.

Address kib's feedback

This seems reasonable to me

I really hate the idea of spreading the linux kpi, but I totally understand why you used it. We should probably move a lot of that stuff to a native interface.

I added Scott, as he's been quite involved in the busdma code. I want to make sure this looks OK to him too.