In D55203#1309727, @glebius wrote:

For this particular patch, I'd suggest the following.

The _svc_vc_checkextpg() should be made a generic function living somewhere in sys/net/route. It should accept socket address and return a referenced struct nhop_object *. It should call fib[46]_lookup() with NHR_REF flag. It also should be passed a u_int * argument that it shall fill with current route table generation number obtained via rt_tables_get_gen(). Alternatively the generic function may assert the epoch, and then it would be the caller's choice to pull the route generation number together with the lookup or not.

The svc_vc code should call this generic function at setup and store returned struct nhop_object * and rtgen number in its xprt.

On packet generation it should first do rt_tables_get_gen() to validate that routing table did not change. If it did, then dereference the stored nhop and call the generic function again to get a new one. After that it can consult xprt->nh->nh_ifp->if_capenable to decide if to use extended mbufs. AFAIK, we are always in the net epoch at the packet generation.

The above is not 100% beautiful as the rt_* and nhop_* and fib_* KPIs are slightly decoupled, despite they describe the same backend. However, the above is exactly what the generic ip_output() does today.

P.S. Alexander had left a promising comment for bright future in nhop.h:

* TODO: subscribe for the interface notifications and update the nexthops
*  with NHF_INVALID flag.

So some day this is going to be improved.

In D55203#1309629, @kib wrote:

I think it could be set to 'enabled' for machines with DMAP, i.e. amd64/arm64, and might be risc-v, if anybody ever uses nfs server on it. For other arches, the knob almost surely should be kept disabled.
mb_unmapped_to_ext() uses non-privately mapped sfbufs for all extents of mbufs. On DMAP systems, it is free. On other arches, allocating such sfbuf causes global IPI, and the whole chain of sfbufs is freed only after the mbufs are released by the network card. Besides the cost of allocating , this would make sfbufs scarce resource for other consumers and even for nfs server itself. There are around ~1K of sfbufs, and mb_unmapped_to_ext() seems to drop packet if an sfbuf cannot be allocated immediately.

In D55203#1309439, @rmacklem wrote:

In D55203#1309344, @gallatin wrote:

In D55203#1309194, @glebius wrote:

I don't think this is a right fix. A route lookup now doesn't guarantee the same interface will be used in the future. There is dynamic routing, weighted routing, policy routing, etc etc etc.

There should be some generic gate that would convert mbufs otherwise, we will need to add a code like this every module that generates mbufs. Again, a code that is not correct when routing isn't static.

As Rick said in the description, this is just a hint, and the conversion routine in ip_output() will handle any case where the egress NIC changes. However, just using extpg mbufs all the time seems like a better solution. That's what SW ktls offload does. mb_unmapped_to_ext() is not super expensive, and most high-ish performance NICs are aware of extpgs these days (iflib + mlx5 + cxgbe covers most 10Gb or more NICs in practice).

Kostik preferred not enabling it all the time.
See thtis:
https://lists.freebsd.org/archives/freebsd-net/2026-May/008766.html

The case of concern is some corner case, where
the "always enabled" causes a regression. I asked on freebsd-net@
to try and determine if such a case exists?

Kostik didn't indicate if he thought such a case exists, but was
concerned that the user wouldn't understand why the regression
happened (and the NFS patch includes a sysctl that turns it off).
--> Right now, it is never enabled, so not enabling it does not

 introduce a regression.
Enabling it unconditionally makes a regression more likely
than only enabling it when this hint thinks the NIC can do it
and that hint is incorrect.

I doubt NFS servers have frequent routing changes, but I do not
know that for certain?

You guys can debate it. If there is no consensus, I'll just leave
it disabled as it is now.

In D55203#1309194, @glebius wrote:

I don't think this is a right fix. A route lookup now doesn't guarantee the same interface will be used in the future. There is dynamic routing, weighted routing, policy routing, etc etc etc.

There should be some generic gate that would convert mbufs otherwise, we will need to add a code like this every module that generates mbufs. Again, a code that is not correct when routing isn't static.

LGTM

Address review feedback by moving this into tcp_twstart()

In D39633#1293388, @csjp wrote:

a few years late to the party here but have you tested this?

Thank you for fixing this for me!

In D56484#1292880, @glebius wrote:

Two options: 1) embed all of this in struct tcp_rack:

#ifdef TCP_REQUEST_TRK
        /* Response tracking addons. */
        uint8_t t_tcpreq_req;   /* Request count */
        uint8_t t_tcpreq_open;  /* Number of open range requests */
        uint8_t t_tcpreq_closed;        /* Number of closed range requests */
        uint32_t tcp_hybrid_start;      /* Num of times we started hybrid pacing */
        uint32_t tcp_hybrid_stop;       /* Num of times we stopped hybrid pacing */
        uint32_t tcp_hybrid_error;      /* Num of times we failed to start hybrid pacing */
        struct tcp_sendfile_track *t_tcpreq_info;
#endif

or 2) create a new structure that holds all of the above, lazily allocate it and have a pointer from struct tcp_rack to it.

In D56337#1289068, @bz wrote:

I am wondering if you can see the V_ipforwarding/V_ip6_forwarding hits?
Does it make any differences if you do the mbuf pkthdr checks first?

Silly idea I don't like much as inet and inet6 should be distinct but would it help if we would make sure they are in the same cache line?

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?