| Message ID | 20250307110339.13788-1-rsalvaterra@gmail.com (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | igc: enable HW VLAN insertion/stripping by default | expand |
On Fri, Mar 07, 2025 at 11:02:39AM +0000, Rui Salvaterra wrote: > This is enabled by default in other Intel drivers I've checked (e1000, e1000e, > iavf, igb and ice). Fixes an out-of-the-box performance issue when running > OpenWrt on typical mini-PCs with igc-supported Ethernet controllers and 802.1Q > VLAN configurations, as ethtool isn't part of the default packages and sane > defaults are expected. > > In my specific case, with an Intel N100-based machine with four I226-V Ethernet > controllers, my upload performance increased from under 30 Mb/s to the expected > ~1 Gb/s. > > Signed-off-by: Rui Salvaterra <rsalvaterra@gmail.com> > --- > > This patch cost me two afternoons of network debugging, last weekend. Is there > any plausible reason why VLAN acceleration wasn't enabled by default for this > driver, specifically? Having looked over this I am also curious to know the answer to that question. This does seem to be the default for other Intel drivers (at least).
Hi, Simon, On Tue, 11 Mar 2025 at 13:52, Simon Horman <horms@kernel.org> wrote: > > Having looked over this I am also curious to know the answer to that question. > This does seem to be the default for other Intel drivers (at least). Well, r8169 also enables it, and RealTek controllers are used everywhere. :) Kind regards, Rui Salvaterra
On 3/11/2025 6:59 AM, Rui Salvaterra wrote: > Hi, Simon, > > On Tue, 11 Mar 2025 at 13:52, Simon Horman <horms@kernel.org> wrote: >> >> Having looked over this I am also curious to know the answer to that question. >> This does seem to be the default for other Intel drivers (at least). Unfortunately, I'm unable check with the original author or those involved at the time. From asking around it sounds like there may have been some initial issues when implementing this; my theory is this was off by default so that it would minimize the affects if additional issues were discovered. I see that you missed Intel Wired LAN (intel-wired-lan@lists.osuosl.org) on the patch. Could you resend with the list included? Also, if you could make it 'PATCH iwl-next' to target the Intel -next tree as that seems the appropriate tree. Thanks, Tony > Well, r8169 also enables it, and RealTek controllers are used everywhere. :) > > Kind regards, > Rui Salvaterra
Hi, Tony, On Wed, 12 Mar 2025 at 21:43, Tony Nguyen <anthony.l.nguyen@intel.com> wrote: > > Unfortunately, I'm unable check with the original author or those > involved at the time. From asking around it sounds like there may have > been some initial issues when implementing this; my theory is this was > off by default so that it would minimize the affects if additional > issues were discovered. I thought about that possibility, but in the end it didn't seem logical to me. If the feature was WIP and/or broken in some way, why would the user be allowed to enable it via ethtool, ever since it was first implemented, in commit 8d7449630e3450bc0546dc0cb692fbb57d1852c0 (almost four years ago)? > I see that you missed Intel Wired LAN (intel-wired-lan@lists.osuosl.org) > on the patch. Could you resend with the list included? Also, if you > could make it 'PATCH iwl-next' to target the Intel -next tree as that > seems the appropriate tree. Oh. That list is marked as moderated, I (wrongly, for sure) assumed there would be restrictions when mailing to it. I'll resend correctly soon. Kind regards, Rui Salvaterra
On 3/13/2025 2:23 AM, Rui Salvaterra wrote: > Hi, Tony, > > On Wed, 12 Mar 2025 at 21:43, Tony Nguyen <anthony.l.nguyen@intel.com> wrote: >> >> Unfortunately, I'm unable check with the original author or those >> involved at the time. From asking around it sounds like there may have >> been some initial issues when implementing this; my theory is this was >> off by default so that it would minimize the affects if additional >> issues were discovered. > > I thought about that possibility, but in the end it didn't seem > logical to me. If the feature was WIP and/or broken in some way, why > would the user be allowed to enable it via ethtool, ever since it was > first implemented, in commit 8d7449630e3450bc0546dc0cb692fbb57d1852c0 > (almost four years ago)? It would allow users to opt-in and use the feature; if an adverse problem was later found, it could be easily mitigated by not turning the feature on. If it were on by default, and a problem found, there would a larger impact. As you mentioned, since the feature has now been in existence for many years, it's probably safe to have on by default. >> I see that you missed Intel Wired LAN (intel-wired-lan@lists.osuosl.org) >> on the patch. Could you resend with the list included? Also, if you >> could make it 'PATCH iwl-next' to target the Intel -next tree as that >> seems the appropriate tree. > > Oh. That list is marked as moderated, I (wrongly, for sure) assumed > there would be restrictions when mailing to it. I'll resend correctly > soon. Thank you. - Tony
diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c index 84307bb7313e..6fef763239bc 100644 --- a/drivers/net/ethernet/intel/igc/igc_main.c +++ b/drivers/net/ethernet/intel/igc/igc_main.c @@ -7049,6 +7049,9 @@ static int igc_probe(struct pci_dev *pdev, netdev->xdp_features = NETDEV_XDP_ACT_BASIC | NETDEV_XDP_ACT_REDIRECT | NETDEV_XDP_ACT_XSK_ZEROCOPY; + /* enable hardware VLAN insertion/stripping by default */ + netdev->features |= NETIF_F_HW_VLAN_CTAG_TX | NETIF_F_HW_VLAN_CTAG_RX; + /* MTU range: 68 - 9216 */ netdev->min_mtu = ETH_MIN_MTU; netdev->max_mtu = MAX_STD_JUMBO_FRAME_SIZE;
This is enabled by default in other Intel drivers I've checked (e1000, e1000e, iavf, igb and ice). Fixes an out-of-the-box performance issue when running OpenWrt on typical mini-PCs with igc-supported Ethernet controllers and 802.1Q VLAN configurations, as ethtool isn't part of the default packages and sane defaults are expected. In my specific case, with an Intel N100-based machine with four I226-V Ethernet controllers, my upload performance increased from under 30 Mb/s to the expected ~1 Gb/s. Signed-off-by: Rui Salvaterra <rsalvaterra@gmail.com> --- This patch cost me two afternoons of network debugging, last weekend. Is there any plausible reason why VLAN acceleration wasn't enabled by default for this driver, specifically? drivers/net/ethernet/intel/igc/igc_main.c | 3 +++ 1 file changed, 3 insertions(+)