Skip to content

Commit 9a14b1c

Browse files
davem330davem330
committed
Merge branch 'ethtool-channels-rxfh-conflict'
Jacob Keller says: ==================== ethtool: correct {GS}CHANNELS and {GS}RXFH conflict This patch series fixes up ethtool_set_channels operation which allowed modifying the RXFH table indirectly by reducing the number of queues below the current max queue used by the Rx flow table. Most drivers incorrectly allowed this to destroy the Rx flow table and would then start by reinitializing it to default settings. However, drivers are not able to correctly handle the conflict since there was no way to differentiate between the default settings and the user requested explicit settings. To fix this, implement a new netdev private flag which we use to indicate whether the RXFH has been user configured. If someone has a better alternative of how to store this information, let me know. I am not sure that priv_flags is the best solution but I have not had any better idea. Secondly, we add a function which just calls the driver's get_rxfh callback to determine the current indirection table. Loop through this and we can determine the current highest queue that will be used by RSS. Now, modify ethtool_set_channels to add a check ensuring that if (a) we have had rxfh configured by user, (b) we can get the maximum RSS queue currently used, then we ensure that the newly requested Rx count (or combined count) is at least as high as this maximum RSS queue. The reasoning here is that we can always safely increase the number of queues. If we decrease the queues we must ensure that the decrease does not go lower than the highest in-use queue for the Rx flow table. Drivers may still need to be patched if they currently overwrite the Rx flow table during channel configuration. If the driver currently always resets Rx flow table when increasing number of queues it must be patched to only do this when netif_is_rxfh_configured returns false. The second patch simply adds a check to ensure that all provided channel counts fit within driver defined maximums. The third patch fixes fm10k to correctly reconfigure the RSS reta table whenever it is still unconfigured. This means that the default state will provide RSS to every queue. Once the user has configured RXFH, then we should maintain it. In addition, since the case where we must reconfigure the RSS table in this case should now no longer occur, add a dev_err message to indicate the user that we did so. I have also supplied an ethtool patch to enable setting the default Rx flow indirection table. Without this, current ethtool does not support sending an indir_size of 0, and thus does not correctly support configuring back to the default. Changes in v2: * fixed compile error * fixed incorrect comparison with max_rx_in_use * adjusted looping over dev_size * removed inline on function * dropped patch about separating combined vs asymmetric channels * verified behavior using fm10k driver ==================== Signed-off-by: David S. Miller <davem@davemloft.net>
2 parents 64f10f6 + 1012014 commit 9a14b1c

File tree

3 files changed

+82
-4
lines changed

3 files changed

+82
-4
lines changed

‎drivers/net/ethernet/intel/fm10k/fm10k_main.c

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1937,15 +1937,21 @@ static void fm10k_init_reta(struct fm10k_intfc *interface)
19371937
u16 i, rss_i = interface->ring_feature[RING_F_RSS].indices;
19381938
u32 reta, base;
19391939

