diff mbox

[net-next,3/4] ethtool: Support for configurable RSS hash key.

Message ID BF3270C86E8B1349A26C34E4EC1C44CB2C83D8E5@CMEXMB1.ad.emulex.com
State Changes Requested, archived
Delegated to: David Miller
Headers show

Commit Message

Venkata Duvvuru Jan. 17, 2014, 1:02 p.m. UTC 
This ethtool patch primarily copies the ioctl command data structures from/to the User space and invokes the driver hook.

Signed-off-by: Venkat Duvvuru <VenkatKumar.Duvvuru@Emulex.com>
---
 include/linux/ethtool.h      |    2 ++
 include/uapi/linux/ethtool.h |   19 +++++++++++++++++++
 net/core/ethtool.c           |   40 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 61 insertions(+), 0 deletions(-)

--
1.7.1

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Ben Hutchings Jan. 19, 2014, 6:27 p.m. UTC | #1 
On Fri, 2014-01-17 at 13:02 +0000, Venkata Duvvuru wrote:
> This ethtool patch primarily copies the ioctl command data structures from/to the User space and invokes the driver hook.
[...]
> --- a/include/uapi/linux/ethtool.h
> +++ b/include/uapi/linux/ethtool.h
> @@ -678,6 +678,22 @@ struct ethtool_rx_ntuple {
>  	struct ethtool_rx_ntuple_flow_spec	fs;
>  };
>  
> +
> +/**
> + * struct ethtool_rss_hkey - command to set/get RSS hash key of the device.
> + * @cmd: Command number - %ETHTOOL_SET_RSS_HKEY/ETHTOOL_GET_RSS_HKEY
> + * @data: 40 or 16 byte rss hash key
> + * @data_len: rss hash key length
> + */
> +
> +#define RSS_HASH_KEY_LEN	40

This should have an 'ETHTOOL_' or 'ETH_' prefix.  But I wonder whether
there should actually be a static maximum length.

> +/* RSS Hash key */
> +struct ethtool_rss_hkey {
> +	__u32   cmd;            /* ETHTOOL_SET/GET_RSS_HKEY */
> +	__u8    data[RSS_HASH_KEY_LEN];
> +	__u32	data_len;
> +};
[...]

How about putting data after the data_len and giving it a length of 0,
so this is extensible to an arbitrary length key?

If we're extending the RSS configuration interface, there are a few
other things that might be worth doing at the same time:

- Single commands to get/set both the key and the indirection table at
the same time
- Add a field to distinguish multiple RSS contexts (some hardware can
use RSS contexts together with filters, though RX NFC does not support
that yet)

Ben.
Venkata Duvvuru Jan. 20, 2014, 12:23 p.m. UTC | #2 
> -----Original Message-----

> From: Ben Hutchings [mailto:ben@decadent.org.uk]

> Sent: Sunday, January 19, 2014 11:57 PM

> To: Venkata Duvvuru

> Cc: netdev@vger.kernel.org

> Subject: Re: [PATCH net-next 3/4] ethtool: Support for configurable RSS hash

> key.

> 

> On Fri, 2014-01-17 at 13:02 +0000, Venkata Duvvuru wrote:

> > This ethtool patch primarily copies the ioctl command data structures

> from/to the User space and invokes the driver hook.

> [...]

> > --- a/include/uapi/linux/ethtool.h

> > +++ b/include/uapi/linux/ethtool.h

> > @@ -678,6 +678,22 @@ struct ethtool_rx_ntuple {

> >  	struct ethtool_rx_ntuple_flow_spec	fs;

> >  };

> >

> > +

> > +/**

> > + * struct ethtool_rss_hkey - command to set/get RSS hash key of the

> device.

> > + * @cmd: Command number -

> %ETHTOOL_SET_RSS_HKEY/ETHTOOL_GET_RSS_HKEY

> > + * @data: 40 or 16 byte rss hash key

> > + * @data_len: rss hash key length

> > + */

> > +

> > +#define RSS_HASH_KEY_LEN	40

> 

> This should have an 'ETHTOOL_' or 'ETH_' prefix.  But I wonder whether there

> should actually be a static maximum length.

> 

> > +/* RSS Hash key */

