[PATCHv3 NEXT 1/1] net: ethtool support to configure number of channels

[PATCHv3 NEXT 1/1] net: ethtool support to configure number of channels

[Amit Kumar Salecha]

o There exist ETHTOOL_GRXRINGS command for getting number of RX rings,
  but it is tigtly coupled with RX flow hash configuration.
o Patches for qlcnic and netxen_nic driver supporting rx channel will follow
  soon.
o This patch is reworked on Ben Hutchings "ethtool: NUMA affinity control" patch,
  dropping the affinity control from it.

Ben Hutching:
o define 'combined' and 'other' types.  Most multiqueue drivers pair up RX and TX
  queues so that most channels combine RX and TX work.
o Please could you use a kernel-doc comment to describe the structure.

Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
Signed-off-by: Amit Kumar Salecha <amit.salecha@qlogic.com>
---
 include/linux/ethtool.h |   41 +++++++++++++++++++++++++++++++++++++++++
 net/core/ethtool.c      |   41 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 82 insertions(+), 0 deletions(-)

diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index c8fcbdd..3c4d968 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -229,6 +229,42 @@ struct ethtool_ringparam {
 	__u32	tx_pending;
 };
 
+/**
+ * struct ethtool_channels - configuring number of network channel
+ * @cmd: ETHTOOL_{G,S}CHANNELS
+ * @type: Channel type defined in ethtool_channel_type
+ * @max_XX: Read only. Maximum number of channel the driver support
+ * @XX_count: Valid values are in the range 1 to the "max_XX" counterpart above.
+ *
+ * This can be used to configure RX,TX and other channels.
+ */
+
+struct ethtool_channels {
+	__u32	cmd;
+	__u32	type;
+	__u32	max_rx;
+	__u32	max_tx;
+	__u32	max_other;
+	__u32	rx_count;
+	__u32	tx_count;
+	__u32	other_count;
+};
+
+/* Channel type */
+/* Channel type should be pass in type field of ethtool_channels.
+ * TYPE_ALL indicates set all channels to XX_count values.
+ * TYPE_RX and TYPE_TX is to get and set RX and TX channels correspondingly.
+ * TYPE_COMBINED is to set both RX and TX channels to rx_count and tx_count
+ * correspondingly. TYPE_OTHER is to configure other channel.
+ */
+enum ethtool_channel_type {
+	ETH_CHAN_TYPE_ALL	= 0,
+	ETH_CHAN_TYPE_RX,
+	ETH_CHAN_TYPE_TX,
+	ETH_CHAN_TYPE_COMBINED,
+	ETH_CHAN_TYPE_OTHER,
+};
+
 /* for configuring link flow control parameters */
 struct ethtool_pauseparam {
 	__u32	cmd;	/* ETHTOOL_{G,S}PAUSEPARAM */
@@ -802,6 +838,9 @@ struct ethtool_ops {
 				  struct ethtool_rxfh_indir *);
 	int	(*set_rxfh_indir)(struct net_device *,
 				  const struct ethtool_rxfh_indir *);
+	int	(*get_channels)(struct net_device *, struct ethtool_channels *);
+	int	(*set_channels)(struct net_device *, struct ethtool_channels *);
+
 };
 #endif /* __KERNEL__ */
 
@@ -870,6 +909,8 @@ struct ethtool_ops {
 
 #define ETHTOOL_GFEATURES	0x0000003a /* Get device offload settings */
 #define ETHTOOL_SFEATURES	0x0000003b /* Change device offload settings */
+#define ETHTOOL_GCHANNELS	0x0000003c /* Get no of channels */
+#define ETHTOOL_SCHANNELS	0x0000003d /* Set no of channels */
 
 /* compatibility with older code */
 #define SPARC_ETH_GSET		ETHTOOL_GSET
diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index 74ead9e..91c3c6d 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -1441,6 +1441,41 @@ static int ethtool_set_ringparam(struct net_device *dev, void __user *useraddr)
 	return dev->ethtool_ops->set_ringparam(dev, &ringparam);
 }
 
