[PATCH 0/2] ethtool: {SG}RXFH indirection deficiency

This patch set adds a new ethtool operation .reset_rxfh_indir which is
used by the core ethtool stack to properly indicate to a driver that
the default RSS indirection table has been requested. Current behavior
for notifying the default settings is indistinguishable from an
explicit request. There is no easy way to look at the indirection
table and tell if it matches the default, either. To allow drivers the
ability to correctly report -EINVAL when changing the number of
channels, add the new operation suggested. I chose to use a new
ethtool op instead of an additional flag since this has a lower impact
and we already use NULL on the *indir variable in set_rxfh to indicate
no change was requested.

The second patch in the series is an example implementation of the
.reset_rxfh_indir operation along with fixes to the fm10k_set_channels
to prevent changing the number of channels if it would interfere
with the current redirection table.

Jacob Keller (2):
  ethtool: support notifying drivers when user requests default rxfh
    table
  fm10k: correctly report error when changing number of channels

 drivers/net/ethernet/intel/fm10k/fm10k.h         |  2 ++
 drivers/net/ethernet/intel/fm10k/fm10k_ethtool.c | 45 ++++++++++++++++++++++++
 drivers/net/ethernet/intel/fm10k/fm10k_main.c    | 11 ++++--
 include/linux/ethtool.h                          |  3 ++
 net/core/ethtool.c                               |  8 +++++
 5 files changed, 66 insertions(+), 3 deletions(-)

-- 
2.7.0.236.gda096a0.dirty

Currently, userspace ethtool supports requesting the default indirection
table by passing 0 as the indir_size. However, ops->set_rxfh does not
distinguish between user requesting default via indir_size=0 and user
requesting explicitly settings which are equivalent of the default.

This causes problems because other driver changes such as number of
channels (queues) should fail when they will not work with other current
settings such as the indirection table.

If there is no way to indicate when the driver is in default RSS table
state compared to user set states, then we wouldn't ever allow changing
the number of queues at all. To fix this, drivers must be able to
distinguish between requested settings and the configured defaults.

We can't use a value of NULL to the *indir pointer as this is already
used to indicate no change to the indirection table. Instead, implement
a new callback ops->reset_rxfh_indir which is used whenever the user
requests size of zero. This has lower impact than adding a new flag and
can be implemented by drivers as necessary.

In this way we have the following scenarios now:

(a) driver is in default configuration, and is free to change RSS
settings as necessary due to other changes such as number of queues.
This makes sense since we can essentially consider this as "RSS
indirection has not been configured". This is the default state of a new
device. In this state, I think the reasonable default is "Always RSS to
all enabled queues".

(b) user has requested RSS indirection settings, should change the
driver state to "RSS indirection table has been configured". Now if the
user requests a change in the number of queues, we can properly indicate
an error when these settings would conflict with the requested RSS
indirection table.

(c) The user can request default settings via the {GS}RXFH operations
and the driver will get a clear indication that it should reset to the
default "RSS indirection table has not been configured" mode. In this
way it will be able to then change the number of queues and go about
business.

If we don't have a way to properly indicate that we've reset to default
then we are not able to implement the proposed behavior, so this patch
adds a new method to properly indicate that we have reset to the default
indirection table.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
Cc: Dave Miller <davem@davemloft.net>
---
This is an alternative proposal to my previous patch. I do not believe
it is possible to obtain desired behavior without this patch, as it is
not possible for the driver to distinguish default settings from user
configured RSS table. If we don't do that, then the user will never be
able to reduce the number of queues without first modifying the RSS
redirection table, which seems wrong to me.

 include/linux/ethtool.h | 3 +++
 net/core/ethtool.c      | 8 ++++++++
 2 files changed, 11 insertions(+)

diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 653dc9c4ebac..700ac5658d34 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h

@@ -186,6 +186,8 @@ static inline u32 ethtool_rxfh_indir_default(u32 index, u32 n_rx_rings)
  *	will remain unchanged.
  *	Returns a negative error code or zero. An error code must be returned
  *	if at least one unsupported change was requested.
+ * @reset_rxfh_indir: Reset the contents of the RX flow hash indirection table
+ * 	to driver defaults. Returns a negative error code or zero.
  * @get_channels: Get number of channels.
  * @set_channels: Set number of channels.  Returns a negative error code or
  *	zero.

@@ -262,6 +264,7 @@ struct ethtool_ops {
 			    u8 *hfunc);
 	int	(*set_rxfh)(struct net_device *, const u32 *indir,
 			    const u8 *key, const u8 hfunc);
