| Message ID | 38fa0a328351ba9283ecda2ba126d1147379416c.1666793468.git.sd@queasysnail.net (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | macsec: offload-related fixes | expand |
On Wed, Oct 26, 2022 at 11:56:23PM +0200, Sabrina Dubroca wrote: > This reverts commit c850240b6c4132574a00f2da439277ab94265b66. > > That commit tried to improve the performance of macsec offload by > taking advantage of some of the NIC's features, but in doing so, broke > macsec offload when the lower device supports both macsec and ipsec > offload, as the ipsec offload feature flags (mainly NETIF_F_HW_ESP) > were copied from the real device. Since the macsec device doesn't > provide xdo_* ops, the XFRM core rejects the registration of the new > macsec device in xfrm_api_check. > > Example perf trace when running > ip link add link eni1np1 type macsec port 4 offload mac > > ip 737 [003] 795.477676: probe:xfrm_dev_event__REGISTER name="macsec0" features=0x1c000080014869 > xfrm_dev_event+0x3a > notifier_call_chain+0x47 > register_netdevice+0x846 > macsec_newlink+0x25a > > ip 737 [003] 795.477687: probe:xfrm_dev_event__return ret=0x8002 (NOTIFY_BAD) > notifier_call_chain+0x47 > register_netdevice+0x846 > macsec_newlink+0x25a > > dev->features includes NETIF_F_HW_ESP (0x04000000000000), so > xfrm_api_check returns NOTIFY_BAD because we don't have > dev->xfrmdev_ops on the macsec device. > > We could probably propagate GSO and a few other features from the > lower device, similar to macvlan. This will be done in a future patch. > > Signed-off-by: Sabrina Dubroca <sd@queasysnail.net> > --- > drivers/net/macsec.c | 27 ++++----------------------- > 1 file changed, 4 insertions(+), 23 deletions(-) > It is still mystery for me why mlx5 works. Thanks, Reviewed-by: Leon Romanovsky <leonro@nvidia.com>
>On Wed, Oct 26, 2022 at 11:56:23PM +0200, Sabrina Dubroca wrote: >> This reverts commit c850240b6c4132574a00f2da439277ab94265b66. >> >> That commit tried to improve the performance of macsec offload by >> taking advantage of some of the NIC's features, but in doing so, broke >> macsec offload when the lower device supports both macsec and ipsec >> offload, as the ipsec offload feature flags (mainly NETIF_F_HW_ESP) >> were copied from the real device. Since the macsec device doesn't >> provide xdo_* ops, the XFRM core rejects the registration of the new >> macsec device in xfrm_api_check. >> >> Example perf trace when running >> ip link add link eni1np1 type macsec port 4 offload mac >> >> ip 737 [003] 795.477676: probe:xfrm_dev_event__REGISTER >name="macsec0" features=0x1c000080014869 >> xfrm_dev_event+0x3a >> notifier_call_chain+0x47 >> register_netdevice+0x846 >> macsec_newlink+0x25a >> >> ip 737 [003] 795.477687: probe:xfrm_dev_event__return ret=0x8002 >(NOTIFY_BAD) >> notifier_call_chain+0x47 >> register_netdevice+0x846 >> macsec_newlink+0x25a >> >> dev->features includes NETIF_F_HW_ESP (0x04000000000000), so >> xfrm_api_check returns NOTIFY_BAD because we don't have >> dev->xfrmdev_ops on the macsec device. >> >> We could probably propagate GSO and a few other features from the >> lower device, similar to macvlan. This will be done in a future patch. >> >> Signed-off-by: Sabrina Dubroca <sd@queasysnail.net> >> --- >> drivers/net/macsec.c | 27 ++++----------------------- >> 1 file changed, 4 insertions(+), 23 deletions(-) >> > >It is still mystery for me why mlx5 works. I think it works when the offload enablement on the macsec device is done after the macsec netdev creation stage i.e.: Ip link add link eni1np1 type macsec port 4 Then ip macsec offload $MACSEC_IF mac I think this path skips xfrm core rejection > >Thanks, >Reviewed-by: Leon Romanovsky <leonro@nvidia.com>
>>On Wed, Oct 26, 2022 at 11:56:23PM +0200, Sabrina Dubroca wrote: >>> This reverts commit c850240b6c4132574a00f2da439277ab94265b66. >>> >>> That commit tried to improve the performance of macsec offload by >>> taking advantage of some of the NIC's features, but in doing so, >>> broke macsec offload when the lower device supports both macsec and >>> ipsec offload, as the ipsec offload feature flags (mainly >>> NETIF_F_HW_ESP) were copied from the real device. Since the macsec >>> device doesn't provide xdo_* ops, the XFRM core rejects the >>> registration of the new macsec device in xfrm_api_check. >>> >>> Example perf trace when running >>> ip link add link eni1np1 type macsec port 4 offload mac >>> >>> ip 737 [003] 795.477676: probe:xfrm_dev_event__REGISTER >>name="macsec0" features=0x1c000080014869 >>> xfrm_dev_event+0x3a >>> notifier_call_chain+0x47 >>> register_netdevice+0x846 >>> macsec_newlink+0x25a >>> >>> ip 737 [003] 795.477687: probe:xfrm_dev_event__return >ret=0x8002 >>(NOTIFY_BAD) >>> notifier_call_chain+0x47 >>> register_netdevice+0x846 >>> macsec_newlink+0x25a >>> >>> dev->features includes NETIF_F_HW_ESP (0x04000000000000), so >>> xfrm_api_check returns NOTIFY_BAD because we don't have >>> dev->xfrmdev_ops on the macsec device. >>> >>> We could probably propagate GSO and a few other features from the >>> lower device, similar to macvlan. This will be done in a future patch. >>> >>> Signed-off-by: Sabrina Dubroca <sd@queasysnail.net> >>> --- >>> drivers/net/macsec.c | 27 ++++----------------------- >>> 1 file changed, 4 insertions(+), 23 deletions(-) >>> >> >>It is still mystery for me why mlx5 works. >I think it works when the offload enablement on the macsec device is done >after the macsec netdev creation stage i.e.: >Ip link add link eni1np1 type macsec port 4 Then ip macsec offload >$MACSEC_IF mac I think this path skips xfrm core rejection Further check it shows that xfrm_api_check fails in both configuration flavors, in first method because of register_netdevice call the second method because of netdev_update_features call, however, using the second method (that "works") masks this error so user space is not aware of it, hence why the second method works. >> >>Thanks, >>Reviewed-by: Leon Romanovsky <leonro@nvidia.com>
diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c index c891b60937a7..b3f76e8071f2 100644 --- a/drivers/net/macsec.c +++ b/drivers/net/macsec.c @@ -2654,11 +2654,6 @@ static int macsec_upd_offload(struct sk_buff *skb, struct genl_info *info) if (ret) goto rollback; - /* Force features update, since they are different for SW MACSec and - * HW offloading cases. - */ - netdev_update_features(dev); - rtnl_unlock(); return 0; @@ -3432,16 +3427,9 @@ static netdev_tx_t macsec_start_xmit(struct sk_buff *skb, return ret; } -#define SW_MACSEC_FEATURES \ +#define MACSEC_FEATURES \ (NETIF_F_SG | NETIF_F_HIGHDMA | NETIF_F_FRAGLIST) -/* If h/w offloading is enabled, use real device features save for - * VLAN_FEATURES - they require additional ops - * HW_MACSEC - no reason to report it - */ -#define REAL_DEV_FEATURES(dev) \ - ((dev)->features & ~(NETIF_F_VLAN_FEATURES | NETIF_F_HW_MACSEC)) - static int macsec_dev_init(struct net_device *dev) { struct macsec_dev *macsec = macsec_priv(dev); @@ -3458,12 +3446,8 @@ static int macsec_dev_init(struct net_device *dev) return err; } - if (macsec_is_offloaded(macsec)) { - dev->features = REAL_DEV_FEATURES(real_dev); - } else { - dev->features = real_dev->features & SW_MACSEC_FEATURES; - dev->features |= NETIF_F_LLTX | NETIF_F_GSO_SOFTWARE; - } + dev->features = real_dev->features & MACSEC_FEATURES; + dev->features |= NETIF_F_LLTX | NETIF_F_GSO_SOFTWARE; dev->needed_headroom = real_dev->needed_headroom + MACSEC_NEEDED_HEADROOM; @@ -3495,10 +3479,7 @@ static netdev_features_t macsec_fix_features(struct net_device *dev, struct macsec_dev *macsec = macsec_priv(dev); struct net_device *real_dev = macsec->real_dev; - if (macsec_is_offloaded(macsec)) - return REAL_DEV_FEATURES(real_dev); - - features &= (real_dev->features & SW_MACSEC_FEATURES) | + features &= (real_dev->features & MACSEC_FEATURES) | NETIF_F_GSO_SOFTWARE | NETIF_F_SOFT_FEATURES; features |= NETIF_F_LLTX;
This reverts commit c850240b6c4132574a00f2da439277ab94265b66. That commit tried to improve the performance of macsec offload by taking advantage of some of the NIC's features, but in doing so, broke macsec offload when the lower device supports both macsec and ipsec offload, as the ipsec offload feature flags (mainly NETIF_F_HW_ESP) were copied from the real device. Since the macsec device doesn't provide xdo_* ops, the XFRM core rejects the registration of the new macsec device in xfrm_api_check. Example perf trace when running ip link add link eni1np1 type macsec port 4 offload mac ip 737 [003] 795.477676: probe:xfrm_dev_event__REGISTER name="macsec0" features=0x1c000080014869 xfrm_dev_event+0x3a notifier_call_chain+0x47 register_netdevice+0x846 macsec_newlink+0x25a ip 737 [003] 795.477687: probe:xfrm_dev_event__return ret=0x8002 (NOTIFY_BAD) notifier_call_chain+0x47 register_netdevice+0x846 macsec_newlink+0x25a dev->features includes NETIF_F_HW_ESP (0x04000000000000), so xfrm_api_check returns NOTIFY_BAD because we don't have dev->xfrmdev_ops on the macsec device. We could probably propagate GSO and a few other features from the lower device, similar to macvlan. This will be done in a future patch. Signed-off-by: Sabrina Dubroca <sd@queasysnail.net> --- drivers/net/macsec.c | 27 ++++----------------------- 1 file changed, 4 insertions(+), 23 deletions(-)