| Message ID | 16c37367670903e86f863cc8c481100dd4b3a323.1678364613.git.lorenzo@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 4d5ab0ad964df178beba031b89429a601893ff61 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | update xdp_features flag according to NIC re-configuration | expand |
On Thu, 9 Mar 2023 13:25:31 +0100 Lorenzo Bianconi wrote: > Take into account LRO and GRO configuration setting device xdp_features > flag. Consider channel rq_wq_type enabling rx scatter-gatter support in > xdp_features flag and disable NETDEV_XDP_ACT_NDO_XMIT_SG since it is not > supported yet by the driver. > Moreover always enable NETDEV_XDP_ACT_NDO_XMIT as the ndo_xdp_xmit > callback does not require to load a dummy xdp program on the NIC. > > Fixes: 66c0e13ad236 ("drivers: net: turn on XDP features") > Co-developed-by: Tariq Toukan <tariqt@nvidia.com> > Signed-off-by: Tariq Toukan <tariqt@nvidia.com> > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> This one hits ASSERT_RTNL(), I think. Don't we need something like: diff --git a/net/core/xdp.c b/net/core/xdp.c index 87e654b7d06c..5722a1fc6e9e 100644 --- a/net/core/xdp.c +++ b/net/core/xdp.c @@ -781,6 +781,9 @@ void xdp_set_features_flag(struct net_device *dev, xdp_features_t val) return; dev->xdp_features = val; + + if (dev->reg_state < NETREG_REGISTERED) + return; call_netdevice_notifiers(NETDEV_XDP_FEAT_CHANGE, dev); } EXPORT_SYMBOL_GPL(xdp_set_features_flag); ? The notifiers are not needed until the device is actually live.
On Wed, 15 Mar 2023 16:39:00 -0700 Jakub Kicinski wrote: > diff --git a/net/core/xdp.c b/net/core/xdp.c > index 87e654b7d06c..5722a1fc6e9e 100644 > --- a/net/core/xdp.c > +++ b/net/core/xdp.c > @@ -781,6 +781,9 @@ void xdp_set_features_flag(struct net_device *dev, xdp_features_t val) > return; > > dev->xdp_features = val; > + > + if (dev->reg_state < NETREG_REGISTERED) > + return; > call_netdevice_notifiers(NETDEV_XDP_FEAT_CHANGE, dev); > } > EXPORT_SYMBOL_GPL(xdp_set_features_flag); > > ? The notifiers are not needed until the device is actually live. I think so.. let me send a full patch.
On 15 Mar 17:29, Jakub Kicinski wrote: >On Wed, 15 Mar 2023 16:39:00 -0700 Jakub Kicinski wrote: >> diff --git a/net/core/xdp.c b/net/core/xdp.c >> index 87e654b7d06c..5722a1fc6e9e 100644 >> --- a/net/core/xdp.c >> +++ b/net/core/xdp.c >> @@ -781,6 +781,9 @@ void xdp_set_features_flag(struct net_device *dev, xdp_features_t val) >> return; >> >> dev->xdp_features = val; >> + >> + if (dev->reg_state < NETREG_REGISTERED) >> + return; >> call_netdevice_notifiers(NETDEV_XDP_FEAT_CHANGE, dev); >> } >> EXPORT_SYMBOL_GPL(xdp_set_features_flag); >> >> ? The notifiers are not needed until the device is actually live. > >I think so.. let me send a full patch. We have an internal version of a fix, Tariq is finalizing some review comments and we will be posting it ASAP.
On Wed, 15 Mar 2023 19:46:10 -0700 Saeed Mahameed wrote: > >I think so.. let me send a full patch. > > We have an internal version of a fix, Tariq is finalizing some review > comments and we will be posting it ASAP. Ah, I already posted. Does it look different? https://patchwork.kernel.org/project/netdevbpf/patch/20230316002903.492497-1-kuba@kernel.org/ I wanted to make sure that it's ready for tomorrow's PR
On 15 Mar 19:53, Jakub Kicinski wrote: >On Wed, 15 Mar 2023 19:46:10 -0700 Saeed Mahameed wrote: >> >I think so.. let me send a full patch. >> >> We have an internal version of a fix, Tariq is finalizing some review >> comments and we will be posting it ASAP. > >Ah, I already posted. Does it look different? > Yes, completely different, Tariq's fix is in mlx5 only. He splits the xdp feature setting into two functions, one to initialize the netdev's xdp before registration, and another one to update xpd features and call the notifier in the "after" registration set_features flows. I like our solution more, since it's more explicit and doesn't require patching xdp stack because mlx5 abused xdp_set_features_flag, unless other drivers have the same issue. >https://patchwork.kernel.org/project/netdevbpf/patch/20230316002903.492497-1-kuba@kernel.org/ > I don't see anything wrong with your patch though.. it also looks more elegant, i dunno, I will let you decide, here's our patch: https://git.kernel.org/pub/scm/linux/kernel/git/saeed/linux.git/commit/?h=testing/xdp-features-fix&id=72e2266525948ba1498e6a3f2d63ea10d5ee86f5 >I wanted to make sure that it's ready for tomorrow's PR
On Wed, 15 Mar 2023 22:03:27 -0700 Saeed Mahameed wrote: > Yes, completely different, Tariq's fix is in mlx5 only. > He splits the xdp feature setting into two functions, > one to initialize the netdev's xdp before registration, > and another one to update xpd features and call the notifier in the > "after" registration set_features flows. > > I like our solution more, since it's more explicit and doesn't require > patching xdp stack because mlx5 abused xdp_set_features_flag, unless other > drivers have the same issue. I reckon there's nothing wrong with calling xdp_set_features_flag() before registration, I didn't do much research, but netif_set_real_num_*x_queues() come to mind, and they can be called at any time. The stack should act accordingly. Many drivers may need to work around an issue which can be handled centrally. Let's see if Lorenzo disagrees. > >https://patchwork.kernel.org/project/netdevbpf/patch/20230316002903.492497-1-kuba@kernel.org/ > > > > I don't see anything wrong with your patch though.. it also looks more > elegant, i dunno, I will let you decide, here's our patch: > https://git.kernel.org/pub/scm/linux/kernel/git/saeed/linux.git/commit/?h=testing/xdp-features-fix&id=72e2266525948ba1498e6a3f2d63ea10d5ee86f5
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en.h b/drivers/net/ethernet/mellanox/mlx5/core/en.h index 88460b7796e5..4276c6eb6820 100644 --- a/drivers/net/ethernet/mellanox/mlx5/core/en.h +++ b/drivers/net/ethernet/mellanox/mlx5/core/en.h @@ -1243,6 +1243,7 @@ void mlx5e_build_nic_params(struct mlx5e_priv *priv, struct mlx5e_xsk *xsk, u16 void mlx5e_rx_dim_work(struct work_struct *work); void mlx5e_tx_dim_work(struct work_struct *work); +void mlx5e_set_xdp_feature(struct net_device *netdev); netdev_features_t mlx5e_features_check(struct sk_buff *skb, struct net_device *netdev, netdev_features_t features); diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c index 7708acc9b2ab..79fd21ecb9cb 100644 --- a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c @@ -1985,6 +1985,7 @@ static int set_pflag_rx_striding_rq(struct net_device *netdev, bool enable) struct mlx5e_priv *priv = netdev_priv(netdev); struct mlx5_core_dev *mdev = priv->mdev; struct mlx5e_params new_params; + int err; if (enable) { /* Checking the regular RQ here; mlx5e_validate_xsk_param called @@ -2005,7 +2006,14 @@ static int set_pflag_rx_striding_rq(struct net_device *netdev, bool enable) MLX5E_SET_PFLAG(&new_params, MLX5E_PFLAG_RX_STRIDING_RQ, enable); mlx5e_set_rq_type(mdev, &new_params); - return mlx5e_safe_switch_params(priv, &new_params, NULL, NULL, true); + err = mlx5e_safe_switch_params(priv, &new_params, NULL, NULL, true); + if (err) + return err; + + /* update XDP supported features */ + mlx5e_set_xdp_feature(netdev); + + return 0; } static int set_pflag_rx_no_csum_complete(struct net_device *netdev, bool enable) diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c index 76a9c5194a70..51b5f3cca504 100644 --- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c @@ -4004,6 +4004,25 @@ static int mlx5e_handle_feature(struct net_device *netdev, return 0; } +void mlx5e_set_xdp_feature(struct net_device *netdev) +{ + struct mlx5e_priv *priv = netdev_priv(netdev); + struct mlx5e_params *params = &priv->channels.params; + xdp_features_t val; + + if (params->packet_merge.type != MLX5E_PACKET_MERGE_NONE) { + xdp_clear_features_flag(netdev); + return; + } + + val = NETDEV_XDP_ACT_BASIC | NETDEV_XDP_ACT_REDIRECT | + NETDEV_XDP_ACT_XSK_ZEROCOPY | + NETDEV_XDP_ACT_NDO_XMIT; + if (params->rq_wq_type == MLX5_WQ_TYPE_CYCLIC) + val |= NETDEV_XDP_ACT_RX_SG; + xdp_set_features_flag(netdev, val); +} + int mlx5e_set_features(struct net_device *netdev, netdev_features_t features) { netdev_features_t oper_features = features; @@ -4030,6 +4049,9 @@ int mlx5e_set_features(struct net_device *netdev, netdev_features_t features) return -EINVAL; } + /* update XDP supported features */ + mlx5e_set_xdp_feature(netdev); + return 0; } @@ -4761,13 +4783,6 @@ static int mlx5e_xdp_set(struct net_device *netdev, struct bpf_prog *prog) if (old_prog) bpf_prog_put(old_prog); - if (reset) { - if (prog) - xdp_features_set_redirect_target(netdev, true); - else - xdp_features_clear_redirect_target(netdev); - } - if (!test_bit(MLX5E_STATE_OPENED, &priv->state) || reset) goto unlock; @@ -5163,13 +5178,10 @@ static void mlx5e_build_nic_netdev(struct net_device *netdev) netdev->features |= NETIF_F_HIGHDMA; netdev->features |= NETIF_F_HW_VLAN_STAG_FILTER; - netdev->xdp_features = NETDEV_XDP_ACT_BASIC | NETDEV_XDP_ACT_REDIRECT | - NETDEV_XDP_ACT_XSK_ZEROCOPY | - NETDEV_XDP_ACT_RX_SG; - netdev->priv_flags |= IFF_UNICAST_FLT; netif_set_tso_max_size(netdev, GSO_MAX_SIZE); + mlx5e_set_xdp_feature(netdev); mlx5e_set_netdev_dev_addr(netdev); mlx5e_macsec_build_netdev(priv); mlx5e_ipsec_build_netdev(priv); @@ -5241,6 +5253,9 @@ static int mlx5e_nic_init(struct mlx5_core_dev *mdev, mlx5_core_err(mdev, "TLS initialization failed, %d\n", err); mlx5e_health_create_reporters(priv); + /* update XDP supported features */ + mlx5e_set_xdp_feature(netdev); + return 0; } diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_rep.c b/drivers/net/ethernet/mellanox/mlx5/core/en_rep.c index 9b9203443085..43fd12fb87b8 100644 --- a/drivers/net/ethernet/mellanox/mlx5/core/en_rep.c +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_rep.c @@ -747,6 +747,9 @@ static void mlx5e_build_rep_params(struct net_device *netdev) /* RQ */ mlx5e_build_rq_params(mdev, params); + /* update XDP supported features */ + mlx5e_set_xdp_feature(netdev); + /* CQ moderation params */ params->rx_dim_enabled = MLX5_CAP_GEN(mdev, cq_moderation); mlx5e_set_rx_cq_mode_params(params, cq_period_mode);
Take into account LRO and GRO configuration setting device xdp_features flag. Consider channel rq_wq_type enabling rx scatter-gatter support in xdp_features flag and disable NETDEV_XDP_ACT_NDO_XMIT_SG since it is not supported yet by the driver. Moreover always enable NETDEV_XDP_ACT_NDO_XMIT as the ndo_xdp_xmit callback does not require to load a dummy xdp program on the NIC. Fixes: 66c0e13ad236 ("drivers: net: turn on XDP features") Co-developed-by: Tariq Toukan <tariqt@nvidia.com> Signed-off-by: Tariq Toukan <tariqt@nvidia.com> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> --- drivers/net/ethernet/mellanox/mlx5/core/en.h | 1 + .../ethernet/mellanox/mlx5/core/en_ethtool.c | 10 ++++- .../net/ethernet/mellanox/mlx5/core/en_main.c | 37 +++++++++++++------ .../net/ethernet/mellanox/mlx5/core/en_rep.c | 3 ++ 4 files changed, 39 insertions(+), 12 deletions(-)