Details

Reviewers

gallatin
kbowling
markj
erj
shurd

Group Reviewers

iflib

Commits

rGcf101bd5ceeb: net: iflib: fix vlan processing in the drivers
rGa61341bf6e51: net: iflib: let the drivers use isc_capenable
rG05c9fb008763: net: iflib: fix vlan processing in the drivers
rGe99828dfbdd7: net: iflib: let the drivers use isc_capenable
rG5df59718ed52: net: iflib: sync isc_capenable to if_capenable
rG6000a417d32a: net: iflib: sync isc_capenable to if_capenable
rGf7926a6d0c10: net: iflib: fix vlan processing in the drivers
rG52f45d8acee9: net: iflib: let the drivers use isc_capenable
rG4561c4f0ca59: net: iflib: sync isc_capenable to if_capenable

Summary

The logic that sets iri_vtag and M_VLANTAG does not handle the case where the vtag is 0 (and IPI
The iflib ift_rxd_pkt_get() routine includes processing for the
IFCAP_RXCSUM and IFCAP_VLAN_HWTAGGING offloads. This change
rearranges the igb processing to be more similar to the other
Intel drivers.

Also see
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=260068

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

vmaffione requested review of this revision.Nov 28 2021, 10:16 PM

vmaffione created this revision.

Use iri_ifp rather than iflib_get_ifp().

vmaffione added a reviewer: markj.Dec 2 2021, 10:20 PM

vmaffione mentioned this in D33154: e1000: fix interface capabilities management.Dec 2 2021, 10:26 PM

erj requested changes to this revision.Dec 2 2021, 10:35 PM

erj added a subscriber: erj.

erj added inline comments.

sys/dev/e1000/igb_txrx.c
501 ↗	(On Diff #99376)	This is a subtle bug that I caught in another of our drivers, but you shouldn't just check if the vtag is non-zero; that improperly strips the packet of a possible priority 0 & VLAN 0-tagged header. You should still check the descriptor see if a tag exists.

This revision now requires changes to proceed.Dec 2 2021, 10:35 PM

vmaffione added inline comments.Dec 8 2021, 9:28 AM

sys/dev/e1000/igb_txrx.c
501 ↗	(On Diff #99376)	Do you mean checking for `(staterr & E1000_RXD_STAT_VP)`? That is done in the loop above, to avoid possible issues with multi-descriptor packets. Or what should I do?

erj added inline comments.Dec 9 2021, 9:12 PM

sys/dev/e1000/igb_txrx.c
501 ↗	(On Diff #99376)	Sorry, maybe it wasn't completely clear in my last comment, but you need to change the check from `if there is a non-zero VLAN tag` to `if a VLAN tag was detected` because it's possible that there is a VLAN tag that is just all 0's and we need to still inform the upper layers that it's there with `M_VLANTAG` -- one way to do that would be to copy that `(staterr & E1000_RXD_STAT_VP)` check down, and replace the `if (vtag)` highlighted here with it.

That's right, thanks. The same issue is there for at least em and ixgbe.
I can fix those if you like.

Herald added a subscriber: imp. · View Herald TranscriptDec 18 2021, 11:54 AM

vmaffione marked an inline comment as done.Dec 18 2021, 11:56 AM

vmaffione added inline comments.

sys/dev/e1000/igb_txrx.c
501 ↗	(On Diff #99376)	I see, thanks. Btw, I was also worried about multi-descriptor packets: I expect the "vtag" to be present in the descriptor that contains the E1000_RXD_STAT_VP bit (staterr), but not in the other packet fragments. This means that it may be wrong to check for the VP bit outside of the loop.

Fix the iri_vtag setting logic for all the iflib drivers.

Herald added a subscriber: ae. · View Herald TranscriptDec 18 2021, 4:40 PM

Also fix scctx->isc_capenable and isc_capabilities usage in igc.

vmaffione edited the summary of this revision. (Show Details)Dec 18 2021, 4:52 PM

vmaffione added inline comments.

sys/dev/e1000/igb_txrx.c
501 ↗	(On Diff #99376)	Never mind. The VLAN info (RXD_STAT_VP bit and vtag) is set in all the descriptors of a multi-descriptor packet. So it is safe to access those info outside of the loop. I tried to fix the issue for all the iflib drivers.

afedorov added a subscriber: afedorov.Dec 18 2021, 7:50 PM

I'm less optimistic about the ifp seeing the change required in all drivers, @gallatin odes anyone have any idea on the impact of the ifp pointer chase and fetch versus grabbing it from the already fetched scctx in these hot paths? How would we sync scctx_capenable with ifnet, that seems to be a critical missing implementation from the outset.

Note that this patch contains a number of changes that fix separate issues:

fix VLAN offload processing handling the case of vtag == 0.
use if_getcapenable() rather the isc_capenable because the latter is not synchronized.
use iri_ifp rather than iflib_get_ifp() to avoid an additional indirection.

Those changes could/should be committed separately.
Regarding point (2), I agree with you that we should avoid the additional indirection by accessing isc_capenable. But the current status is that we have some drivers that use isc_capenable (and are therefore broken) and some drivers that use iflib_get_ifp() and are therefore correct although suboptimal. I would then propose to make the change in two steps. The first step is to restore correctness (this patch), as soon as possible, since there is no point in having "fast broken" code. Then we can have a second step where we properly synchronize isc_capenable and then change back all the drivers to access that.

use iri_ifp rather than iflib_get_ifp() in ice and ixgbe.

vmaffione marked an inline comment as done.Dec 19 2021, 8:45 AM

...or we can also switch to isc_capenable in the same changeset.
The last diff tweaks iflib's ioctl handler to synchronize isc_capenable to if_capenable.

Herald added a reviewer: shurd. · View Herald TranscriptDec 19 2021, 1:51 PM

Herald added a reviewer: iflib. · View Herald Transcript

Herald added subscribers: glebius, melifaro. · View Herald Transcript

kbowling accepted this revision.Dec 20 2021, 10:34 AM

Any comments from @erj or @gallatin ?

gallatin accepted this revision.Dec 27 2021, 2:30 PM

This revision was not accepted when it landed; it landed in state Needs Review.Dec 28 2021, 10:56 AM

Closed by commit rG4561c4f0ca59: net: iflib: sync isc_capenable to if_capenable (authored by vmaffione). · Explain Why

This revision was automatically updated to reflect the committed changes.

vmaffione added a commit: rG4561c4f0ca59: net: iflib: sync isc_capenable to if_capenable.

vmaffione added a commit: rG52f45d8acee9: net: iflib: let the drivers use isc_capenable.Dec 28 2021, 11:04 AM

vmaffione added a commit: rGf7926a6d0c10: net: iflib: fix vlan processing in the drivers.Dec 28 2021, 11:13 AM