> > +struct ethtool_rss_hkey {

> > +	__u32   cmd;            /* ETHTOOL_SET/GET_RSS_HKEY */

> > +	__u8    data[RSS_HASH_KEY_LEN];

> > +	__u32	data_len;

> > +};

> [...]

> 

> How about putting data after the data_len and giving it a length of 0, so this is

> extensible to an arbitrary length key?

> 

> If we're extending the RSS configuration interface, there are a few other

> things that might be worth doing at the same time:

> 

> - Single commands to get/set both the key and the indirection table at the

> same time

> - Add a field to distinguish multiple RSS contexts (some hardware can use RSS

> contexts together with filters, though RX NFC does not support that yet)

Are you referring to the filter-id that is created at the time of config-nfc? Pls clarify.
> 

> Ben.

> 

> --

> Ben Hutchings

> friends: People who know you well, but like you anyway.
Ben Hutchings Jan. 20, 2014, 1:20 p.m. UTC | #3 
On Mon, 2014-01-20 at 12:23 +0000, Venkata Duvvuru wrote:
[...]
> > > +/* RSS Hash key */
> > > +struct ethtool_rss_hkey {
> > > +	__u32   cmd;            /* ETHTOOL_SET/GET_RSS_HKEY */
> > > +	__u8    data[RSS_HASH_KEY_LEN];
> > > +	__u32	data_len;
> > > +};
> > [...]
> > 
> > How about putting data after the data_len and giving it a length of 0, so this is
> > extensible to an arbitrary length key?
> > 
> > If we're extending the RSS configuration interface, there are a few other
> > things that might be worth doing at the same time:
> > 
> > - Single commands to get/set both the key and the indirection table at the
> > same time
> > - Add a field to distinguish multiple RSS contexts (some hardware can use RSS
> > contexts together with filters, though RX NFC does not support that yet)
> Are you referring to the filter-id that is created at the time of config-nfc? Pls clarify.

No, what I mean is:

1. An RX flow steering filter can specify use of RSS, in which case the
value looked up in the indirection is added to the queue number
specified in the filter.  This is not yet controllable through RX NFC
though there is room for extension there.

2. Multi-function controllers need multiple RSS contexts (key +
indirection table) to support independent use of RSS on each function.
But it may also be possible to allocate multiple contexts to a single
function.  This could be useful in conjunction with 1.  But there would
need to be a way to allocate and configure extra contexts first.

Ben.
Ben Hutchings Jan. 23, 2014, 5:39 a.m. UTC | #4 
On Wed, 2014-01-22 at 12:12 +0000, Venkata Duvvuru wrote:
[...]
> > No, what I mean is:
> > 
> > 1. An RX flow steering filter can specify use of RSS, in which case the value
> > looked up in the indirection is added to the queue number specified in the
> > filter.  This is not yet controllable through RX NFC though there is room for
> > extension there.
> > 
> > 2. Multi-function controllers need multiple RSS contexts (key + indirection
> > table) to support independent use of RSS on each function.
> > But it may also be possible to allocate multiple contexts to a single function.
> > This could be useful in conjunction with 1.  But there would need to be a way
> > to allocate and configure extra contexts first.
> The proposed changes will be incremental so I think this can be done
> in a separate patch. Thoughts?

The ethtool ABI (to userland) has to remain backward-compatible, and it
is preferable if we don't add lots of different structures for this.