+	int	(*reset_rxfh_indir)(struct net_device *);
 	void	(*get_channels)(struct net_device *, struct ethtool_channels *);
 	int	(*set_channels)(struct net_device *, struct ethtool_channels *);
 	int	(*get_dump_flag)(struct net_device *, struct ethtool_dump *);

diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index daf04709dd3c..4c6a1c2b8b61 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c

@@ -725,7 +725,13 @@ static noinline_for_stack int ethtool_set_rxfh_indir(struct net_device *dev,
 	if (ret)
 		goto out;
 
+	/* user_size == 0 means reset the indir table to default. */
 	if (user_size == 0) {
+		if (ops->reset_rxfh_indir) {
+			err = ops->reset_rxfh_indir(dev);
+			goto out;
+		}
+
 		for (i = 0; i < dev_size; i++)
 			indir[i] = ethtool_rxfh_indir_default(i, rx_rings.data);
 	} else {

@@ -880,6 +886,8 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,
 						  rxfh.indir_size);
 		if (ret)
 			goto out;
+	} else if (rxfh.indir_size == 0 && ops->reset_rxfh_indir) {
+		ret = ops->reset_rxfh_indir(dev);
 	} else if (rxfh.indir_size == 0) {
 		indir = (u32 *)rss_config;
 		for (i = 0; i < dev_indir_size; i++)

-- 
2.7.0.236.gda096a0.dirty

Previously, the fm10k driver would incorrectly allow changing the number
of combined channels when this would have altered user configured RSS
indirection table. With the new ethtool operation .reset_rxfh_indir, we
are now able to correctly handle the changes to number of channels. This
requires several changes:

(a) we must first store whether or not the RSS redirection table has
    been manually configured, as there is no way to tell the default
    table from an explicit request. Do this by implementing
    reset_rxfh_indir ethtool op, and storing a flag to indicate how the
    redirection table is set.

(b) replace the fm10k_init_reta code with a check of intialized
    netdevice to instead check the new FM10K_FLAG_RETA_TABLE_CONFIGURED.
    This will ensure that the table is always repopulated if we're in
    the default (unconfigured) state. We still must repopulate if
    somehow the reta table is changed to what will become an invalid
    state due to new RSS limits. However, since this should no longer
    happen, add a dev_err() call to indicate clearly to user what
    happened.

(c) modify the fm10k_set_channels call to check that th new count is
    within bounds on the reta table. This check is only enforced if the
    user has manually configured the RSS indirection table, since the
    driver should be free to repopulate its own default configuration.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
 drivers/net/ethernet/intel/fm10k/fm10k.h         |  2 ++
 drivers/net/ethernet/intel/fm10k/fm10k_ethtool.c | 45 ++++++++++++++++++++++++
 drivers/net/ethernet/intel/fm10k/fm10k_main.c    | 11 ++++--
 3 files changed, 55 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/intel/fm10k/fm10k.h b/drivers/net/ethernet/intel/fm10k/fm10k.h
index 83f386714e87..983bdda9509b 100644
--- a/drivers/net/ethernet/intel/fm10k/fm10k.h
+++ b/drivers/net/ethernet/intel/fm10k/fm10k.h

@@ -268,6 +268,7 @@ struct fm10k_intfc {
 #define FM10K_FLAG_RX_TS_ENABLED		(u32)(BIT(3))
 #define FM10K_FLAG_SWPRI_CONFIG			(u32)(BIT(4))
 #define FM10K_FLAG_DEBUG_STATS			(u32)(BIT(5))
+#define FM10K_FLAG_RETA_TABLE_CONFIGURED	(u32)(BIT(6))
 	int xcast_mode;
 
 	/* Tx fast path data */

@@ -475,6 +476,7 @@ netdev_tx_t fm10k_xmit_frame_ring(struct sk_buff *skb,
 void fm10k_tx_timeout_reset(struct fm10k_intfc *interface);
 bool fm10k_check_tx_hang(struct fm10k_ring *tx_ring);
 void fm10k_alloc_rx_buffers(struct fm10k_ring *rx_ring, u16 cleaned_count);
+void fm10k_init_reta(struct fm10k_intfc *interface);
 
 /* PCI */
 void fm10k_mbx_free_irq(struct fm10k_intfc *);

diff --git a/drivers/net/ethernet/intel/fm10k/fm10k_ethtool.c b/drivers/net/ethernet/intel/fm10k/fm10k_ethtool.c
index 6a9f9886cb98..febfa2b009ea 100644
--- a/drivers/net/ethernet/intel/fm10k/fm10k_ethtool.c
+++ b/drivers/net/ethernet/intel/fm10k/fm10k_ethtool.c

@@ -1060,6 +1060,8 @@ static int fm10k_set_reta(struct net_device *netdev, const u32 *indir)
 	if (!indir)
 		return 0;
 
+	interface->flags |= FM10K_FLAG_RETA_TABLE_CONFIGURED;
+
 	/* Verify user input. */
 	rss_i = interface->ring_feature[RING_F_RSS].indices;
 	for (i = fm10k_get_reta_size(netdev); i--;) {

@@ -1137,6 +1139,25 @@ static int fm10k_set_rssh(struct net_device *netdev, const u32 *indir,
 	return 0;
 }
 
+static int fm10k_reset_rssh(struct net_device *netdev)
+{
+	struct fm10k_intfc *interface = netdev_priv(netdev);
+	struct fm10k_hw *hw = &interface->hw;
+	int i;
+
+	/* user has requested default configuration so clear configured flag */
+	interface->flags &= ~FM10K_FLAG_RETA_TABLE_CONFIGURED;
+
+	/* initialize the reta table to driver defaults */
+	fm10k_init_reta(interface);
+
+	/* write the new RETA table to hardware */
+	for (i = 0; i < FM10K_RETA_SIZE; i++)
+		fm10k_write_reg(hw, FM10K_RETA(0, i), interface->reta[i]);
+
+	return 0;
+}
+
 static unsigned int fm10k_max_channels(struct net_device *dev)
 {
 	struct fm10k_intfc *interface = netdev_priv(dev);

@@ -1173,6 +1194,8 @@ static int fm10k_set_channels(struct net_device *dev,
 	struct fm10k_intfc *interface = netdev_priv(dev);
 	unsigned int count = ch->combined_count;
 	struct fm10k_hw *hw = &interface->hw;
+	u32 reta0, reta1, reta2, reta3;
+	int i, rss_i = 0;
 
 	/* verify they are not requesting separate vectors */
 	if (!count || ch->rx_count || ch->tx_count)

@@ -1186,6 +1209,27 @@ static int fm10k_set_channels(struct net_device *dev,
 	if (count > fm10k_max_channels(dev))
 		return -EINVAL;
 
+	/* determine the current number of queues used by the reta table */
+	for (i = FM10K_RETA_SIZE; i--;) {
+		reta0 = (interface->reta[i] << 24) >> 24;
+		reta1 = (interface->reta[i] << 16) >> 24;
+		reta2 = (interface->reta[i] <<  8) >> 24;
+		reta3 = (interface->reta[i])       >> 24;
+
+		if (reta0 > rss_i)
+			rss_i = reta0;
+		if (reta1 > rss_i)
+			rss_i = reta1;
+		if (reta2 > rss_i)
+			rss_i = reta2;
+		if (reta3 > rss_i)
+			rss_i = reta3;
+	}
+
+	/* verify the request doesn't invalidate the current reta table */
+	if (count < rss_i)
+		return -EINVAL;
+
 	interface->ring_feature[RING_F_RSS].limit = count;
 
 	/* use setup TC to update any traffic class queue mapping */

@@ -1242,6 +1286,7 @@ static const struct ethtool_ops fm10k_ethtool_ops = {
 	.get_rxfh_key_size	= fm10k_get_rssrk_size,
 	.get_rxfh		= fm10k_get_rssh,
 	.set_rxfh		= fm10k_set_rssh,
+	.reset_rxfh_indir	= fm10k_reset_rssh,
 	.get_channels		= fm10k_get_channels,
 	.set_channels		= fm10k_set_channels,
 	.get_ts_info            = fm10k_get_ts_info,

diff --git a/drivers/net/ethernet/intel/fm10k/fm10k_main.c b/drivers/net/ethernet/intel/fm10k/fm10k_main.c
index 134ce4daa994..54205d3da29e 100644
--- a/drivers/net/ethernet/intel/fm10k/fm10k_main.c
+++ b/drivers/net/ethernet/intel/fm10k/fm10k_main.c

@@ -1932,13 +1932,14 @@ static void fm10k_assign_rings(struct fm10k_intfc *interface)
 	fm10k_cache_ring_rss(interface);
 }
 
-static void fm10k_init_reta(struct fm10k_intfc *interface)
+void fm10k_init_reta(struct fm10k_intfc *interface)
 {
 	u16 i, rss_i = interface->ring_feature[RING_F_RSS].indices;
 	u32 reta, base;
 
-	/* If the netdev is initialized we have to maintain table if possible */
-	if (interface->netdev->reg_state != NETREG_UNINITIALIZED) {
+	/* if the redirection table is user-configured, we must maintain it
+	 * when possible. */
+	if (interface->flags & FM10K_FLAG_RETA_TABLE_CONFIGURED) {
 		for (i = FM10K_RETA_SIZE; i--;) {
 			reta = interface->reta[i];
 			if ((((reta << 24) >> 24) < rss_i) &&

@@ -1946,6 +1947,10 @@ static void fm10k_init_reta(struct fm10k_intfc *interface)
 			    (((reta <<  8) >> 24) < rss_i) &&
 			    (((reta)       >> 24) < rss_i))
 				continue;
+
+			/* this should never happen */
+			dev_err(&interface->pdev->dev,
+				"RSS indirection table is invalid and must be reconfigured\n");
 			goto repopulate_reta;
 		}
 

-- 
2.7.0.236.gda096a0.dirty

Hi Jacob!

First of all thanks for putting your time into sorting this out,
figuring out what to do with user-set RSS table when queues are
reconfigured was a head scratcher for me as well.

On Fri,  5 Feb 2016 12:30:21 -0800, Jacob Keller wrote:

+#define FM10K_FLAG_RETA_TABLE_CONFIGURED	(u32)(BIT(6))

If we go with your proposal every driver will have to keep track of 
how the RSS table was set and find max value on queue reconfig -
replicating effort and leaving space for diverging behaviour...

Would it be worth considering to place more of this code in the core?

On Mon, 2016-02-08 at 13:26 +0000, Jakub Kicinski wrote:

Hi Jacob!

First of all thanks for putting your time into sorting this out,
figuring out what to do with user-set RSS table when queues are
reconfigured was a head scratcher for me as well.

Yep!

On Fri,  5 Feb 2016 12:30:21 -0800, Jacob Keller wrote:

quoted

+#define FM10K_FLAG_RETA_TABLE_CONFIGURED	(u32)(BIT(6))

If we go with your proposal every driver will have to keep track of 
how the RSS table was set and find max value on queue reconfig -
replicating effort and leaving space for diverging behaviour...

in which behavior has already diverged quite significantly, so shoring
that up would be good as well.

Would it be worth considering to place more of this code in the core?

Yes. I was unsure of how to do this, but I think I have a possible
solution. Since basically all drivers are going to have the same issue,
I think we can just do the check inside net/core/ethtool.c

At least some of the check can be done inside core ethtool, but I think
we still need a way for driver to know it is in "default" mode, as the
driver does behave differently in its reset flow depending on whether
the RSS table has been set.

Maybe we can store it as a flag in the netdev structure instead?

I do agree that the queue size reconfig can handle the new minimum
queue value easily.

Regards,
Jake

On Mon, 8 Feb 2016 17:13:15 +0000, Keller, Jacob E wrote:

On Mon, 2016-02-08 at 13:26 +0000, Jakub Kicinski wrote:

quoted

On Fri,  5 Feb 2016 12:30:21 -0800, Jacob Keller wrote:

quoted

+#define FM10K_FLAG_RETA_TABLE_CONFIGURED	(u32)(BIT(6))

If we go with your proposal every driver will have to keep track of 
how the RSS table was set and find max value on queue reconfig -
replicating effort and leaving space for diverging behaviour...

in which behavior has already diverged quite significantly, so shoring
that up would be good as well.

quoted

Would it be worth considering to place more of this code in the core?

Yes. I was unsure of how to do this, but I think I have a possible
solution. Since basically all drivers are going to have the same issue,
I think we can just do the check inside net/core/ethtool.c

At least some of the check can be done inside core ethtool, but I think
we still need a way for driver to know it is in "default" mode, as the
driver does behave differently in its reset flow depending on whether
the RSS table has been set.

Maybe we can store it as a flag in the netdev structure instead?

Either flag in the netdev or keep your reset callback.  Neither seems
spectacularly clean.  Maybe someone with a better idea will speak up ;)

On Mon, 2016-02-08 at 19:23 +0000, Jakub Kiciński wrote:

On Mon, 8 Feb 2016 17:13:15 +0000, Keller, Jacob E wrote:

quoted

Maybe we can store it as a flag in the netdev structure instead?

Either flag in the netdev or keep your reset callback.  Neither seems
spectacularly clean.  Maybe someone with a better idea will speak up
;)

I chose a priv_flags for netdev, since I think this is better than a
new ethtool op.

I Cc'ed you on the new patch series if you could give it a look? I also
added a few more sanity checks to ethtool_set_channels, in addition to
the check for the rxfh flow table.

Regards,
Jake