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

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.