In D53674#1225349, @mjg wrote:

these also need to go:

sys/net/iflib.c:#define IFC_PREFETCH            0x040
sys/net/iflib.c:        if (rxq->ifr_ctx->ifc_flags & IFC_PREFETCH)
sys/net/iflib.c:        do_prefetch = (ctx->ifc_flags & IFC_PREFETCH);
sys/net/iflib.c:                ctx->ifc_flags |= IFC_PREFETCH;

In D53674#1225304, @mjg wrote:

This looks legitimate.

The kernel does not have infra for static branches, so a tunable would not look good anyway. However, instead of just removing it I would propose just ifdefing the code for the time being, maybe start the file with:

/*
 * Uncomment to enable prefetching. Measurements in https://reviews.freebsd.org/D53674 show it hurts performance.
 */
//#define IFLIB_PREFETCH

Then interested parties can experiment should the choose to.

If mere existence of this code is making other work harder, then by all means remove it.

• tried to address kib's concerns

• added comment regarding allocation failure behavior, as suggested by markj

Is there any concern that this new use will add pressure to the sf_buf system on non-direct map platforms?

• Made rss_config.c standard. This is needed because most

drivers do not check to see if INET/INET6 is enabled before
setting up RSS. This fixes compilation errors in most
drivers with LINT-NOIP

In D53176#1214928, @mjg wrote:

Do you have stats how many classes can be realistically present? maybe this would be way faster single-threaded by iterating an array?

In D53100#1214833, @akiyano_amazon.com wrote:

I built and installed the latest kernel with this patchset in AWS and ran a network sanity check with iperf and all looks good.

turned on net.inet.tcp.input_verify_hash with sysctl net.inet.tcp.input_verify_hash=1

ran traffic

lookede at

sysctl net.inet.tcp.input_nohash
sysctl net.inet.tcp.input_badhash

and both were 0

no prints to dmesg

commit lgtm.

In D53101#1214513, @zlei wrote:

In D53101#1214510, @zlei wrote:

I'm not getting this change. The kernel build option RSS is not enabled by default. Is this change want to enable a variant RSS when RSS is not enabled in the kernel ?

Ahh, I missed D53089 .

In D53091#1214239, @kib wrote:

In D53091#1213715, @gallatin wrote:

In D53091#1213605, @kib wrote:

Sorry, I do not understand the change.
The compilation of both in_rss.c and rss_config.c is gated by the RSS option, and this seems to not change for the whole time these files existed.

In other words, if the 'options RSS' is not included into the kernel config, would the driver even compile?

Sorry, I wish I was better at reviews of patch series. This is one of many patches, starting with https://reviews.freebsd.org/D53089 and ending with https://reviews.freebsd.org/D53104
The goal is to achieve consistent hashing for TCP ingress/egress by exposing the hashing parts of RSS to drivers, so that all NICs use the same hash key and algo.

I do not know how to list of the patches in your series. You could add 'related objects' to your review, then it will be visible to everybody.

That said, I have no objections against this change after D53089 goes in.

We (Nvidia) might request that the commit of this revision might be postponed until D53089 is committed, so that Nvidia' verification could do the scrutiny before driver changes.
Or you might provide us the minimal set of reviews we could apply and run verification on, to ensure that your commit is fine for mlx5.

Update patch to unifdef check for adapter using toeplitz hashing

In D53100#1213606, @akiyano_amazon.com wrote:

Hi,

Thank you for submitting this patch.
Can you please provide a commit message that explains this change.
For example questions that immediately arise:

1. What "newly exposed RSS hash key API" you are talking about (link to the commits that added this API)?
2. Why it is ok to remove the #ifdef RSS?
3. Why do you also remove the #ifdef RSS in ena_datapath.c?
4. How did you test the change?

Thanks!

In D53091#1213605, @kib wrote:

Sorry, I do not understand the change.
The compilation of both in_rss.c and rss_config.c is gated by the RSS option, and this seems to not change for the whole time these files existed.

In other words, if the 'options RSS' is not included into the kernel config, would the driver even compile?

In D53088#1213086, @tuexen wrote:

Why do we have D53088 and D53105?

Also, this needs to be MFC'ed to 15 for the release..

To be clear, igc worked fine when configured with multiple queues. I had configured it with a single rx queue in order to debug something else, and it took the somewhat reasonable approach of returning garbage as the hash since it didn't need to hash packets to multiple queues. Thank you for coming up with this... it is much cleaner than my solution, and should ensure that any other non-iflib drivers avoid this behavior as well.

Update patch to address @markj 's feedback, and to properly fix the bug I encountered where doorbells were withheld when using the mp_ring transmit method.

@markj I've marked things as done as I've done them in my local tree. I'll update the patch once I do some more research into the doorbell issue. thank you for the reveiw!

• remove extra space in iflib_if_get_counter as pointed out by @kbowling

In D52369#1197238, @markj wrote:

Instead of making each driver report 0 if it doesn't collect a drop counter from the MAC, and treating these counters as a special case in iflib_if_get_counter(), wouldn't it be simpler and cleaner to make them all drivers return the value from if_get_counter_default()?

reversed logic and made the driver responsible to query the generic counters for odrop/oerr rather than iflib.

Updated drivers that were getting generic counters using if_get_counter_default() as part of their ifdi_get_counter method to avoid getting generic counters for output drops and output errors, as suggested by @markj .

• Call iflib_remove_mbuf() to unmap and remove the mbuf from list of mbufs to be freed when we free an mbuf due to ENOBUFS

In D51772#1185535, @markj wrote:

removing forwarding checks ... LRO already does this

But LRO only checks the forwarding configuration when it actually comes time to process packets. With this patch, if forwarding is enabled, we'll do a bunch of needless sorting.