+static noinline_for_stack int ethtool_get_channels(struct net_device *dev,
+						   void __user *useraddr)
+{
+	struct ethtool_channels channels;
+	int rc;
+
+	if (!dev->ethtool_ops->get_channels)
+		return -EOPNOTSUPP;
+
+	if (copy_from_user(&channels, useraddr, sizeof(channels)))
+		return -EFAULT;
+
+	rc = dev->ethtool_ops->get_channels(dev, &channels);
+	if (rc)
+		return rc;
+
+	if (copy_to_user(useraddr, &channels, sizeof(channels)))
+		return -EFAULT;
+	return 0;
+}
+
+static noinline_for_stack int ethtool_set_channels(struct net_device *dev,
+						   void __user *useraddr)
+{
+	struct ethtool_channels channels;
+
+	if (!dev->ethtool_ops->set_channels)
+		return -EOPNOTSUPP;
+
+	if (copy_from_user(&channels, useraddr, sizeof(channels)))
+		return -EFAULT;
+
+	return dev->ethtool_ops->set_channels(dev, &channels);
+}
+
 static int ethtool_get_pauseparam(struct net_device *dev, void __user *useraddr)
 {
 	struct ethtool_pauseparam pauseparam = { ETHTOOL_GPAUSEPARAM };
@@ -1953,6 +1988,12 @@ int dev_ethtool(struct net *net, struct ifreq *ifr)
 	case ETHTOOL_SGRO:
 		rc = ethtool_set_one_feature(dev, useraddr, ethcmd);
 		break;
+	case ETHTOOL_GCHANNELS:
+		rc = ethtool_get_channels(dev, useraddr);
+		break;
+	case ETHTOOL_SCHANNELS:
+		rc = ethtool_set_channels(dev, useraddr);
+		break;
 	default:
 		rc = -EOPNOTSUPP;
 	}
-- 
1.7.3.2

Re: [PATCHv3 NEXT 1/1] net: ethtool support to configure number of channels

[David Miller]

From: Amit Kumar Salecha <amit.salecha@qlogic.com>
Date: Fri,  1 Apr 2011 21:58:03 -0700

> o There exist ETHTOOL_GRXRINGS command for getting number of RX rings,
>   but it is tigtly coupled with RX flow hash configuration.
> o Patches for qlcnic and netxen_nic driver supporting rx channel will follow
>   soon.
> o This patch is reworked on Ben Hutchings "ethtool: NUMA affinity control" patch,
>   dropping the affinity control from it.
> 
> Ben Hutching:
> o define 'combined' and 'other' types.  Most multiqueue drivers pair up RX and TX
>   queues so that most channels combine RX and TX work.
> o Please could you use a kernel-doc comment to describe the structure.
> 
> Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
> Signed-off-by: Amit Kumar Salecha <amit.salecha@qlogic.com>

Ben, are you currently OK with this patch?

There was some back and forth, and I just want to make sure all of the
issues you raised were addressed to your satisfaction.

Thanks.

Re: [PATCHv3 NEXT 1/1] net: ethtool support to configure number of channels

[Ben Hutchings]

On Wed, 2011-04-06 at 12:56 -0700, David Miller wrote:
> From: Amit Kumar Salecha <amit.salecha@qlogic.com>
> Date: Fri,  1 Apr 2011 21:58:03 -0700
> 
> > o There exist ETHTOOL_GRXRINGS command for getting number of RX rings,
> >   but it is tigtly coupled with RX flow hash configuration.
> > o Patches for qlcnic and netxen_nic driver supporting rx channel will follow
> >   soon.
> > o This patch is reworked on Ben Hutchings "ethtool: NUMA affinity control" patch,
> >   dropping the affinity control from it.
> > 
> > Ben Hutching:
> > o define 'combined' and 'other' types.  Most multiqueue drivers pair up RX and TX
> >   queues so that most channels combine RX and TX work.
> > o Please could you use a kernel-doc comment to describe the structure.
> > 
> > Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
> > Signed-off-by: Amit Kumar Salecha <amit.salecha@qlogic.com>
> 
> Ben, are you currently OK with this patch?
> 
> There was some back and forth, and I just want to make sure all of the
> issues you raised were addressed to your satisfaction.

