Re-worked thread setup to use mp_ncpus rather than mp_maxid, as suggested by @markj

In D55477#1272889, @olce wrote:

• fixed typo in sysctl declaration

I tried this on 3 EPYC generations at Netflix. The most recent (AMD EPYC 8434P, AMD EPYC 9535) behaved as expected. There was an dev.hwpstate_amd. node for each CPU, and a lot more freqs were exposed (from 3 to roughly a dozen).
On the oldest EPYC we have is EPYC 7502P, where it didn't change anything. We still only have 3 frequencies exposed. On this machine, we see just a single node from dev.hwpstate:
dev.hwpstate_amd.0.freq_settings: 2500/2750 2200/2200 1500/1350
dev.hwpstate_amd.0.%iommu:
dev.hwpstate_amd.0.%parent: cpu0
dev.hwpstate_amd.0.%pnpinfo:
dev.hwpstate_amd.0.%location:
dev.hwpstate_amd.0.%driver: hwpstate_amd
dev.hwpstate_amd.0.%desc: Cool`n'Quiet 2.0
dev.hwpstate_amd.%parent:

This looks OK to me, but I defer to Warner.

Looks good. Thank you!

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.

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.

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

In D53089#1248199, @kgalazka wrote:

In D53089#1244212, @gallatin wrote:

In D53089#1244032, @adrian wrote:

what are you seeing? I remember ixl RSS being buggy!

A valid rss type set with a 0 flowid for connections nginx accepts. Not sure why ixl can't calculate a proper hash in that case, but it can be worked around by checking to see if the flowid (rss hash result) is valid.

See https://reviews.freebsd.org/D54442

Thanks a lot for the fix! Did you see that issue on X710 or X722 interface? As far as I understand X722 HW supports more PCTYPEs. They were enabled by default when RSS was undefined, but it looks that they are not covered when rss_gethashconfig is used.

Bravo. Anything we can do to make iflib simpler is a good thing in my book!

• use atomic_store rather than atomic_set, as per @markj 's feedback
• update man page date

• Updated to document tx_defer_mfree separately

• rebased diff
• added documentation for tx_defer_mfree sysctl

In D54356#1246570, @ziaee wrote:

Note that deferred mbuf freeing is not enabled by default, and can be

enabled using the dev.$DEV.$UNIT.iflib.tx_defer_mfree sysctl.

Can you put tx_defer_mfree in iflib(4) in this commit?

In D53089#1244032, @adrian wrote:

what are you seeing? I remember ixl RSS being buggy!

In D53089#1242869, @zarychtam_plan-b.pwste.edu.pl wrote:

Is it safe for MFC to stable/15 ? I see no plans for the MFC, I see that cherry-picking works fine, but there are clearly no plans for MFC in the commit message.

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