Nice work!!!

• Added a comment to why we enable debug stuff here as per mjg's request

I think pushing it and fixing the lagg issue after its in the tree is probably the best path forward.

The workflow I'm referring to is easily cleaning up after applying patches with git clean, using standard arguments.

Thank you!

In D33055#757882, @jrtc27 wrote:

In D33055#756416, @gallatin wrote:

In D33055#755988, @jrtc27 wrote:

Do you intend to commit this soon? If not I'm happy to commit it as Reported+Tested by you instead.

Sorry for the delay, I just pushed it.

Thanks. Will you be MFC'ing it (there's no MFC after: tag in your commit message)?

In D33395#756733, @tuexen wrote:

I like the second patch you suggested. How to proceed? Would you like to create a new review for your patch and I approve it or would you like me to update this review with your patch?

In D33395#756468, @tuexen wrote:

I like your approach much better than mine. Does your patch already handle the calling sequence:

oce_if_deactivate()
  oce_stop_rx()
    oce_rx_cq_clean()
      oce_rx_flush_lro()
        tcp_lro_flush_all()

Or don't we need to cover that?

In D33395#756461, @gallatin wrote:

Why not just fix the driver for epoch for real so that the normal, non LRO path holds epoch and we don't need to repeatedly enter it in ether_input()? I've tried to upload an alternate patch..

t.diff1 KBDownload

Why not just fix the driver for epoch for real so that the normal, non LRO path holds epoch and we don't need to repeatedly enter it in ether_input()? I've tried to upload an alternate patch..

In D33055#755988, @jrtc27 wrote:

Do you intend to commit this soon? If not I'm happy to commit it as Reported+Tested by you instead.

In D33055#751482, @jrtc27 wrote:

In D33055#751478, @gallatin wrote:

Any objection to me committing this change ?

Mind if I reword the description?

Any objection to me committing this change ?

OK, I'm happy to keep this as a separate change. I mostly just want to get this committed so I can bring it down into our (netflix) tree and not carry a diff going forward..

• keep image->pi_entry = eh.e_entry as requested by jrt27

I've updated the patch to

• use first_exec_segment as suggested by jrtc27
• set image->pi_entry = eh.e_entry as suggested by brett_gutste.in

I don't like that the drivers are dealing in ifps and calling things like if_getcapenable(). Establishing an interface that allowed drivers to have no knowledge of these details was one of the motivations for iflib.

Very nice.

In D32940#743788, @markj wrote:

In D32940#743764, @gallatin wrote:

We already have code in ip_output() to convert to normal mbuf chains in the case that checksums are off on an egress interface. Is there something special about bridges that they fall through the cracks here?

Sorry, I should have provided more background. My plan is to remove that code. See revisions D32941 and D32942 where I did this for sctp_delayed_cksum(). My point about if_bridge is just that it's not that unusual to have to fall back to software checksuming.

The problem is that there's still a number of places, e.g., in pf(4), which calculate checksums but are missing the corresponding mb_unmapped_to_ext() calls. Some of them are hard to fix since we have to modify several layers of code which currently don't expect errors (mb_unmapped_to_ext() can fail to allocate memory) and don't expect the mbuf chain to change (as happens if mb_unmapped_to_ext() is required). Rather than doing that, my idea was to instead modify the software checksum code to handle unmapped mbufs, using m_apply(). But this requires the direct map, hence this change.

I honestly don't care about anything other than amd64 and arm64 and wish we could remove all other architectures, so I don't want to stand in the way too much here. If you think this is the best direction to go in, then that's fine.

We already have code in ip_output() to convert to normal mbuf chains in the case that checksums are off on an egress interface. Is there something special about bridges that they fall through the cracks here?

To be honest, when I did early testing of extpgs, I found by far, hands down, the most improvement for sendfile use of extpgs on 32 bit platforms (x86), and the cases we're worried about (sw checksum) are corner cases that most people should never run into. So I'd prefer if it just defaulted to off on platforms without a direct map, but the user was allowed to force it.

I'm not a fan of this added complexity, but it looks like it was done in a very careful way. I really appreciate it being done when creating the session, rather than upon enqueue.

I agree that removing the indirection with a new SOCK_{SEND|RECVBUF}_LOCK() is a good idea.

I would do it in hn_nvs_handle_rxbuf(). Eg, take the epoch outside the for() loop, so it is held for all rx pkt reception.

In D31658#714086, @tuexen wrote:

Please take into account what gallatin@ says regarding performance impacts.

In D31648#713928, @tuexen wrote:

In D31648#713813, @hselasky wrote:

Why is this under #ifdef ? Anything inputting packets via if_input() needs the network epoch.

I put it under #ifdef, because the intention of this patch is only to move an NET_EPOCH_ASSERT() from an internal function to the beginning on functions which enter the LRO code and end up in calling the internal function.

If we agree on this plan, we can remove the #ifdef where the caller always should be in the network epoch.

In D31648#713927, @tuexen wrote:

In D31648#713901, @gallatin wrote:

Please no.

99% of drivers already have the network epoch via their ithread. For the few that don't, like the hyperv driver in the associated bug, the driver needs to take the network epoch. Let's please not penalize the 99% of drivers that already have the epoch for a few oddball drivers that don't use ithreads.

Now I'm confused. My intention was do enforce what you suggest: the drivers need to enter and leave the network epoch.

Are you saying that 99% of the driver which have the network epoch via their ithread result having it such that a NET_EPOCH_ASSERT() would fail? Is there any other way of testing?

I like this.

Address new feedback from jhb:

• Incorporated jhb's feedback
• fixed a typo in a comment
• changed alloc thread's name so that after truncation, its name appears different from the worker threads in 'ps axH'

Where is the new flag LRO_FLAG_DECRYPTED used? It seems to be write-only in this patch.

Addressed Mark's feedback.

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.