So please define the new command structure to include both the key and
indirection table, and some reserved space (documented as 'userland must
set to 0') for future extensions.

Ben.
Venkata Duvvuru Jan. 23, 2014, 1:47 p.m. UTC | #5 
> -----Original Message-----

> From: Ben Hutchings [mailto:ben@decadent.org.uk]

> Sent: Thursday, January 23, 2014 11:09 AM

> To: Venkata Duvvuru

> Cc: netdev@vger.kernel.org

> Subject: Re: [PATCH net-next 3/4] ethtool: Support for configurable RSS hash

> key.

> 

> On Wed, 2014-01-22 at 12:12 +0000, Venkata Duvvuru wrote:

> [...]

> > > No, what I mean is:

> > >

> > > 1. An RX flow steering filter can specify use of RSS, in which case

> > > the value looked up in the indirection is added to the queue number

> > > specified in the filter.  This is not yet controllable through RX

> > > NFC though there is room for extension there.

> > >

> > > 2. Multi-function controllers need multiple RSS contexts (key +

> > > indirection

> > > table) to support independent use of RSS on each function.

> > > But it may also be possible to allocate multiple contexts to a single

> function.

> > > This could be useful in conjunction with 1.  But there would need to

> > > be a way to allocate and configure extra contexts first.

> > The proposed changes will be incremental so I think this can be done

> > in a separate patch. Thoughts?

> 

> The ethtool ABI (to userland) has to remain backward-compatible, and it is

> preferable if we don't add lots of different structures for this.

> 

> So please define the new command structure to include both the key and

> indirection table, and some reserved space (documented as 'userland must

> set to 0') for future extensions.


I think it’s better to keep key and indirection table settings as different ethtool commands. We can probably add rss contexts (reserved space) to both the command structures.
If we mix key and indirection table into one command structure then it will hamper the compatibility.

Current structure:
struct ethtool_rxfh_indir {
        __u32   cmd;
        __u32   size;
        __u32   ring_index[0];
};

For example, 
1. You mentioned about having key as zero byte array, there is already a zero byte array “ring_index”. We can probably have a single zero byte array and allocate both indir + key memory. This  may not work as the old ethtool would not allocate memory for key and the newer kernel will try to interpret key which could be relatively junk or invalid addresses.
2. Even if we assume that key will be static array. We have to have the key field before the zero byte array “ring_index”. This also will not be compatible as we are introducing a field in the middle.


> 

> Ben.

> 

> --

> Ben Hutchings

> compatible: Gracefully accepts erroneous data from any source
Ben Hutchings Jan. 23, 2014, 3:09 p.m. UTC | #6 
On Thu, 2014-01-23 at 13:47 +0000, Venkata Duvvuru wrote:
> > -----Original Message-----
> > From: Ben Hutchings [mailto:ben@decadent.org.uk]
> > Sent: Thursday, January 23, 2014 11:09 AM
> > To: Venkata Duvvuru
> > Cc: netdev@vger.kernel.org
> > Subject: Re: [PATCH net-next 3/4] ethtool: Support for configurable RSS hash
> > key.
> > 
> > On Wed, 2014-01-22 at 12:12 +0000, Venkata Duvvuru wrote:
> > [...]
> > > > No, what I mean is:
> > > >
> > > > 1. An RX flow steering filter can specify use of RSS, in which case
> > > > the value looked up in the indirection is added to the queue number
> > > > specified in the filter.  This is not yet controllable through RX
> > > > NFC though there is room for extension there.
> > > >
> > > > 2. Multi-function controllers need multiple RSS contexts (key +
> > > > indirection
> > > > table) to support independent use of RSS on each function.
> > > > But it may also be possible to allocate multiple contexts to a single
> > function.
> > > > This could be useful in conjunction with 1.  But there would need to
> > > > be a way to allocate and configure extra contexts first.
> > > The proposed changes will be incremental so I think this can be done
> > > in a separate patch. Thoughts?
> > 
> > The ethtool ABI (to userland) has to remain backward-compatible, and it is
> > preferable if we don't add lots of different structures for this.
> > 
> > So please define the new command structure to include both the key and
> > indirection table, and some reserved space (documented as 'userland must
> > set to 0') for future extensions.
> 
> I think it’s better to keep key and indirection table settings as
> different ethtool commands. We can probably add rss contexts (reserved
> space) to both the command structures.
> If we mix key and indirection table into one command structure then it
> will hamper the compatibility.
[...]

Right, there is no compatible way to extend struct ethtool_rxfh_indir.
I should have thought ahead when defining it!  But the new structure
doesn't need to have that problem.

Ben.
diff mbox

Patch

diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h index c8e3e7e..2c30fd1 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -227,6 +227,8 @@  struct ethtool_ops {
 	int	(*get_rxnfc)(struct net_device *,
 			     struct ethtool_rxnfc *, u32 *rule_locs);
 	int	(*set_rxnfc)(struct net_device *, struct ethtool_rxnfc *);
+	int     (*set_rsshkey) (struct net_device *, struct ethtool_rss_hkey *);
+	int     (*get_rsshkey) (struct net_device *, struct ethtool_rss_hkey *);
 	int	(*flash_device)(struct net_device *, struct ethtool_flash *);
 	int	(*reset)(struct net_device *, u32 *);
 	u32	(*get_rxfh_indir_size)(struct net_device *);
diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h index 38dbafa..f39d82f 100644
--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h
@@ -678,6 +678,22 @@  struct ethtool_rx_ntuple {
 	struct ethtool_rx_ntuple_flow_spec	fs;
 };
 
+
+/**
+ * struct ethtool_rss_hkey - command to set/get RSS hash key of the device.
+ * @cmd: Command number - %ETHTOOL_SET_RSS_HKEY/ETHTOOL_GET_RSS_HKEY
+ * @data: 40 or 16 byte rss hash key
+ * @data_len: rss hash key length
+ */
+
+#define RSS_HASH_KEY_LEN	40
+/* RSS Hash key */
+struct ethtool_rss_hkey {
+	__u32   cmd;            /* ETHTOOL_SET/GET_RSS_HKEY */
+	__u8    data[RSS_HASH_KEY_LEN];
+	__u32	data_len;
+};
+
 #define ETHTOOL_FLASH_MAX_FILENAME	128
 enum ethtool_flash_op_type {
 	ETHTOOL_FLASH_ALL_REGIONS	= 0,
@@ -901,6 +917,9 @@  enum ethtool_sfeatures_retval_bits {
 #define ETHTOOL_GEEE		0x00000044 /* Get EEE settings */
 #define ETHTOOL_SEEE		0x00000045 /* Set EEE settings */
 
+#define ETHTOOL_SET_RSS_HKEY	0x00000046 /* Set RSS hash key */
+#define ETHTOOL_GET_RSS_HKEY	0x00000047 /* Get RSS hash key */
+
 /* compatibility with older code */
 #define SPARC_ETH_GSET		ETHTOOL_GSET
 #define SPARC_ETH_SSET		ETHTOOL_SSET
diff --git a/net/core/ethtool.c b/net/core/ethtool.c index 30071de..70f68ff 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -501,6 +501,40 @@  static noinline_for_stack int ethtool_set_rxnfc(struct net_device *dev,
 	return 0;
 }
 
+static noinline_for_stack int ethtool_get_rsshkey(struct net_device *dev,
+						  void __user *useraddr)
+{
+	struct ethtool_rss_hkey info;
+	int rc;
+
+	if (!dev->ethtool_ops->get_rsshkey)
+		return -EOPNOTSUPP;
+
+	rc = dev->ethtool_ops->get_rsshkey(dev, &info);
+
+	if (copy_to_user(useraddr, &info, sizeof(info)))
+		rc = -EFAULT;
+
+	return rc;
+}
+
+static noinline_for_stack int ethtool_set_rsshkey(struct net_device *dev,
+						  void __user *useraddr)
+{
+	struct ethtool_rss_hkey info;
+	int rc;
+
+	if (!dev->ethtool_ops->set_rsshkey)
+		return -EOPNOTSUPP;
+
+	if (copy_from_user(&info, useraddr, sizeof(info)))
+		return -EFAULT;
+
+	rc = dev->ethtool_ops->set_rsshkey(dev, &info);
+
+	return rc;
+}
+
 static noinline_for_stack int ethtool_get_rxnfc(struct net_device *dev,
 						u32 cmd, void __user *useraddr)
 {
@@ -1612,6 +1646,12 @@  int dev_ethtool(struct net *net, struct ifreq *ifr)
 	case ETHTOOL_SRXCLSRLINS:
 		rc = ethtool_set_rxnfc(dev, ethcmd, useraddr);
 		break;
+	case ETHTOOL_GET_RSS_HKEY:
+		rc = ethtool_get_rsshkey(dev, useraddr);
+		break;
+	case ETHTOOL_SET_RSS_HKEY:
+		rc = ethtool_set_rsshkey(dev, useraddr);
+		break;
 	case ETHTOOL_FLASHDEV:
 		rc = ethtool_flash_device(dev, useraddr);
 		break;