I'm afraid not.

On Fri, 2011-04-01 at 21:58 -0700, Amit Kumar Salecha wrote:
o There exist ETHTOOL_GRXRINGS command for getting number of RX rings,
>   but it is tigtly coupled with RX flow hash configuration.
> o Patches for qlcnic and netxen_nic driver supporting rx channel will follow
>   soon.
> o This patch is reworked on Ben Hutchings "ethtool: NUMA affinity control" patch,
>   dropping the affinity control from it.
> 
> Ben Hutching:
> o define 'combined' and 'other' types.  Most multiqueue drivers pair up RX and TX
>   queues so that most channels combine RX and TX work.
> o Please could you use a kernel-doc comment to describe the structure.
> 
> Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
> Signed-off-by: Amit Kumar Salecha <amit.salecha@qlogic.com>

Amit, I told you already that you must not use my Signed-off-by line
since you are changing the patch significantly.

> ---
>  include/linux/ethtool.h |   41 +++++++++++++++++++++++++++++++++++++++++
>  net/core/ethtool.c      |   41 +++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 82 insertions(+), 0 deletions(-)
> 
> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
> index c8fcbdd..3c4d968 100644
> --- a/include/linux/ethtool.h
> +++ b/include/linux/ethtool.h
> @@ -229,6 +229,42 @@ struct ethtool_ringparam {
>  	__u32	tx_pending;
>  };
>  
> +/**
> + * struct ethtool_channels - configuring number of network channel
> + * @cmd: ETHTOOL_{G,S}CHANNELS
> + * @type: Channel type defined in ethtool_channel_type
> + * @max_XX: Read only. Maximum number of channel the driver support
> + * @XX_count: Valid values are in the range 1 to the "max_XX" counterpart above.

The kernel-doc tools are going to complain about these field names with
'XX' in them.  You'll have to describe each field individually.

> + * This can be used to configure RX,TX and other channels.
> + */
> +
> +struct ethtool_channels {
> +	__u32	cmd;
> +	__u32	type;
> +	__u32	max_rx;
> +	__u32	max_tx;
> +	__u32	max_other;
> +	__u32	rx_count;
> +	__u32	tx_count;
> +	__u32	other_count;
> +};
> +
> +/* Channel type */
> +/* Channel type should be pass in type field of ethtool_channels.
> + * TYPE_ALL indicates set all channels to XX_count values.
> + * TYPE_RX and TYPE_TX is to get and set RX and TX channels correspondingly.
> + * TYPE_COMBINED is to set both RX and TX channels to rx_count and tx_count
> + * correspondingly.

That's not what I meant by 'combined'.  I meant a set of RX queues and
TX queues (usually 1 of each) with an IRQ and maybe an event queue
shared between them.  It should be possible for ethtool to distinguish
combined channels, so it doesn't just report 'Invalid argument' if the
user tries to set rx_count != tx_count.

I think this requires that there are max_combined and combined_count (or
similar) fields in struct ethtool_channels, so a driver that only
supports combined channels can report max_rx = 0, max_tx = 0,
max_combined = N.

> TYPE_OTHER is to configure other channel.
> + */
> +enum ethtool_channel_type {
> +	ETH_CHAN_TYPE_ALL	= 0,
> +	ETH_CHAN_TYPE_RX,
> +	ETH_CHAN_TYPE_TX,
> +	ETH_CHAN_TYPE_COMBINED,
> +	ETH_CHAN_TYPE_OTHER,
> +};
[...]

Really I'm not sure whether there's a need to be able to specify which
channel counts are being changed.  ethtool or whatever utility is used
can do ETHTOOL_GCHANNELS, modify channel counts, ETHTOOL_SCHANNELS and
all the counts the user didn't want to change will be unchanged.  If you
still think it is useful then use flags for the different channel types
so there is no arbitrary restriction on which counts can be changed at
the same time.

