In D54356#1242220, @glebius wrote:

Have you tried chaining mbufs instead of having an array? Does that have negative cache effect?

I've only been able to test this on amd64, and I'm pretty bad at atomic acq/rel semantics. If somebody could review that, I'd appreciate it.

In D54275#1240636, @jhb wrote:

Maybe to make it clearer we can change the struct to use a union of next_seqno and initial_seqno which won't change the ABI but will let us document it in the header and be more clear about the intent?

added union for next_seqno / initial offload seqno to provide a better description & preserve ABI, as suggested by @jhb

• realized that so_unsplice can be called with an so2 that's already been recycled when its called via so_splice(), since we don't hold a reference. So ensure we null out sp_dest and deal with a NULL so2 in so_unsplice

Remove changes to the so_unsplice() path on SPLICE_INIT splices.. @markj correctly pointed out that codepath is not a problem.

In D54053#1235030, @glebius wrote:

My understanding. First, qsort() itself never passes NULL pointer. Second, could a map[] entry within p->pm_count be NULL or not? The new code assumes it can't be. Then we also have a case with lacp_port that has NULL lp_ifp, which is covered.

Remove more defensive debug code

update based on Gleb's feedback

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?