| Message ID | 20240803042624.970352-7-kuba@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | ethtool: rss: driver tweaks and netlink context dumps | expand |
On 03/08/2024 05:26, Jakub Kicinski wrote: > marvell/otx2 and mvpp2 do not support setting different > keys for different RSS contexts. Contexts have separate > indirection tables but key is shared with all other contexts. > This is likely fine, indirection table is the most important > piece. Since drivers that do not support this are the odd ones out, would it be better to invert the sense of the flag? Or is this to make sure that driver authors who don't think/know about the distinction automatically get safe behaviour? > Don't report the key-related parameters from such drivers. > This prevents driver-errors, e.g. otx2 always writes > the main key, even when user asks to change per-context key. > The second reason is that without this change tracking > the keys by the core gets complicated. Even if the driver > correctly reject setting key with rss_context != 0, > change of the main key would have to be reflected in > the XArray for all additional contexts. > > Since the additional contexts don't have their own keys > not including the attributes (in Netlink speak) seems > intuitive. ethtool CLI seems to deal with it just fine. > > Reviewed-by: Joe Damato <jdamato@fastly.com> > Signed-off-by: Jakub Kicinski <kuba@kernel.org> ... > diff --git a/drivers/net/ethernet/sfc/ef100_ethtool.c b/drivers/net/ethernet/sfc/ef100_ethtool.c > index 746b5314acb5..127b9d6ade6f 100644 > --- a/drivers/net/ethernet/sfc/ef100_ethtool.c > +++ b/drivers/net/ethernet/sfc/ef100_ethtool.c > @@ -58,6 +58,7 @@ const struct ethtool_ops ef100_ethtool_ops = { > > .get_rxfh_indir_size = efx_ethtool_get_rxfh_indir_size, > .get_rxfh_key_size = efx_ethtool_get_rxfh_key_size, > + .rxfh_per_ctx_key = 1, I would prefer 'true' for the sfc drivers, I think that better fits the general style of our code. > .rxfh_priv_size = sizeof(struct efx_rss_context_priv), > .get_rxfh = efx_ethtool_get_rxfh, > .set_rxfh = efx_ethtool_set_rxfh, > diff --git a/drivers/net/ethernet/sfc/ethtool.c b/drivers/net/ethernet/sfc/ethtool.c > index 15245720c949..e4d86123b797 100644 > --- a/drivers/net/ethernet/sfc/ethtool.c > +++ b/drivers/net/ethernet/sfc/ethtool.c > @@ -267,6 +267,7 @@ const struct ethtool_ops efx_ethtool_ops = { > .set_rxnfc = efx_ethtool_set_rxnfc, > .get_rxfh_indir_size = efx_ethtool_get_rxfh_indir_size, > .get_rxfh_key_size = efx_ethtool_get_rxfh_key_size, > + .rxfh_per_ctx_key = 1, > .rxfh_priv_size = sizeof(struct efx_rss_context_priv), > .get_rxfh = efx_ethtool_get_rxfh, > .set_rxfh = efx_ethtool_set_rxfh, > diff --git a/drivers/net/ethernet/sfc/siena/ethtool.c b/drivers/net/ethernet/sfc/siena/ethtool.c > index 4c182d4edfc2..6d4e5101433a 100644 > --- a/drivers/net/ethernet/sfc/siena/ethtool.c > +++ b/drivers/net/ethernet/sfc/siena/ethtool.c > @@ -241,6 +241,7 @@ static int efx_ethtool_get_ts_info(struct net_device *net_dev, > > const struct ethtool_ops efx_siena_ethtool_ops = { > .cap_rss_ctx_supported = true, > + .rxfh_per_ctx_key = true, For the record, Siena hardware doesn't actually support custom RSS contexts; the code is only present in the driver as a holdover from when Siena and EF10 used the same driver. Trying to actually use them on Siena will fail -EOPNOTSUPP.[1] I'll send a patch to rip it out. > .supported_coalesce_params = ETHTOOL_COALESCE_USECS | > ETHTOOL_COALESCE_USECS_IRQ | > ETHTOOL_COALESCE_USE_ADAPTIVE_RX, > diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h > index 55c9f613ab64..16f72a556fe9 100644 > --- a/include/linux/ethtool.h > +++ b/include/linux/ethtool.h > @@ -731,6 +731,8 @@ struct kernel_ethtool_ts_info { > * do not have to set this bit. > * @cap_rss_sym_xor_supported: indicates if the driver supports symmetric-xor > * RSS. > + * @rxfh_per_ctx_key: device supports setting different RSS key for each > + * additional context. This comment should really make clear that it covers hfunc and input_xfrm as well, not just the key itself. -ed [1]: https://elixir.bootlin.com/linux/v6.10.3/source/drivers/net/ethernet/sfc/siena/ethtool_common.c#L1234
[found this stuck in my outgoing mail :S] On Mon, 5 Aug 2024 15:36:28 +0100 Edward Cree wrote: > On 03/08/2024 05:26, Jakub Kicinski wrote: > > marvell/otx2 and mvpp2 do not support setting different > > keys for different RSS contexts. Contexts have separate > > indirection tables but key is shared with all other contexts. > > This is likely fine, indirection table is the most important > > piece. > > Since drivers that do not support this are the odd ones out, > would it be better to invert the sense of the flag? Or is > this to make sure that driver authors who don't think/know > about the distinction automatically get safe behaviour? Yes, I wanted the 0 / default / sloppy choice to be the safe one. As annoying as it is to have to set it in most drivers, I still prefer that to the inevitable false-negatives. > > Don't report the key-related parameters from such drivers. > > This prevents driver-errors, e.g. otx2 always writes > > the main key, even when user asks to change per-context key. > > The second reason is that without this change tracking > > the keys by the core gets complicated. Even if the driver > > correctly reject setting key with rss_context != 0, > > change of the main key would have to be reflected in > > the XArray for all additional contexts. > > > > Since the additional contexts don't have their own keys > > not including the attributes (in Netlink speak) seems > > intuitive. ethtool CLI seems to deal with it just fine. > > > > Reviewed-by: Joe Damato <jdamato@fastly.com> > > Signed-off-by: Jakub Kicinski <kuba@kernel.org> > ... > > diff --git a/drivers/net/ethernet/sfc/ef100_ethtool.c b/drivers/net/ethernet/sfc/ef100_ethtool.c > > index 746b5314acb5..127b9d6ade6f 100644 > > --- a/drivers/net/ethernet/sfc/ef100_ethtool.c > > +++ b/drivers/net/ethernet/sfc/ef100_ethtool.c > > @@ -58,6 +58,7 @@ const struct ethtool_ops ef100_ethtool_ops = { > > > > .get_rxfh_indir_size = efx_ethtool_get_rxfh_indir_size, > > .get_rxfh_key_size = efx_ethtool_get_rxfh_key_size, > > + .rxfh_per_ctx_key = 1, > > I would prefer 'true' for the sfc drivers, I think that > better fits the general style of our code. Sure thing. > > .rxfh_priv_size = sizeof(struct efx_rss_context_priv), > > .get_rxfh = efx_ethtool_get_rxfh, > > .set_rxfh = efx_ethtool_set_rxfh, > > diff --git a/drivers/net/ethernet/sfc/siena/ethtool.c b/drivers/net/ethernet/sfc/siena/ethtool.c > > index 4c182d4edfc2..6d4e5101433a 100644 > > --- a/drivers/net/ethernet/sfc/siena/ethtool.c > > +++ b/drivers/net/ethernet/sfc/siena/ethtool.c > > @@ -241,6 +241,7 @@ static int efx_ethtool_get_ts_info(struct net_device *net_dev, > > > > const struct ethtool_ops efx_siena_ethtool_ops = { > > .cap_rss_ctx_supported = true, > > + .rxfh_per_ctx_key = true, > > For the record, Siena hardware doesn't actually support > custom RSS contexts; the code is only present in the > driver as a holdover from when Siena and EF10 used the > same driver. Trying to actually use them on Siena will > fail -EOPNOTSUPP.[1] > I'll send a patch to rip it out. Ack, will drop this chunk to avoid conflicts, then. > > .supported_coalesce_params = ETHTOOL_COALESCE_USECS | > > ETHTOOL_COALESCE_USECS_IRQ | > > ETHTOOL_COALESCE_USE_ADAPTIVE_RX, > > diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h > > index 55c9f613ab64..16f72a556fe9 100644 > > --- a/include/linux/ethtool.h > > +++ b/include/linux/ethtool.h > > @@ -731,6 +731,8 @@ struct kernel_ethtool_ts_info { > > * do not have to set this bit. > > * @cap_rss_sym_xor_supported: indicates if the driver supports symmetric-xor > > * RSS. > > + * @rxfh_per_ctx_key: device supports setting different RSS key for each > > + * additional context. > > This comment should really make clear that it covers hfunc and > input_xfrm as well, not just the key itself. Ack.
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c index 33e8cf0a3764..77621ccfff5e 100644 --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c @@ -5289,6 +5289,7 @@ void bnxt_ethtool_free(struct bnxt *bp) const struct ethtool_ops bnxt_ethtool_ops = { .cap_link_lanes_supported = 1, + .rxfh_per_ctx_key = 1, .rxfh_max_context_id = BNXT_MAX_ETH_RSS_CTX, .rxfh_indir_space = BNXT_MAX_RSS_TABLE_ENTRIES_P5, .rxfh_priv_size = sizeof(struct bnxt_rss_ctx), diff --git a/drivers/net/ethernet/intel/ice/ice_ethtool.c b/drivers/net/ethernet/intel/ice/ice_ethtool.c index 8c990c976132..b5b57926cdc0 100644 --- a/drivers/net/ethernet/intel/ice/ice_ethtool.c +++ b/drivers/net/ethernet/intel/ice/ice_ethtool.c @@ -4725,6 +4725,7 @@ static const struct ethtool_ops ice_ethtool_ops = { ETHTOOL_COALESCE_USE_ADAPTIVE | ETHTOOL_COALESCE_RX_USECS_HIGH, .cap_rss_sym_xor_supported = true, + .rxfh_per_ctx_key = true, .get_link_ksettings = ice_get_link_ksettings, .set_link_ksettings = ice_set_link_ksettings, .get_fec_stats = ice_get_fec_stats, diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c index 0b941482db30..2d514210aaec 100644 --- a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c @@ -2593,6 +2593,7 @@ static void mlx5e_get_ts_stats(struct net_device *netdev, const struct ethtool_ops mlx5e_ethtool_ops = { .cap_rss_ctx_supported = true, + .rxfh_per_ctx_key = true, .supported_coalesce_params = ETHTOOL_COALESCE_USECS | ETHTOOL_COALESCE_MAX_FRAMES | ETHTOOL_COALESCE_USE_ADAPTIVE | diff --git a/drivers/net/ethernet/sfc/ef100_ethtool.c b/drivers/net/ethernet/sfc/ef100_ethtool.c index 746b5314acb5..127b9d6ade6f 100644 --- a/drivers/net/ethernet/sfc/ef100_ethtool.c +++ b/drivers/net/ethernet/sfc/ef100_ethtool.c @@ -58,6 +58,7 @@ const struct ethtool_ops ef100_ethtool_ops = { .get_rxfh_indir_size = efx_ethtool_get_rxfh_indir_size, .get_rxfh_key_size = efx_ethtool_get_rxfh_key_size, + .rxfh_per_ctx_key = 1, .rxfh_priv_size = sizeof(struct efx_rss_context_priv), .get_rxfh = efx_ethtool_get_rxfh, .set_rxfh = efx_ethtool_set_rxfh, diff --git a/drivers/net/ethernet/sfc/ethtool.c b/drivers/net/ethernet/sfc/ethtool.c index 15245720c949..e4d86123b797 100644 --- a/drivers/net/ethernet/sfc/ethtool.c +++ b/drivers/net/ethernet/sfc/ethtool.c @@ -267,6 +267,7 @@ const struct ethtool_ops efx_ethtool_ops = { .set_rxnfc = efx_ethtool_set_rxnfc, .get_rxfh_indir_size = efx_ethtool_get_rxfh_indir_size, .get_rxfh_key_size = efx_ethtool_get_rxfh_key_size, + .rxfh_per_ctx_key = 1, .rxfh_priv_size = sizeof(struct efx_rss_context_priv), .get_rxfh = efx_ethtool_get_rxfh, .set_rxfh = efx_ethtool_set_rxfh, diff --git a/drivers/net/ethernet/sfc/siena/ethtool.c b/drivers/net/ethernet/sfc/siena/ethtool.c index 4c182d4edfc2..6d4e5101433a 100644 --- a/drivers/net/ethernet/sfc/siena/ethtool.c +++ b/drivers/net/ethernet/sfc/siena/ethtool.c @@ -241,6 +241,7 @@ static int efx_ethtool_get_ts_info(struct net_device *net_dev, const struct ethtool_ops efx_siena_ethtool_ops = { .cap_rss_ctx_supported = true, + .rxfh_per_ctx_key = true, .supported_coalesce_params = ETHTOOL_COALESCE_USECS | ETHTOOL_COALESCE_USECS_IRQ | ETHTOOL_COALESCE_USE_ADAPTIVE_RX, diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h index 55c9f613ab64..16f72a556fe9 100644 --- a/include/linux/ethtool.h +++ b/include/linux/ethtool.h @@ -731,6 +731,8 @@ struct kernel_ethtool_ts_info { * do not have to set this bit. * @cap_rss_sym_xor_supported: indicates if the driver supports symmetric-xor * RSS. + * @rxfh_per_ctx_key: device supports setting different RSS key for each + * additional context. * @rxfh_indir_space: max size of RSS indirection tables, if indirection table * size as returned by @get_rxfh_indir_size may change during lifetime * of the device. Leave as 0 if the table size is constant. @@ -952,6 +954,7 @@ struct ethtool_ops { u32 cap_link_lanes_supported:1; u32 cap_rss_ctx_supported:1; u32 cap_rss_sym_xor_supported:1; + u32 rxfh_per_ctx_key:1; u32 rxfh_indir_space; u16 rxfh_key_space; u16 rxfh_priv_size; diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c index 52dfb07393a6..e32b791f8d1c 100644 --- a/net/ethtool/ioctl.c +++ b/net/ethtool/ioctl.c @@ -1261,10 +1261,15 @@ static noinline_for_stack int ethtool_get_rxfh(struct net_device *dev, if (rxfh_dev.indir) memcpy(rxfh_dev.indir, ethtool_rxfh_context_indir(ctx), indir_bytes); - if (rxfh_dev.key) - memcpy(rxfh_dev.key, ethtool_rxfh_context_key(ctx), - user_key_size); - rxfh_dev.hfunc = ctx->hfunc; + if (!ops->rxfh_per_ctx_key) { + rxfh_dev.key_size = 0; + } else { + if (rxfh_dev.key) + memcpy(rxfh_dev.key, + ethtool_rxfh_context_key(ctx), + user_key_size); + rxfh_dev.hfunc = ctx->hfunc; + } rxfh_dev.input_xfrm = ctx->input_xfrm; ret = 0; } else { @@ -1281,6 +1286,11 @@ static noinline_for_stack int ethtool_get_rxfh(struct net_device *dev, &rxfh_dev.input_xfrm, sizeof(rxfh.input_xfrm))) { ret = -EFAULT; + } else if (copy_to_user(useraddr + + offsetof(struct ethtool_rxfh, key_size), + &rxfh_dev.key_size, + sizeof(rxfh.key_size))) { + ret = -EFAULT; } else if (copy_to_user(useraddr + offsetof(struct ethtool_rxfh, rss_config[0]), rss_config, total_size)) { @@ -1386,6 +1396,13 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev, indir_bytes = dev_indir_size * sizeof(rxfh_dev.indir[0]); + /* Check settings which may be global rather than per RSS-context */ + if (rxfh.rss_context && !ops->rxfh_per_ctx_key) + if (rxfh.key_size || + (rxfh.hfunc && rxfh.hfunc != ETH_RSS_HASH_NO_CHANGE) || + (rxfh.input_xfrm && rxfh.input_xfrm != RXH_XFRM_NO_CHANGE)) + return -EOPNOTSUPP; + rss_config = kzalloc(indir_bytes + dev_key_size, GFP_USER); if (!rss_config) return -ENOMEM; diff --git a/net/ethtool/rss.c b/net/ethtool/rss.c index a06bdac8b8a2..cd8100d81919 100644 --- a/net/ethtool/rss.c +++ b/net/ethtool/rss.c @@ -10,6 +10,7 @@ struct rss_req_info { struct rss_reply_data { struct ethnl_reply_data base; + bool no_key_fields; u32 indir_size; u32 hkey_size; u32 hfunc; @@ -60,9 +61,12 @@ rss_prepare_data(const struct ethnl_req_info *req_base, return -EOPNOTSUPP; /* Some drivers don't handle rss_context */ - if (request->rss_context && !(ops->cap_rss_ctx_supported || - ops->create_rxfh_context)) - return -EOPNOTSUPP; + if (request->rss_context) { + if (!ops->cap_rss_ctx_supported && !ops->create_rxfh_context) + return -EOPNOTSUPP; + + data->no_key_fields = !ops->rxfh_per_ctx_key; + } ret = ethnl_ops_begin(dev); if (ret < 0) @@ -132,13 +136,18 @@ rss_fill_reply(struct sk_buff *skb, const struct ethnl_req_info *req_base, nla_put_u32(skb, ETHTOOL_A_RSS_CONTEXT, request->rss_context)) return -EMSGSIZE; + if ((data->indir_size && + nla_put(skb, ETHTOOL_A_RSS_INDIR, + sizeof(u32) * data->indir_size, data->indir_table))) + return -EMSGSIZE; + + if (data->no_key_fields) + return 0; + if ((data->hfunc && nla_put_u32(skb, ETHTOOL_A_RSS_HFUNC, data->hfunc)) || (data->input_xfrm && nla_put_u32(skb, ETHTOOL_A_RSS_INPUT_XFRM, data->input_xfrm)) || - (data->indir_size && - nla_put(skb, ETHTOOL_A_RSS_INDIR, - sizeof(u32) * data->indir_size, data->indir_table)) || (data->hkey_size && nla_put(skb, ETHTOOL_A_RSS_HKEY, data->hkey_size, data->hkey))) return -EMSGSIZE;