In D55143#1260975, @cc wrote:

In D55143#1260913, @kbowling wrote:

Thanks. This matches i.e. http://iommu.com/datasheets/ethernet/controllers-nics/intel/e1000/ethernet-controller-i350-datasheet.pdf pg.313-314.

We can infer that igc needs a similar change based on lineage.

ixgbe uses the same hash function and nullification per http://iommu.com/datasheets/ethernet/controllers-nics/intel/ixgbe/333168-x540-datasheet-v3-1.pdf pg.290-291

I suspect this issue is now widespread in other drivers; M_HASHTYPE_OPAQUE and M_HASHTYPE_OPAQUE_HASH would only be suitable if the card is producing a unique but not tagged to one of the standard protocols (i.e. cxgb, ena are probably using it right, ixl, iavf, ice have one error).

I am afraid so. Maybe it's time to replace M_HASHTYPE_OPAQUE with M_HASHTYPE_NONE in default. I will bring this issue (together with the recently introduced sc_flowid) in Transport@.

1. cancel the unnecessary change to sc_flowtype check in syncache_socket()
2. for the rest, check RSS types by a new macro M_HASHTYPE_ISHASH_TCP

In D55196#1262402, @tuexen wrote:

In D55196#1262391, @gallatin wrote:

In D55196#1262351, @tuexen wrote:

In D55196#1262270, @gallatin wrote:

In D55196#1262239, @kbowling wrote:

I think it's driver bugs that have flown under the radar until the recent RSS change so I'm not sure this is the place to do this.

In particular to this review, opaque and opaque_hash should both be usable hashes if they are implemented correctly in the driver.

Thinking about this more, I think when we are using the hash for setting flowtype, we should check for valid toeplitz hash types, since we're counting on being able to calculate the same hash in software. So I think this patch is probably the correct place to fix this. I think it would be shorter/cleaner to use M_HASHTYPE_ISHASH(). See https://reviews.freebsd.org/D52989

Question: Is M_HASHTYPE_RSS_IPV4 or M_HASHTYPE_RSS_IPV6 (a hash only on the src/dst address) acceptable or not? M_HASHTYPE_ISHASH() is true for them.

It should never happen. But to be safe, maybe make a new M_HASHTYPE_ISHASH_TCP ? At this point, my only objection is to the ugly comparisons :)

I totally agree. If we want such a hash, we should check for it.

In D55196#1262239, @kbowling wrote:

I think it's driver bugs that have flown under the radar until the recent RSS change so I'm not sure this is the place to do this.

In particular to this review, opaque and opaque_hash should both be usable hashes if they are implemented correctly in the driver.

In D55196#1262206, @gallatin wrote:

Slightly worried some NIC somwhere might not be setting this when it should. Is this solving a problem for you?

In D55143#1260913, @kbowling wrote:

Thanks. This matches i.e. http://iommu.com/datasheets/ethernet/controllers-nics/intel/e1000/ethernet-controller-i350-datasheet.pdf pg.313-314.

We can infer that igc needs a similar change based on lineage.

ixgbe uses the same hash function and nullification per http://iommu.com/datasheets/ethernet/controllers-nics/intel/ixgbe/333168-x540-datasheet-v3-1.pdf pg.290-291

I suspect this issue is now widespread in other drivers; M_HASHTYPE_OPAQUE and M_HASHTYPE_OPAQUE_HASH would only be suitable if the card is producing a unique but not tagged to one of the standard protocols (i.e. cxgb, ena are probably using it right, ixl, iavf, ice have one error).

In D55137#1260582, @kbowling wrote:

In D55137#1260494, @cc wrote:

In D55137#1260492, @kbowling wrote:

Looking at the linked change.. em can have two queues in one hw case. Does it matter?

My understanding is that if only one rx queue, RSS is not needed, so no hash value is needed. In a case of two rx queues, does not the hw enable RSS and provide RSS hash values?

I think that jives with pg.140: http://iommu.com/datasheets/ethernet/controllers-nics/intel/e1000/82574.pdf, the stipulation being we are seeing '0' in an error case and also no hw RSS?

igb_txrx might need the same fix

In D55137#1260492, @kbowling wrote:

Looking at the linked change.. em can have two queues in one hw case. Does it matter?

use the macro M_HASHTYPE_CLEAR instead, no functional change

As discussed, this change is not necessary.

The latest FreeBSD MAN page for TCP(4) shows the TCP_MAXSEG allows to determine the result of negotiation between sender and receiver.

Does not seem to be needed as the abandon reason is in https://reviews.freebsd.org/D47218#1253529.

In D47218#1253522, @rrs wrote:

What other CC modules are you contemplating using this for?

My one comment on the change is that now you will always be setting something into the cwnd.. the old code only
did it when there was a change. Have not looked at the cache line layout of the data, I suspect it makes no difference
since you probably have the cwnd in cache anyway :)

Does not seem to be needed.

In D53104#1217628, @adrian wrote:

ok, so this looks fine, but I'm trying to remember where I used to assign the flowid/flowtype for a connection back in the day. On the mbuf side it used to be done in the driver side and then overriden by the network stack input path if it didn't match what was required (eg the magic required in ip reassembly, which is still there, but also to support completely software flow hashing.)

(remember way WAY back in the day the flowid/flowtype was also used by kip's flowdirector stuff for doing more explicit control over where flows went, and RSS just 'also' used it.)

For reference, the Linux kernel recently changed it from 6MB to 32MB.

Looks good to me. I only have two minor suggestions.

In D7986#1184580, @kbowling wrote:

@imp if I recall this was something we were looking at on my team at LLNW, if it were in Bugzilla I'd say "Close - OBE" is appropriate.

@cc since you've been working on CUBIC lately, the only thing possibly still relevant here is turning the first level if into the early returns.. it removes one level of indention which might help justify a couple other nearby style(9) fixes but it's not worth holding this review open.

Thanks for the following improvements:

I am ok with this patch. But I want to be sure how snd_recover shrinks. Is it because tp->snd_recover = tp->snd_recover_prev in CC_RTO_ERR? But that shall not be in SACK recovery.

Other than the fd is not used in the test code, I am good with this change.

Thanks for the elaboration. Looks good to me now.

Looks I am missing the context. Given the fact that the socket is in TCPS_TIME_WAIT while switching to the default stack, was this assert a day one issue that was not test covered before? Or was this panic caused by recent changes?

This is for the fast path fix. Just want to confirm with your that a second patch may be around to improve both of the fast and slow path?

Didn't see any surprising regression from my test result for this patch:
testD49047

code update based on Richard's comments

re-base, add a missing part and update based on Richard's comment in the meeting

In D49047#1118719, @glebius wrote:

Question: why does this function returns signed value? And why some of the sackhint counters are signed?

There are several calls to tcp_compute_pipe() that did not check the V_tcp_do_newsack: 2 in tcp_output and 1 in tcp_input. Their behavior will change with revised SACK turned off. Commit message should explain why this is okay.