In D55912#1287051, @kib wrote:

Sorry for the late reply.

I think this should work as is. See below.

I hate that I'm saying this, but this isn't just an ice(4) problem... The same thing will happen for the queue selection in iflib itself for most other drivers, and for the queue selection in non-iflib drivers. So I think it should be solved in a more generic fashion.

Updated diff to add a new ioctl for page-bank operations.
Changed the 2 drivers that might use page/bank (iflib, mce) to accept the new ioctl and to ensure they are not reading garbage page/bank fields when operating with the legacy SIOGI2C ioctl

In D55912#1282432, @kib wrote:

I mean something like this (ignore naming), in the generic ioctl handler, not in the driver code;

case SIOCGI2C_OLD:
     i2c.page = i2c.bank = 0;
     ioctl(SIOCG2C, &i2c);
     break;

I.e. drivers only need to implement the new ioctl that knows about bank/page.

Or put it differently, your original patch for mlx5en then would be correct, with this addition to the net/, instead of requiring each driver to have code to handle both ioctls.

I think that I may not understand your proposal. When you say "at the ioctl level..", Do you mean intercept the ioctls in ifhwioctl()? I think that seems awkward because ifi2req lives in userspace, and individual drivers copy it in / out, rather than it being done in a single spot for all drivers. So to zero fields, ifhwioctl would need copy-in ifi2creq, fix it up, & copy it out again..

In D55912#1279758, @kib wrote:

Why not simply not allocate the new number for the new ioctl. Rename current ioctl to something like SIOCGI2C_NOBANK. Then, at the ioctl level, do two things:

• for new SIOCGI2C check that reserved bytes in the structure are zero
• for SIOCGI2C_NOBANK, zero bank/page/padding and call into SIOCGI2C

@kib What if we reclaimed some of the spare fields in ifi2creq to a uint16_t magic field. User sets i2creq.magic=PAGE_BANK_REQ and driver sets magic=PAGE_BANK_ACK ? That prevents the confusion you're concerned about.

This is really clever. What about other lock types (rm / rms, sx, etc)?

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