| Message ID | 20240803042624.970352-3-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: > Implement the separate create/modify/delete ops for RSS. > > No problems with IDs - even tho RSS tables are per device > the driver already seems to allocate IDs linearly per port. > There's a translation table from per-port context ID > to device context ID. > > mvpp2 doesn't have a key for the hash, it defaults to > an empty/previous indir table. Given that, should this be after patch #6? So as to make it obviously correct not to populate ethtool_rxfh_context_key(ctx) with the default context's key. > @@ -5750,6 +5792,7 @@ static const struct net_device_ops mvpp2_netdev_ops = { > > static const struct ethtool_ops mvpp2_eth_tool_ops = { > .cap_rss_ctx_supported = true, > + .rxfh_max_context_id = MVPP22_N_RSS_TABLES, Max ID is inclusive, not exclusive, so I think this should be MVPP22_N_RSS_TABLES - 1?
On Mon, 5 Aug 2024 12:25:28 +0100 Edward Cree wrote: > > mvpp2 doesn't have a key for the hash, it defaults to > > an empty/previous indir table. > > Given that, should this be after patch #6? So as to make it > obviously correct not to populate ethtool_rxfh_context_key(ctx) > with the default context's key. It's a bit different. Patch 6 is about devices which have a key but the same key is used for all contexts. mvpp2 has no key at all even for context 0 (get_rxfh_key_size is not defined). > > @@ -5750,6 +5792,7 @@ static const struct net_device_ops mvpp2_netdev_ops = { > > > > static const struct ethtool_ops mvpp2_eth_tool_ops = { > > .cap_rss_ctx_supported = true, > > + .rxfh_max_context_id = MVPP22_N_RSS_TABLES, > > Max ID is inclusive, not exclusive, so I think this should be > MVPP22_N_RSS_TABLES - 1? I totally did check this before sending: * @rxfh_max_context_id: maximum (exclusive) supported RSS context ID. If this * is zero then the core may choose any (nonzero) ID, otherwise the core * will only use IDs strictly less than this value, as the @rss_context * argument to @create_rxfh_context and friends. But you're right, the code acts as if it was inclusive :S Coincidentally, the default also appears exclusive: u32 limit = ops->rxfh_max_context_id ?: U32_MAX; U32_MAX can't be used, it has special meaning: #define ETH_RXFH_CONTEXT_ALLOC 0xffffffff These seem like net-worthy fixes, no?
On 05/08/2024 22:29, Jakub Kicinski wrote: > On Mon, 5 Aug 2024 12:25:28 +0100 Edward Cree wrote: >>> mvpp2 doesn't have a key for the hash, it defaults to >>> an empty/previous indir table. >> >> Given that, should this be after patch #6? So as to make it >> obviously correct not to populate ethtool_rxfh_context_key(ctx) >> with the default context's key. > > It's a bit different. Patch 6 is about devices which have a key but > the same key is used for all contexts. mvpp2 has no key at all > even for context 0 (get_rxfh_key_size is not defined). Oh, I see. Clarify that in the commit message, perhaps? >>> @@ -5750,6 +5792,7 @@ static const struct net_device_ops mvpp2_netdev_ops = { >>> >>> static const struct ethtool_ops mvpp2_eth_tool_ops = { >>> .cap_rss_ctx_supported = true, >>> + .rxfh_max_context_id = MVPP22_N_RSS_TABLES, >> >> Max ID is inclusive, not exclusive, so I think this should be >> MVPP22_N_RSS_TABLES - 1? > > I totally did check this before sending: > > * @rxfh_max_context_id: maximum (exclusive) supported RSS context ID. If this > * is zero then the core may choose any (nonzero) ID, otherwise the core > * will only use IDs strictly less than this value, as the @rss_context > * argument to @create_rxfh_context and friends. > > But you're right, the code acts as if it was inclusive :S Mea culpa, clearly when I was porting to XArray I must have confused myself over this. > Coincidentally, the default also appears exclusive: > > u32 limit = ops->rxfh_max_context_id ?: U32_MAX; > > U32_MAX can't be used, it has special meaning: > > #define ETH_RXFH_CONTEXT_ALLOC 0xffffffff Given that both the default and drivers look more reasonable with an exclusive than an inclusive limit (I assume most drivers with a limit will have an N, like mvpp2 does, rather than a MAX), I guess we should change the code to match the doc rather than the other way around. > These seem like net-worthy fixes, no? Yep, agreed. I'll send a patch.
On Tue, 6 Aug 2024 14:28:16 +0100 Edward Cree wrote: > > Coincidentally, the default also appears exclusive: > > > > u32 limit = ops->rxfh_max_context_id ?: U32_MAX; > > > > U32_MAX can't be used, it has special meaning: > > > > #define ETH_RXFH_CONTEXT_ALLOC 0xffffffff > > Given that both the default and drivers look more reasonable > with an exclusive than an inclusive limit (I assume most > drivers with a limit will have an N, like mvpp2 does, rather > than a MAX), I guess we should change the code to match the > doc rather than the other way around. Somewhat unclear, because context 0 may not count, so to speak. At least for bnxt using inclusive max context worked well. But no preference, with the (obvious?) caveat that if we change the definition of the field to be exclusive we should rename it.
diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_cls.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_cls.c index 40aeaa7bd739..1641791a2d5b 100644 --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_cls.c +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_cls.c @@ -1522,29 +1522,19 @@ static int mvpp22_rss_context_create(struct mvpp2_port *port, u32 *rss_ctx) return 0; } -int mvpp22_port_rss_ctx_create(struct mvpp2_port *port, u32 *port_ctx) +int mvpp22_port_rss_ctx_create(struct mvpp2_port *port, u32 port_ctx) { u32 rss_ctx; - int ret, i; + int ret; ret = mvpp22_rss_context_create(port, &rss_ctx); if (ret) return ret; - /* Find the first available context number in the port, starting from 1. - * Context 0 on each port is reserved for the default context. - */ - for (i = 1; i < MVPP22_N_RSS_TABLES; i++) { - if (port->rss_ctx[i] < 0) - break; - } - - if (i == MVPP22_N_RSS_TABLES) + if (WARN_ON_ONCE(port->rss_ctx[port_ctx] >= 0)) return -EINVAL; - port->rss_ctx[i] = rss_ctx; - *port_ctx = i; - + port->rss_ctx[port_ctx] = rss_ctx; return 0; } diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_cls.h b/drivers/net/ethernet/marvell/mvpp2/mvpp2_cls.h index 663157dc8062..85c9c6e80678 100644 --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_cls.h +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_cls.h @@ -264,7 +264,7 @@ int mvpp22_port_rss_init(struct mvpp2_port *port); int mvpp22_port_rss_enable(struct mvpp2_port *port); int mvpp22_port_rss_disable(struct mvpp2_port *port); -int mvpp22_port_rss_ctx_create(struct mvpp2_port *port, u32 *rss_ctx); +int mvpp22_port_rss_ctx_create(struct mvpp2_port *port, u32 rss_ctx); int mvpp22_port_rss_ctx_delete(struct mvpp2_port *port, u32 rss_ctx); int mvpp22_port_rss_ctx_indir_set(struct mvpp2_port *port, u32 rss_ctx, diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c index 0d62a33afa80..90182f6fd9a1 100644 --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c @@ -5696,38 +5696,80 @@ static int mvpp2_ethtool_get_rxfh(struct net_device *dev, return ret; } +static bool mvpp2_ethtool_rxfh_okay(struct mvpp2_port *port, + const struct ethtool_rxfh_param *rxfh) +{ + if (!mvpp22_rss_is_supported(port)) + return false; + + if (rxfh->hfunc != ETH_RSS_HASH_NO_CHANGE && + rxfh->hfunc != ETH_RSS_HASH_CRC32) + return false; + + if (rxfh->key) + return false; + + return true; +} + +static int mvpp2_create_rxfh_context(struct net_device *dev, + struct ethtool_rxfh_context *ctx, + const struct ethtool_rxfh_param *rxfh, + struct netlink_ext_ack *extack) +{ + struct mvpp2_port *port = netdev_priv(dev); + int ret = 0; + + if (!mvpp2_ethtool_rxfh_okay(port, rxfh)) + return -EOPNOTSUPP; + + ctx->hfunc = ETH_RSS_HASH_CRC32; + + ret = mvpp22_port_rss_ctx_create(port, rxfh->rss_context); + if (ret) + return ret; + + if (!rxfh->indir) + ret = mvpp22_port_rss_ctx_indir_get(port, rxfh->rss_context, + ethtool_rxfh_context_indir(ctx)); + else + ret = mvpp22_port_rss_ctx_indir_set(port, rxfh->rss_context, + rxfh->indir); + return ret; +} + +static int mvpp2_modify_rxfh_context(struct net_device *dev, + struct ethtool_rxfh_context *ctx, + const struct ethtool_rxfh_param *rxfh, + struct netlink_ext_ack *extack) +{ + struct mvpp2_port *port = netdev_priv(dev); + int ret = 0; + + if (!mvpp2_ethtool_rxfh_okay(port, rxfh)) + return -EOPNOTSUPP; + + if (rxfh->indir) + ret = mvpp22_port_rss_ctx_indir_set(port, rxfh->rss_context, + rxfh->indir); + return ret; +} + +static int mvpp2_remove_rxfh_context(struct net_device *dev, + struct ethtool_rxfh_context *ctx, + u32 rss_context, + struct netlink_ext_ack *extack) +{ + struct mvpp2_port *port = netdev_priv(dev); + + return mvpp22_port_rss_ctx_delete(port, rss_context); +} + static int mvpp2_ethtool_set_rxfh(struct net_device *dev, struct ethtool_rxfh_param *rxfh, struct netlink_ext_ack *extack) { - struct mvpp2_port *port = netdev_priv(dev); - u32 *rss_context = &rxfh->rss_context; - int ret = 0; - - if (!mvpp22_rss_is_supported(port)) - return -EOPNOTSUPP; - - if (rxfh->hfunc != ETH_RSS_HASH_NO_CHANGE && - rxfh->hfunc != ETH_RSS_HASH_CRC32) - return -EOPNOTSUPP; - - if (rxfh->key) - return -EOPNOTSUPP; - - if (*rss_context && rxfh->rss_delete) - return mvpp22_port_rss_ctx_delete(port, *rss_context); - - if (*rss_context == ETH_RXFH_CONTEXT_ALLOC) { - ret = mvpp22_port_rss_ctx_create(port, rss_context); - if (ret) - return ret; - } - - if (rxfh->indir) - ret = mvpp22_port_rss_ctx_indir_set(port, *rss_context, - rxfh->indir); - - return ret; + return mvpp2_modify_rxfh_context(dev, NULL, rxfh, extack); } /* Device ops */ @@ -5750,6 +5792,7 @@ static const struct net_device_ops mvpp2_netdev_ops = { static const struct ethtool_ops mvpp2_eth_tool_ops = { .cap_rss_ctx_supported = true, + .rxfh_max_context_id = MVPP22_N_RSS_TABLES, .supported_coalesce_params = ETHTOOL_COALESCE_USECS | ETHTOOL_COALESCE_MAX_FRAMES, .nway_reset = mvpp2_ethtool_nway_reset, @@ -5772,6 +5815,9 @@ static const struct ethtool_ops mvpp2_eth_tool_ops = { .get_rxfh_indir_size = mvpp2_ethtool_get_rxfh_indir_size, .get_rxfh = mvpp2_ethtool_get_rxfh, .set_rxfh = mvpp2_ethtool_set_rxfh, + .create_rxfh_context = mvpp2_create_rxfh_context, + .modify_rxfh_context = mvpp2_modify_rxfh_context, + .remove_rxfh_context = mvpp2_remove_rxfh_context, }; /* Used for PPv2.1, or PPv2.2 with the old Device Tree binding that
Implement the separate create/modify/delete ops for RSS. No problems with IDs - even tho RSS tables are per device the driver already seems to allocate IDs linearly per port. There's a translation table from per-port context ID to device context ID. mvpp2 doesn't have a key for the hash, it defaults to an empty/previous indir table. Compile-tested only. Signed-off-by: Jakub Kicinski <kuba@kernel.org> --- v2: - copy the default indir table - use ID from the core - fix build issues CC: marcin.s.wojtas@gmail.com CC: linux@armlinux.org.uk --- .../net/ethernet/marvell/mvpp2/mvpp2_cls.c | 18 +--- .../net/ethernet/marvell/mvpp2/mvpp2_cls.h | 2 +- .../net/ethernet/marvell/mvpp2/mvpp2_main.c | 102 +++++++++++++----- 3 files changed, 79 insertions(+), 43 deletions(-)