Ben.

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

Re: [PATCHv3 NEXT 1/1] net: ethtool support to configure number of channels

[David Miller]

From: Ben Hutchings <bhutchings@solarflare.com>
Date: Wed, 06 Apr 2011 21:20:47 +0100

> Amit, I told you already that you must not use my Signed-off-by line
> since you are changing the patch significantly.

Amit, this is a very serious infraction.

You must not ever add someone else's signed-off-by when you make
changes to a patch, unless you have their very clear and explicit
permission to do so.

RE: [PATCHv3 NEXT 1/1] net: ethtool support to configure number of channels

[Amit Salecha]

>
> From: Ben Hutchings <bhutchings@solarflare.com>
> Date: Wed, 06 Apr 2011 21:20:47 +0100
>
> > Amit, I told you already that you must not use my Signed-off-by line
> > since you are changing the patch significantly.
>
> Amit, this is a very serious infraction.
>
> You must not ever add someone else's signed-off-by when you make
> changes to a patch, unless you have their very clear and explicit
> permission to do so.

As this patch is based on Ben patch, so I thought its my duty to add his Signed-off.
Then I misinterpreted Ben comment and thought he want me to explain his contribution.
Sorry Ben.

-Amit

This message and any attached documents contain information from QLogic Corporation or its wholly-owned subsidiaries that may be confidential. If you are not the intended recipient, you may not read, copy, distribute, or use this information. If you have received this transmission in error, please notify the sender immediately by reply e-mail and then delete this message.

RE: [PATCHv3 NEXT 1/1] net: ethtool support to configure number of channels

[Amit Salecha]

> > +/* Channel type */
> > +/* Channel type should be pass in type field of ethtool_channels.
> > + * TYPE_ALL indicates set all channels to XX_count values.
> > + * TYPE_RX and TYPE_TX is to get and set RX and TX channels
> correspondingly.
> > + * TYPE_COMBINED is to set both RX and TX channels to rx_count and
> tx_count
> > + * correspondingly.
>
> That's not what I meant by 'combined'.  I meant a set of RX queues and
> TX queues (usually 1 of each) with an IRQ and maybe an event queue
> shared between them.  It should be possible for ethtool to distinguish
> combined channels, so it doesn't just report 'Invalid argument' if the
> user tries to set rx_count != tx_count.
>
> I think this requires that there are max_combined and combined_count
> (or
> similar) fields in struct ethtool_channels, so a driver that only
> supports combined channels can report max_rx = 0, max_tx = 0,
> max_combined = N.
>

I will add max_combined and combined_count field in ethtool_channels structure.

> > TYPE_OTHER is to configure other channel.
> > + */
> > +enum ethtool_channel_type {
> > +   ETH_CHAN_TYPE_ALL       = 0,
> > +   ETH_CHAN_TYPE_RX,
> > +   ETH_CHAN_TYPE_TX,
> > +   ETH_CHAN_TYPE_COMBINED,
> > +   ETH_CHAN_TYPE_OTHER,
> > +};
> [...]
>
> Really I'm not sure whether there's a need to be able to specify which
> channel counts are being changed.  ethtool or whatever utility is used
> can do ETHTOOL_GCHANNELS, modify channel counts, ETHTOOL_SCHANNELS and
> all the counts the user didn't want to change will be unchanged.  If
> you
> still think it is useful then use flags for the different channel types
> so there is no arbitrary restriction on which counts can be changed at
> the same time.
>

I will drop this enum, as we have defined all fields in ethtool_channels (rx, tx, other and combined).


This message and any attached documents contain information from QLogic Corporation or its wholly-owned subsidiaries that may be confidential. If you are not the intended recipient, you may not read, copy, distribute, or use this information. If you have received this transmission in error, please notify the sender immediately by reply e-mail and then delete this message.