In D31260#703987, @mjg wrote:

Would uma_zone_reserve take care of this? The patch as proposed should not be necessary.

This is a driver bug. The hyperv driver appears to be using a taskqueue to inject packets into the network stack, rather than a normal interrupt handler. As such, I feel that it is the hyperv driver's responsibility to take the network epoch, and not force 99% of drivers which do the right thing to take the epoch twice.

Why is this even needed?

In D31172#701479, @kbowling wrote:

In D31172#701478, @gallatin wrote:

iflib_stop() is pretty heavy-weight -- is this needed for all devices? Maybe add a flag to do this only on devices where its needed, or have the device do the disabling itself?

I'm not super familiar with this area, is there something that would cause media changes to be "common" like EEE or something? I'm trying to imagine a situation where it wouldn't be as drastic as a link flap already which would justify spending more time thinking about this.

iflib_stop() is pretty heavy-weight -- is this needed for all devices? Maybe add a flag to do this only on devices where its needed, or have the device do the disabling itself?

I added myself as a reviewer. shurd has not been active recently.

dumb question: Does this depend on another review? I was trying to see what m_gethdr_raw() does, and I can't seem to find it in -current.

• Address Mark's review feedback
• Re-check inp flags before calling into the tcp function block, as ktls_set_tx_mode() drops the inp_wlock, which can result in the connection being closed. I had a panic on a test machine due to not checking the flags last night.

In D30908#696274, @hselasky wrote:

Answering my own question: Is the in_pcbref() supposed to prevent the structure for going away? Then it looks good.

Made ktls_ifnet_max_rexmit_pct unsigned as requested by Hans

Thanks for pointing out the dtor checks the flags already, thats a bummer. So this makes things no worse, except for some tests which i think is fine.

I think I'd prefer an m_free_raw(). I think that if we switched to m_free(), we'd wind up bringing in the first cache line of the mbuf (for the flags checks). If I'm wrong about that, and you see something else that will bring in that cache line already, then I'm fine with m_free.

Remove epoch until i'm certain its needed, it introduces a lot of complexity and at least ktls_set_tx_mode() does not require epoch (as it sleeps)

In D30908#695729, @hselasky wrote:

If you are not draining, then probably should protect all code with the network EPOCH at least.

Incorporated review feedback

In D30908#695725, @hselasky wrote:

Where are you draining the ktls_disable_ifnet_help() function?

Moved the sorele() to after the call into tfb_hwtls_change(), as the sorele() may end up calling pru_detach() in sofree. This could release TCP state, causing an invalid tcpcb, leading to a panic when trying to call into the tcp function block.

Thank you for upstreaming this!

Thanks so much for providing the correct fixQ

Thanks so much for doing this!

In D30094#676876, @stallamr_netapp.com wrote:

Also, the same comments regarding rx checksum __predict() as in the other 2 intel driver reviews

Same comments regarding __predict() as in the e1000 review.

In D29928#671554, @hselasky wrote:

In your patch you also need to remove tcp_lro_free() from under the IFCAP_LRO!

if (if_getcapabilities(ctx->ifc_ifp) & IFCAP_LRO)
        tcp_lro_free(&rxq->ifr_lc);

Now that we unconditionally allocate LRO resources, we must also unconditionally free them, as pointed out by Hans

In D29928#671538, @hselasky wrote:

It is also possible to add a check before calling into LRO functions like:

https://reviews.freebsd.org/D29900

--HPS

In D29663#665631, @rmacklem wrote:

I think hps@'s idea is a good one, if only
done when there are enough cpus.
Maybe something like:

if ncpus > 4
   - only allow ncpus - 1 to be assigned to

In D29663#665379, @hselasky wrote:

multi-queue NICs can consume all CPU

Why can't you reserve a CPU core for other stuff, and just let the N-1 CPUs handle whatever they can?

In D29663#665258, @hselasky wrote:

What about the netisr threads ?

In D29663#665198, @mav wrote:

My first thought about the high network priority was about serial ports without any hardware buffering and flow control, extremely sensitive to interrupt latency, but uart seems to use TTY priority, at least now. Not sure what else could be so time-critical in networking before, but sure there could be NICs with very few RX buffers. Typical disk hardware though should have much higher latency tolerance, since the amount of traffic is usually predetermined being initiated from the system, that is why I guess its priority was set lower.

iSCSI should not care about these priority levels, since at least for software iSCSI all of its receive/transmit is going through separate threads with lower (PVM) kernel priority. I don't remember right now how NFS client works, but I suspect something alike. iSCSI target and NFS server both run at even lower priority, so don't care about this at all.

Speaking about SWI, IIRC there is "intr/swi1: netisr 0". Not sure what it does these days, but you may wish to also check its priority relative to one of "intr/swi4: clock".

In D29564#664209, @hselasky wrote:

Hi Drew,

For sorted LRO the mbufs are still hot in the CPU cache. Remember we are flushing all the entries after each new flowid value!

--HPS

I may be reading this wrong, but for mbuf compression, you now seem to be appending a chain of packets, then coalescing them. The original design choice of coalescing as the new packet was encountered was intentional. The idea is that you want to operate on and free the incoming mbuf while it is hot in cache. By chaining mbufs and dealing with them later, they're likely to be cold in cache, so you're likely encounter additional cache misses on every mbuf in the chain as you copy out data and free it.