Removed redundant parens around bus_get_domain() call as pointed out by markj

Address kib's feedback

• remove unneeded _domainset.h include
• re-write if_alloc_dev() to be more concise

• Added if_alloc_dev() and changed example mlx5en and cxgbe code to use them, as suggested by kib.
• Added links to if_alloc_domain and if_alloc_dev for man pages
• Fixed style issues pointed out by kib

In D19930#428329, @kib wrote:

I wonder if a KPI of the format

struct ifnet *if_alloc_dev(u_char type, dev_t device);

would be more useful. It would hide all NUMA/non-NUMA/bus_get_domain() failed details that driver authors will copy/paste ad infinitum.
I do not propose to not add if_alloc_domain(), just think that if_alloc_dev() should be the primari KPI. Also it might allow to get more device_t context in if_alloc().

Address review feedback from marius & shurd

YES! Death to M_DEVBUF!

Fixed comments, as pointed out by ejg

I think this is a mostly theoretical issue, as you'll have had a chain which was impossible to send (eg, far too large, etc) even after defrag.

• Use new pfil_mem2mbuf() to avoid unaligned access of mbuf pointer
• Remove unneeded breaks

Much cleaner, as usual, than the hack I gave you to start with.

Guard against null pfil at device detach by deregistering pfil hook later. While here, check for a null pfil hook and cache it in a local to make the code easier to read.

I understand the missing FILTER_STRAY.

Updated to truncate the length passed to pfil for multi-segment jumbo frames to just the length of the first segment.

Any high performance interface will look similar to this and be similarly invasive.

The reason to have this in the driver is for performance. The plan for pfil is to eventually just use a pointer to a memory blob for filtering. The current fake mbuf stuff is a step in that direction.

• moved rv declaration to top of function
• entered CURVNET to prevent panic from null curvnet when VIMAGE is compiled in Note placing this at the top of the rx function is a trade off between tiny overhead in the common case (nothing hooked) and a bit larger overhead in the case when something is hooked, and entering/restoring the VNET each time we call into the filter.
• Addressed all possible return values from pfil_run_hooks Note that PFIL_REALLOCED was tested by hacking pfil.c to copy packets
• Added a label to jump to when PFIL_REALLOCED is returned, and we need to send up an mbuf allocated by the filter.

Remove XXX as per wlosh

I really like collapsing all those pointers, and making the NULL checks flags. Great work.

I like this much better; hopefully the performance tests will show that it is an improvement.

I was about to say that cxgbe still uses it, but it looks like that was just changed in r340465. So yes, lets get rid of it.

• Fixed a pre-existing bug where, when receiving small packets, rx clusters were not unmapped, yet they were re-mapped when refilling the ring. This was fixed by keeping track of the bus address in ifsd_ba, and simply re-using them rather than re-doing the virt to bus mapping

Thanks Olivier. 5% is about what I had expected. I think I see a way to improve that though. We seem to be doing repeated virt to phys translation on clusters where we have copied out a small mbuf on the rx side in iflib_rxd_pkt_get(). We call rxd_frag_to_sd() with a FALSE arg, to prevent unmapping. Yet in _iflib_fl_refill(), we seem to always do the mapping, even when we do not re-allocate clusters. This is one bit that's going to be more expensive on low-end boxes, since it will now result in a busdma callback function being called. I think this is an actual bug that would impact systems w/IOMMUs

In D17665#377526, @np wrote:

Stepping back for a second, are we going to get to a point where we can make malloc() and uma_zalloc() use the current thread's domainset policy, rather than one derived from the kernel object?

FWIW, i have patched both of these changes (this + D17492) into the upstream ck git repo, and run them through the CK regression tests for epoch in userpace on my 32-core/64-thread amd, and they passed.

In D17416#372763, @markj wrote:

In D17416#372648, @gallatin wrote:

You mention kstacks. I have not gone through the rest of your reviews, but I was wondering if you were working on affinitizing kstacks. Did you plan to break the cache out by domain, or drop it for affinitized kstack allocations? The default is so undersized these days, we're mostly not getting the benefit of the cache anyway (and the cache might be pessimal, since it is protected by a global lock).

I wasn't working on affinity. Rather, I want to ensure that the kstack allocation policy allows allocations from any domain, ensuring that we can permit kstack swapins so long as at least one domain has some free pages. See D17304; it's really just a bug fix for 12.0.

I haven't thought much about the problems you listed. Do you have a proposal?

You mention kstacks. I have not gone through the rest of your reviews, but I was wondering if you were working on affinitizing kstacks. Did you plan to break the cache out by domain, or drop it for affinitized kstack allocations? The default is so undersized these days, we're mostly not getting the benefit of the cache anyway (and the cache might be pessimal, since it is protected by a global lock).

Do we really need a new malloc variant? Couldn't this be done with a new malloc flag that was specific to malloc_domain(). Something like M_ALTOK or something? (or maybe M_EXACT if you want the default to be the other way around, and have malloc_domain() fall back by default)

This was committed as r339043. Due to a cut and paste error, the wrong review was linked from the commit message, so the review was not auto-closed.

In D16836#368220, @markj wrote:

kmem_back() now also needs updating.

• Fixed spelling errors pointed out by alc
• Updated to account for recent VM changes:
  • added empty domain guard in kmm_back()
  • fixed conflict in keg_fetch_slab() after r338755

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.