Re: [PATCH 1/2] ethtool: add dynamic flag to ETHTOOL_{GS}RXFH commands
On Thu, 2016-02-04 at 19:30 -0500, David Miller wrote:
> From: "Keller, Jacob E"
> Date: Thu, 4 Feb 2016 23:09:56 +
>
> > So you're suggesting instead, to error when the second operation
> > (change number of queues) would fail the current settings?
>
> Yes.
>
> This is absolutely required.
I will investigate this route. I think there needs to some way for the
{GS}RXFH to pass enough information in a way the driver can clearly see
as "reset to default", but otherwise I think this is pretty straight
forward to implement.
Thanks,
Jake
Re: [PATCH 1/2] ethtool: add dynamic flag to ETHTOOL_{GS}RXFH commands
From: "Keller, Jacob E" Date: Thu, 4 Feb 2016 23:09:56 + > So you're suggesting instead, to error when the second operation > (change number of queues) would fail the current settings? Yes. This is absolutely required.
Re: [PATCH 1/2] ethtool: add dynamic flag to ETHTOOL_{GS}RXFH commands
On Thu, 2016-02-04 at 17:53 -0500, David Miller wrote: > From: Jacob Keller > Date: Tue, 2 Feb 2016 15:22:06 -0800 > > > Ethtool supports a few operations for modifying and controlling > > a device's RSS table. Sometimes, changes in other features of the > > device > > may require (or desire) changes to the RSS table. Currently there > > is no > > method to indicate to the driver whether the current RSS table > > settings > > should be maintained or overridden. > > Yes, there certainly is a way to indicate this. > > If the user asks for the change in the number of queues, and you > cannot retain the user's requested RSS settings, then you must fail > the queue setting change. > > And vice versa. > > You can't say to the user "I can adhere to your requested > configuration > change, but I might undo it for some unspecified reason" > The trouble here is the case where the indirection table configurations are valid now, but a change in the number of queues happening at a later time currently causes these settings to be lost. > That's unacceptable behavior, and that's exactly what this dynamic > flag means. > > If you cannot give the user what he asks for, precisely and reliably, > you fail the operation with an error. > > There is no way I am adding code which allows these "maybe" kind of > configuration operations. Either you can or you can't, and you tell > the user when you can't by erroring out on the operation that > invalidates the requirements. > > So you're suggesting instead, to error when the second operation (change number of queues) would fail the current settings? Current driver behaviors for all the drivers I checked work in one of two ways. 1) changing queues will destroy the RSS table as it will be reinitialized regardless of current settings 2) changing queues will maintain the RSS table if possible, unless the previous RSS table can't function. No driver currently fails this operation if the RSS table settings can't be preserved. In addition it results in weird behavior when a driver sets the RSS table at load, then increases the number of queues via an ethtool op, the result is that RSS does not use the new queues added by the ethtool operation. I can instead drop the ethtool changes and just have my driver record when the user has changed the tables, and attempt to error on queue setting operation, which may work. Essentially the idea was to have a flag indicating "use the driver defaults" which the driver can change as necessary when the number of queues changes, or other factors that may require RSS table changes. I can do this all hidden in the driver but then there is nothing exposing how the driver will behave under this circumstance. I'm all for a better suggestion, because I think what we're doing now is wrong, and the proposed solutions so far don't seem right either. If we preserve the RSS table when queues increase, then the user may be confused because RSS settings won't spread to the new queues. If we destroy the RSS settings the user will be possibly confused because their selected RSS settings do not work. If we fail the setting of queues when RSS table is not the default value, then a user might be confused as to why they can't change the number of queues. Also, I am unsure whether or not we can tell from the ethtool op function that we actually are being "reset" to the default, vs just being set. This is because the netlink message of "0 length" which indicates default simply has the ethtool core fill in a standard equal weight default, which I am not sure our driver can tell that it should now be ok to enable queue changes. So, something is missing in the current flow to allow this. I think the best solution is simply prevent changing number of queues while we have a non-default RSS setting, and require RSS to be reset before queues can be changed. Thoughts? Regards, Jake
Re: [PATCH 1/2] ethtool: add dynamic flag to ETHTOOL_{GS}RXFH commands
From: Jacob Keller Date: Tue, 2 Feb 2016 15:22:06 -0800 > Ethtool supports a few operations for modifying and controlling > a device's RSS table. Sometimes, changes in other features of the device > may require (or desire) changes to the RSS table. Currently there is no > method to indicate to the driver whether the current RSS table settings > should be maintained or overridden. Yes, there certainly is a way to indicate this. If the user asks for the change in the number of queues, and you cannot retain the user's requested RSS settings, then you must fail the queue setting change. And vice versa. You can't say to the user "I can adhere to your requested configuration change, but I might undo it for some unspecified reason" That's unacceptable behavior, and that's exactly what this dynamic flag means. If you cannot give the user what he asks for, precisely and reliably, you fail the operation with an error. There is no way I am adding code which allows these "maybe" kind of configuration operations. Either you can or you can't, and you tell the user when you can't by erroring out on the operation that invalidates the requirements.
[PATCH 1/2] ethtool: add dynamic flag to ETHTOOL_{GS}RXFH commands
Ethtool supports a few operations for modifying and controlling
a device's RSS table. Sometimes, changes in other features of the device
may require (or desire) changes to the RSS table. Currently there is no
method to indicate to the driver whether the current RSS table settings
should be maintained or overridden.
A simple example of this is for when the number of receive queues is
changed, there are two possibilities. First, the number of queues is
decreased. This must result in a reprogramming of the RSS table since it
will no longer match correctly and may attempt to assign traffic to
a queue which is now disabled. In this case drivers have a clear
indication of what to do.
The second case, is when the number of queues has increased. In this
case, the current RSS table may be preserved. However, doing so would
result in the new queues being unused for RSS. But if the driver chooses
to destroy the RSS configuration it may result in unwanted behavior as
now the user's configured changes are lost.
This patch attempts to resolve this (and other similar) issues by
indicating a new flag "dynamic" which can be set by the user when
calling the ethtool interface.
This flag indicates to the driver that it may overwrite settings in the
RSS table. If false, it indicates the driver should do what it can to
preserve the RSS table changes requested by the user. That is, for cases
where it can preserve the table it must. If the value is set true, it
means the driver may or may not apply the current settings and is free
to change the values as necessary. The current default is set to false,
as this is how most drivers appear to behave today.
Signed-off-by: Jacob Keller
Cc: Lendacky, Thomas
Cc: Yuval Mintz
Cc: Michael Chan
Cc: Matt Carlson
Cc: Sunil Goutham
Cc: Hariprasad Shenai
Cc: Govindarajulu Varadarajan <_gov...@gmx.com>
Cc: Kalesh AP
Cc: Andrew Lunn
Cc: Shannon Nelson
Cc: Mitch Williams
Cc: Carolyn Wyborny
Cc: Emil Tantilov
Cc: Thomas Petazzoni
Cc: Amir Vadai
Cc: Achiad Shochat
Cc: Ben Hutchings
Cc: Michał Mirosław
Cc: Alexander Duyck
---
drivers/net/ethernet/amd/xgbe/xgbe-ethtool.c | 7 --
.../net/ethernet/broadcom/bnx2x/bnx2x_ethtool.c| 7 --
drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 5 +++-
drivers/net/ethernet/broadcom/tg3.c| 8 +--
.../net/ethernet/cavium/thunder/nicvf_ethtool.c| 7 --
drivers/net/ethernet/chelsio/cxgb4/cxgb4_ethtool.c | 7 --
drivers/net/ethernet/cisco/enic/enic_ethtool.c | 7 --
drivers/net/ethernet/emulex/benet/be_ethtool.c | 7 --
drivers/net/ethernet/hisilicon/hns/hns_ethtool.c | 8 +--
drivers/net/ethernet/intel/fm10k/fm10k_ethtool.c | 7 --
drivers/net/ethernet/intel/i40e/i40e_ethtool.c | 7 --
drivers/net/ethernet/intel/i40evf/i40evf_ethtool.c | 6 +++--
drivers/net/ethernet/intel/igb/igb_ethtool.c | 6 +++--
drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c | 7 --
drivers/net/ethernet/intel/ixgbevf/ethtool.c | 5 +++-
drivers/net/ethernet/marvell/mvneta.c | 8 +--
drivers/net/ethernet/mellanox/mlx4/en_ethtool.c| 7 --
.../net/ethernet/mellanox/mlx5/core/en_ethtool.c | 7 --
.../net/ethernet/netronome/nfp/nfp_net_ethtool.c | 7 --
drivers/net/ethernet/sfc/ethtool.c | 7 --
include/linux/ethtool.h| 4 ++--
include/uapi/linux/ethtool.h | 8 ++-
net/core/ethtool.c | 27 +-
23 files changed, 124 insertions(+), 52 deletions(-)
diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-ethtool.c
b/drivers/net/ethernet/amd/xgbe/xgbe-ethtool.c
index 6040293db9c1..4eecd225db7c 100644
--- a/drivers/net/ethernet/amd/xgbe/xgbe-ethtool.c
+++ b/drivers/net/ethernet/amd/xgbe/xgbe-ethtool.c
@@ -509,11 +509,14 @@ static u32 xgbe_get_rxfh_indir_size(struct net_device
*netdev)
}
static int xgbe_get_rxfh(struct net_device *netdev, u32 *indir, u8 *key,
-u8 *hfunc)
+u8 *hfunc, u8 *dynamic)
{
struct xgbe_prv_data *pdata = netdev_priv(netdev);
unsigned int i;
+ if (dynamic)
+ *dynamic = false;
+
if (indir) {
for (i = 0; i < ARRAY_SIZE(pdata->rss_table); i++)
indir[i] = XGMAC_GET_BITS(pdata->rss_table[i],
@@ -530,7 +533,7 @@ static int xgbe_get_rxfh(struct net_device *netdev, u32
*indir, u8 *key,
}
static int xgbe_set_rxfh(struct net_device *netdev, const u32 *indir,
-const u8 *key, const u8 hfunc)
+const u8 *key, const u8 hfunc, const u8 dynamic)
{
struct xgbe_prv_data *pdata = netdev_priv(netdev);
struct xgbe_hw_if *hw_if = &pdata->hw_if;
diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_ethtool.c
b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_ethtool.c
index 820b7e04bb5f..b346ac8d2b49 1
