iflib: Discern when to reset on isc_rxd_pkt_get errors

[kbowling]

Summary

erj@ provides a good analysis in the PR.. I checked the various iflib drivers and the only error cases are Intel drivers for transient receive issues or mgb which uses EINVAL for contractual enforcement of its descriptors.

I think we have some dealer's choice here.. we could return 0 for the intel isc_rxd_pkt_get instead of EBADMSG or discern EBADMSG as a transient issue and consider anything else a contractual issue as I am proposing here.

PR: 262024

[markj]

sys/net/iflib.c

3100

Why not patch drivers to not return an error for that case? It looks like iflib is not really doing anything with EBADMSG except returning early here.

Also, looking at em_isc_rxd_pkt_get(), is there some missing cleanup in the error path? It's not really clear to me how the driver is supposed to recover.

[erj]

sys/net/iflib.c

3100

It doesn't look like there's any critical cleanup for em_isc_rxd_pkt_get() to do in that error case; all the other code in there after that just manipulates local variables or the packet info struct (which iflib could maybe choose to modify or do something with when it sees EBADMSG).

I don't know the specifics of em hardware, but I'd think that in theory the hardware would be fine just moving onto the next packet; I don't think it stops or needs some special routine to happen if it gets a bad packet.

But as for the error code, yeah, it doesn't currently functionally make sense to treat that error specially if nothing special happens as a result; returning 0 would be ok. Though, I guess keeping the error code would help with writing unit test code for this function. :p

[markj]

sys/net/iflib.c

3100

all the other code in there after that just manipulates local variables or the packet info struct

I was wondering specifically about:

/* Zero out the receive descriptors status. */
rxd->wb.upper.status_error &= htole32(~0xFF);

Do we need to do that in the error case as well?

[erj]

sys/net/iflib.c

3100

I'm not sure. We do clear those bits in the newer drivers, too, but it's not clear if that's a holdover from the legacy em to prevent processing the same descriptors multiple times in one function call/loop or if that's something that's necessary to keep things working normally.

[kbowling]

I've been pondering this and it seems pretty broken in the tree and what I have suggest doesn't make sense.

In the driver, we should clear the error from the driver's descriptor as @markj suggests and then in iflib it seems like we are missing an error path that would advance the queue without calling the upper network layers. It seems all generations of intel drivers are currently subjected to a reset on any layer 1 (serdes/phy) or layer 2 (framing) errors which does not seem to be the intent of the HW designers. This would be triggered as sundry as a collision on classical shared Ethernet or a CRC error. On e1000 and igc we are saved by setting RCTL.SBP (Show Bad Packets) to disabled by default now which filters these out in hardware. I haven't looked at what the newer parts do but assume they have similar HW filters by default.