[RFC PATCH net-2.6] net/ethtool: add multiple queue support to {get,set}_ringparams

[RFC PATCH net-2.6] net/ethtool: add multiple queue support to {get,set}_ringparams

[Ajit Khaparde]

With network devices and hence device drivers supporting
multiple Tx and Rx rings, currently there is no way for
ethtool to specify which Tx/Rx ring the user wants to get/set.
This patch enhances the {get,set}_ringparams by allowing
the user to specify the Tx/Rx ring id of interest.
Please review.

Signed-off-by: Ajit Khaparde <ajitk@serverengines.com>
---
 include/linux/ethtool.h |    1 +
 net/core/ethtool.c      |    5 ++++-
 2 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index b33f316..de6a90a 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -206,6 +206,7 @@ struct ethtool_coalesce {
 /* for configuring RX/TX ring parameters */
 struct ethtool_ringparam {
 	__u32	cmd;	/* ETHTOOL_{G,S}RINGPARAM */
+	__u32	ring_id;
 
 	/* Read only attributes.  These indicate the maximum number
 	 * of pending RX/TX ring entries the driver will allow the
diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index f4cb6b6..81e2e62 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -881,11 +881,14 @@ static noinline_for_stack int ethtool_set_coalesce(struct net_device *dev, void
 
 static int ethtool_get_ringparam(struct net_device *dev, void __user *useraddr)
 {
-	struct ethtool_ringparam ringparam = { .cmd = ETHTOOL_GRINGPARAM };
+	struct ethtool_ringparam ringparam;
 
 	if (!dev->ethtool_ops->get_ringparam)
 		return -EOPNOTSUPP;
 
+	if (copy_from_user(&ringparam, useraddr, sizeof(ringparam)))
+		return -EFAULT;
+
 	dev->ethtool_ops->get_ringparam(dev, &ringparam);
 
 	if (copy_to_user(useraddr, &ringparam, sizeof(ringparam)))
-- 
1.6.3.3

Re: [RFC PATCH net-2.6] net/ethtool: add multiple queue support to {get,set}_ringparams

[David Miller]

From: Ajit Khaparde <ajitkhaparde@gmail.com>
Date: Sun, 14 Mar 2010 07:13:45 +0530

> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
> index b33f316..de6a90a 100644
> --- a/include/linux/ethtool.h
> +++ b/include/linux/ethtool.h
> @@ -206,6 +206,7 @@ struct ethtool_coalesce {
>  /* for configuring RX/TX ring parameters */
>  struct ethtool_ringparam {
>  	__u32	cmd;	/* ETHTOOL_{G,S}RINGPARAM */
> +	__u32	ring_id;
>  
>  	/* Read only attributes.  These indicate the maximum number
>  	 * of pending RX/TX ring entries the driver will allow the

For the millionth time, you cannot change these datastructures
like this without breaking all existing userspace applications
out there.

[PATCH] ethtool.h: Add "structs are public" disclaimer comment

[Joe Perches]

On Sat, 2010-03-13 at 18:11 -0800, David Miller wrote:
> For the millionth time, you cannot change these datastructures
> like this without breaking all existing userspace applications
> out there.

Maybe this might help reduce the broken record repetitiveness.

Signed-off-by: Joe Perches <joe@perches.com>
---
diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index b33f316..9bd7583 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -10,6 +10,15 @@
  * Portions Copyright (C) Sun Microsystems 2008
  */
 
+/*
+ * Do not submit patches that change the public structs not guarded by
+ * #ifdef __KERNEL__ in this file.
+ *
+ * In case the use of __u8, __u16, __u32 and other reserved types don't
+ * mean much to you, these structs are used by user-space applications
+ * and must not be changed.
+ */
+
 #ifndef _LINUX_ETHTOOL_H
 #define _LINUX_ETHTOOL_H
 

Re: [PATCH] ethtool.h: Add "structs are public" disclaimer comment

[Ben Hutchings]

On Sun, 2010-03-14 at 09:17 -0700, Joe Perches wrote:
> On Sat, 2010-03-13 at 18:11 -0800, David Miller wrote:
> > For the millionth time, you cannot change these datastructures
> > like this without breaking all existing userspace applications
> > out there.
> 
> Maybe this might help reduce the broken record repetitiveness.
> 
> Signed-off-by: Joe Perches <joe@perches.com>
> ---
> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
> index b33f316..9bd7583 100644
> --- a/include/linux/ethtool.h
> +++ b/include/linux/ethtool.h
> @@ -10,6 +10,15 @@
>   * Portions Copyright (C) Sun Microsystems 2008
>   */
>  
> +/*
> + * Do not submit patches that change the public structs not guarded by
> + * #ifdef __KERNEL__ in this file.
> + *
> + * In case the use of __u8, __u16, __u32 and other reserved types don't
> + * mean much to you, these structs are used by user-space applications
> + * and must not be changed.
> + */

This comment is not quite correct: reserved fields may be replaced so
long as the structure size and offsets of other fields stay the same.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH] ethtool.h: Add "structs are public" disclaimer comment

[Jeff Garzik]

On 03/14/2010 01:50 PM, Ben Hutchings wrote:
> On Sun, 2010-03-14 at 09:17 -0700, Joe Perches wrote:
>> On Sat, 2010-03-13 at 18:11 -0800, David Miller wrote:
>>> For the millionth time, you cannot change these datastructures
>>> like this without breaking all existing userspace applications
>>> out there.
>>
>> Maybe this might help reduce the broken record repetitiveness.
>>
>> Signed-off-by: Joe Perches<joe@perches.com>
>> ---
>> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
>> index b33f316..9bd7583 100644
>> --- a/include/linux/ethtool.h
>> +++ b/include/linux/ethtool.h
>> @@ -10,6 +10,15 @@
>>    * Portions Copyright (C) Sun Microsystems 2008
>>    */
>>
>> +/*
>> + * Do not submit patches that change the public structs not guarded by
>> + * #ifdef __KERNEL__ in this file.
>> + *
>> + * In case the use of __u8, __u16, __u32 and other reserved types don't
>> + * mean much to you, these structs are used by user-space applications
>> + * and must not be changed.
>> + */
>
> This comment is not quite correct: reserved fields may be replaced so
> long as the structure size and offsets of other fields stay the same.

I don't think we need this in every ABI-related header, though, do we?

	Jeff

Re: [PATCH] ethtool.h: Add "structs are public" disclaimer comment

[David Miller]

From: Joe Perches <joe@perches.com>
Date: Sun, 14 Mar 2010 09:17:58 -0700

> On Sat, 2010-03-13 at 18:11 -0800, David Miller wrote:
>> For the millionth time, you cannot change these datastructures
>> like this without breaking all existing userspace applications
>> out there.
> 
> Maybe this might help reduce the broken record repetitiveness.
> 
> Signed-off-by: Joe Perches <joe@perches.com>

That's rediculious.  Just about every single file in
include/linux/foo.h has similar stuff.

And we already have a way to document this, the fact that a header
file appears in include/linux/Kbuild shows that it provides userland
interfaces.

And your comment wouldn't help this person at all, they _KNEW_ it
changed userspace, because in the very next patch they patch the
userland tool to change the datastructures too!

Re: [RFC PATCH net-2.6] net/ethtool: add multiple queue support to {get,set}_ringparams

[Ben Hutchings]

On Sun, 2010-03-14 at 07:13 +0530, Ajit Khaparde wrote:
> With network devices and hence device drivers supporting
> multiple Tx and Rx rings, currently there is no way for
> ethtool to specify which Tx/Rx ring the user wants to get/set.
> This patch enhances the {get,set}_ringparams by allowing
> the user to specify the Tx/Rx ring id of interest.
> Please review.
[...]

So long as queue selection is done using a hash of the flow parameters,
there is no sense in setting different sizes for different queues.  If
drivers use additional queues that are not selected in this way and
which may require different sizes, that information also needs to be
exposed through ethtool (and perhaps to the networking core).

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.