1940-
/* If the netdev is initialized we have to maintain table if possible */
1941-
if (interface->netdev->reg_state != NETREG_UNINITIALIZED) {
1940+
/* If the Rx flow indirection table has been configured manually, we
1941+
* need to maintain it when possible.
1942+
*/
1943+
if (netif_is_rxfh_configured(interface->netdev)) {
19421944
for (i = FM10K_RETA_SIZE; i--;) {
19431945
reta = interface->reta[i];
19441946
if ((((reta << 24) >> 24) < rss_i) &&
19451947
(((reta << 16) >> 24) < rss_i) &&
19461948
(((reta << 8) >> 24) < rss_i) &&
19471949
(((reta) >> 24) < rss_i))
19481950
continue;
1951+
1952+
/* this should never happen */
1953+
dev_err(&interface->pdev->dev,
1954+
"RSS indirection table assigned flows out of queue bounds. Reconfiguring.\n");
19491955
goto repopulate_reta;
19501956
}
19511957

‎include/linux/netdevice.h

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1291,6 +1291,7 @@ struct net_device_ops {
12911291
* @IFF_OPENVSWITCH: device is a Open vSwitch master
12921292
* @IFF_L3MDEV_SLAVE: device is enslaved to an L3 master device
12931293
* @IFF_TEAM: device is a team device
1294+
* @IFF_RXFH_CONFIGURED: device has had Rx Flow indirection table configured
12941295
*/
12951296
enum netdev_priv_flags {
12961297
IFF_802_1Q_VLAN = 1<<0,
@@ -1318,6 +1319,7 @@ enum netdev_priv_flags {
13181319
IFF_OPENVSWITCH = 1<<22,
13191320
IFF_L3MDEV_SLAVE = 1<<23,
13201321
IFF_TEAM = 1<<24,
1322+
IFF_RXFH_CONFIGURED = 1<<25,
13211323
};
13221324

13231325
#define IFF_802_1Q_VLAN IFF_802_1Q_VLAN
@@ -1345,6 +1347,7 @@ enum netdev_priv_flags {
13451347
#define IFF_OPENVSWITCH IFF_OPENVSWITCH
13461348
#define IFF_L3MDEV_SLAVE IFF_L3MDEV_SLAVE
13471349
#define IFF_TEAM IFF_TEAM
1350+
#define IFF_RXFH_CONFIGURED IFF_RXFH_CONFIGURED
13481351

13491352
/**
13501353
* struct net_device - The DEVICE structure.
@@ -4048,6 +4051,11 @@ static inline bool netif_is_lag_port(const struct net_device *dev)
40484051
return netif_is_bond_slave(dev) || netif_is_team_port(dev);
40494052
}
40504053

4054+
static inline bool netif_is_rxfh_configured(const struct net_device *dev)
4055+
{
4056+
return dev->priv_flags & IFF_RXFH_CONFIGURED;
4057+
}
4058+
40514059
/* This device needs to keep skb dst for qdisc enqueue or ndo_start_xmit() */
40524060
static inline void netif_keep_dst(struct net_device *dev)
40534061
{

‎net/core/ethtool.c

Lines changed: 66 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -642,6 +642,37 @@ void netdev_rss_key_fill(void *buffer, size_t len)
642642
}
643643
EXPORT_SYMBOL(netdev_rss_key_fill);
644644

645+
static int ethtool_get_max_rxfh_channel(struct net_device *dev, u32 *max)
646+
{
647+
u32 dev_size, current_max = 0;
648+
u32 *indir;
649+
int ret;
650+
651+
if (!dev->ethtool_ops->get_rxfh_indir_size ||
652+
!dev->ethtool_ops->get_rxfh)
653+
return -EOPNOTSUPP;
654+
dev_size = dev->ethtool_ops->get_rxfh_indir_size(dev);
655+
if (dev_size == 0)
656+
return -EOPNOTSUPP;
657+
658+
indir = kcalloc(dev_size, sizeof(indir[0]), GFP_USER);
659+
if (!indir)
660+
return -ENOMEM;
661+
662+
ret = dev->ethtool_ops->get_rxfh(dev, indir, NULL, NULL);
663+
if (ret)
664+
goto out;
665+
666+
while (dev_size--)
667+
current_max = max(current_max, indir[dev_size]);
668+
669+
*max = current_max;
670+
671+
out:
672+
kfree(indir);
673+
return ret;
674+
}
675+
645676
static noinline_for_stack int ethtool_get_rxfh_indir(struct net_device *dev,
646677
void __user *useraddr)
647678
{
@@ -738,6 +769,14 @@ static noinline_for_stack int ethtool_set_rxfh_indir(struct net_device *dev,
738769
}
739770

740771
ret = ops->set_rxfh(dev, indir, NULL, ETH_RSS_HASH_NO_CHANGE);
772+
if (ret)
773+
goto out;
774+
775+
/* indicate whether rxfh was set to default */
776+
if (user_size == 0)
777+
dev->priv_flags &= ~IFF_RXFH_CONFIGURED;
778+
else
779+
dev->priv_flags |= IFF_RXFH_CONFIGURED;
741780

742781
out:
743782
kfree(indir);
@@ -897,6 +936,14 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,
897936
}
898937

899938
ret = ops->set_rxfh(dev, indir, hkey, rxfh.hfunc);
939+
if (ret)
940+
goto out;
941+
942+
/* indicate whether rxfh was set to default */
943+
if (rxfh.indir_size == 0)
944+
dev->priv_flags &= ~IFF_RXFH_CONFIGURED;
945+
else if (rxfh.indir_size != ETH_RXFH_INDIR_NO_CHANGE)
946+
dev->priv_flags |= IFF_RXFH_CONFIGURED;
900947

901948
out:
902949
kfree(rss_config);
@@ -1227,14 +1274,31 @@ static noinline_for_stack int ethtool_get_channels(struct net_device *dev,
12271274
static noinline_for_stack int ethtool_set_channels(struct net_device *dev,
12281275
void __user *useraddr)
12291276
{
1230-
struct ethtool_channels channels;
1277+
struct ethtool_channels channels, max;
1278+
u32 max_rx_in_use = 0;
12311279

1232-
if (!dev->ethtool_ops->set_channels)
1280+
if (!dev->ethtool_ops->set_channels || !dev->ethtool_ops->get_channels)
12331281
return -EOPNOTSUPP;
12341282

12351283
if (copy_from_user(&channels, useraddr, sizeof(channels)))
12361284
return -EFAULT;
12371285

1286+
dev->ethtool_ops->get_channels(dev, &max);
1287+
1288+
/* ensure new counts are within the maximums */
1289+
if ((channels.rx_count > max.max_rx) ||
1290+
(channels.tx_count > max.max_tx) ||
1291+
(channels.combined_count > max.max_combined) ||
1292+
(channels.other_count > max.max_other))
1293+
return -EINVAL;
1294+
1295+
/* ensure the new Rx count fits within the configured Rx flow
1296+
* indirection table settings */
1297+
if (netif_is_rxfh_configured(dev) &&
1298+
!ethtool_get_max_rxfh_channel(dev, &max_rx_in_use) &&
1299+
(channels.combined_count + channels.rx_count) <= max_rx_in_use)
1300+
return -EINVAL;
1301+
12381302
return dev->ethtool_ops->set_channels(dev, &channels);
12391303
}
12401304

0 commit comments

Comments
 (0)