Nice! These stats look really interesting.

Also, bnxt_ktls_pre_xmit() and bnxt_ktls_submit_mbuf() both write BDs and advance txr->prod directly, and neither

compares against the consumer.  Is that safe?

Why is the if needed? UINT32_MAX +1 == 0

Can't most these just be implemented in terms of the already existing primitives in sys/bitstring.h? Eg, bnxt_find_first_zero_bit -> bit_ffc bnxt_find_next_zero_bit -> bit_ffc_at bnxt_find_next_bit > bit_ffs_

I think every mpc allocation is leaked on detach. Eg, frees are done from bnxt_queues_free(). iflib runs bnxt_detach(), which sets mpc_info = NULL. And then runs bnxt_queues_free(), which gates freeing on if (!mpc) return;

Does something need to re-apply this after reset?

• In addition to the inline comments, please make the code conform to style(9)
• There do not seem to be licenses on the new files

I spent a while looking at this, and I think maybe we should do a taskqueue_drain_all() here instead. Some drivers (bnxt) use their own tasks registered via iflib_config_task_init that deal with link events and might be subject to the same issue.

In D58266#1336569, @netchild wrote:

This fires too often, maybe some false positives. I will rework.

The whole watchdog thing, like mp_ring, is absurdly over-complicated for no real reason. Just do it the mxge way: if we have pending transmits (eg, producer != consumer) *AND* the nic has not consumed any packets since the last time the watchdog callout ran, then reset. I get dizzy every time i look at the iflib queue-state stuff. Its just extra complexity that adds nothing.

Before and after:

I'd write this as 'mcopydata(m,off, len, mtod(..));

I agree that its unfortunate to burn a flag on this, but in the near term, I think this fixes a real bug and should be committed.

In D57537#1321127, @kp wrote:

In D57537#1319082, @gallatin wrote:

It took me a while, but I think your patch is correct.

@gallatin Do you want to commit this, or should I? (Reid doesn't have a bit, so someone else needs to.)

It took me a while, but I think your patch is correct.

So, TL;DR, eventlog is different enough from how bblog is architected that it must be a new system.

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()