Added check for empty domains in uma_large_malloc_domain(), and removed added blank line, as pointed out by Mark

In D16836#365423, @markj wrote:

We already have code to renumber NUMA domains to avoid holes in the domain ID space (renumber_domains() in srat.c). Wouldn't it be simpler to apply a similar approach to empty domains, or is there some reason that won't work?

FWIW, as I mentioned in private email, the testing went fine. I verified that there were no apparent mem leaks, and that in_pcblbgroup_free_deferred() was called when I bounced nginx several times on a heavily loaded (> 90Gb/s) box.

We've had it in our config at Netflix for so long that I forgot that it was not in GENERIC..

For what it is, Van's birthday might be a better choice. Eg, some non-valid pointer that would cause free or mfree to panic rather than accept it and corrupt data.

Sorry, having the ring pointer in the ring is just so odd.. Can you explain why it is there? Maybe it would be better to just not have the ring pointer in the ring and not have to have all these special cases.

What keeps the normal, non-IFC_QFLUSH from hitting this condition? Is the txq pointer included in r->size?

In D16836#359533, @markj wrote:

I haven't had much of a chance to review this yet, but I wanted to test an "options NUMA" kernel on a system with an empty domain (dual socket Haswell xeon with only one DIMM installed at the moment). This patch worked fine for me. Thanks!

Thanks for the clarification on the non-offset range!

Thank you so much for the pointer to the docs!!! It is much nicer to write from real docs, rather than crib from undocumented changes in another project.

In general, I looked around at ixlv.c, and it looks like there are many functions for the main pf which are identical, or nearly identical for the VF. Can we please work on collapsing them down to common code?

Thank for doing this!

I ran this on a Netflix 100g box. I observed no measurable difference in CPU time. So I think this patch is "neutral" for the perspective of our (mostly kernel) workload.

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.