diff mbox

net: ethtool support to configure number of channels

Message ID 1301652075-382-1-git-send-email-amit.salecha@qlogic.com
State Changes Requested, archived
Delegated to: David Miller
Headers show

Commit Message

amit salecha April 1, 2011, 10:01 a.m. UTC 
o Instead of cluttering struct ethtool_channels, defined channel type in
  struct ethtool_channels, making it scalable for addition future channels.
  Application needs to send different ioctl for each channel type.
o There exist ETHTOOL_GRXRINGS command for getting number of RX rings,
  but it is tigtly coupled with RX flow hash configuration.
o New channel id can be added in ethtool_channel_id to make it configurable.
o Patches for qlcnic and netxen_nic driver supporting rx channel will follow
  soon.

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

Comments

Ben Hutchings April 1, 2011, 5:10 p.m. UTC | #1 
On Fri, 2011-04-01 at 03:01 -0700, Amit Kumar Salecha wrote:
> o Instead of cluttering struct ethtool_channels, defined channel type in
>   struct ethtool_channels, making it scalable for addition future channels.
>   Application needs to send different ioctl for each channel type.
> o There exist ETHTOOL_GRXRINGS command for getting number of RX rings,
>   but it is tigtly coupled with RX flow hash configuration.
> o New channel id can be added in ethtool_channel_id to make it configurable.
> o Patches for qlcnic and netxen_nic driver supporting rx channel will follow
>   soon.
> 
> Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>

Since you have modified this, you must not use my Signed-off-by without
explaining that you have done so.

> Signed-off-by: Amit Kumar Salecha <amit.salecha@qlogic.com>
> ---
>  include/linux/ethtool.h |   27 +++++++++++++++++++++++++++
>  net/core/ethtool.c      |   41 +++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 68 insertions(+), 0 deletions(-)
> 
> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
> index c8fcbdd..cdb69d6 100644
> --- a/include/linux/ethtool.h
> +++ b/include/linux/ethtool.h
> @@ -229,6 +229,28 @@ struct ethtool_ringparam {
>  	__u32	tx_pending;
>  };
>  
> +/* for configuring number of network channel */
> +struct ethtool_channels {
> +	__u32	cmd;	/* ETHTOOL_{G,S}CHANNELS */
> +	__u32	type;	/* Channel type defined in ethtool_channel_id */
> +
> +	/* Read only attributes.  These indicate the maximum number
> +	 * of channel the driver will allow the user to set.
> +	 */
> +	__u32	max;

This is a single read-only attribute, not 'attributes'.

> +	/* Values changeable by the user.  The valid values are
> +	 * in the range 1 to the "*_max" counterpart above.
> +	 */
> +	__u32	pending;
> +};

Please could you use a kernel-doc comment to describe the structure.  I
know my earlier patch (and most of the existing structures in ethtool.h)
didn't, but I have been gradually changing that.

I'm not sure why you reduced this to a single count.  If if the driver
or hardware doesn't allow certain combinations of counts, it might be
necessary to configure several types at the same time 

> +/* Channel ID is made up of a type */
> +enum ethtool_channel_id {
> +	ETH_CHAN_TYPE_RX = 0x1,
> +	ETH_CHAN_TYPE_TX = 0x2
> +};
[...]

enum ethtool_channel_id was meant to be an identifier of a specific
channel.  An enumeration of channel types should be named differently.

This also omits the 'combined' and 'other' types.  Most multiqueue
drivers pair up RX and TX queues so that most channels combine RX and TX
work.

Ben.
Ben Hutchings April 2, 2011, 2:55 a.m. UTC | #2 
On Fri, 2011-04-01 at 21:36 -0500, Amit Salecha wrote:
> > I'm not sure why you reduced this to a single count.  If if the driver
> > or hardware doesn't allow certain combinations of counts, it might be
> > necessary to configure several types at the same time
> >
> > > +/* Channel ID is made up of a type */
> > > +enum ethtool_channel_id {
> > > +   ETH_CHAN_TYPE_RX = 0x1,
> > > +   ETH_CHAN_TYPE_TX = 0x2
> > > +};
> > [...]
> >
> > enum ethtool_channel_id was meant to be an identifier of a specific
> > channel.  An enumeration of channel types should be named differently.
> >
> 
> I will name it as ethtool_channel_type. Any other suggestion ?
> 
> > This also omits the 'combined' and 'other' types.  Most multiqueue
> > drivers pair up RX and TX queues so that most channels combine RX and
> > TX
> > work.
> 
> 'combined' is ok, what is use of 'other' ?

Could be link interrupts, SR-IOV coordination, or something else.  Not
something you'd likely be able to change, but it could be useful to know
that some interrupts are allocated to them.  Actually, that does mean it
might be helpful for the 'get' operation to return a minimum value along
with the maximum value.

Ben.
amit salecha April 2, 2011, 3:13 a.m. UTC | #3 
> >

> > > This also omits the 'combined' and 'other' types.  Most multiqueue

> > > drivers pair up RX and TX queues so that most channels combine RX

> and

> > > TX

> > > work.

> >

> > 'combined' is ok, what is use of 'other' ?

>

> Could be link interrupts, SR-IOV coordination, or something else.  Not

> something you'd likely be able to change, but it could be useful to

> know

> that some interrupts are allocated to them.  Actually, that does mean

> it

> might be helpful for the 'get' operation to return a minimum value

> along

> with the maximum value.

>

Instead of naming as 'other', corresponding channel type should be declared.
Adapter may have multiple 'other' channel. It's better to name them.
Naming them will be useful, if one want to configure them.
So I will rather leave this open and let people to declared channel type as they required.


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.
amit salecha April 2, 2011, 3:22 a.m. UTC | #4 
> > >

> > > > This also omits the 'combined' and 'other' types.  Most

> multiqueue

> > > > drivers pair up RX and TX queues so that most channels combine RX

> > and

> > > > TX

> > > > work.

> > >

> > > 'combined' is ok, what is use of 'other' ?

> >

> > Could be link interrupts, SR-IOV coordination, or something else.

> Not

> > something you'd likely be able to change, but it could be useful to

> > know

> > that some interrupts are allocated to them.  Actually, that does mean

> > it

> > might be helpful for the 'get' operation to return a minimum value

> > along

> > with the maximum value.

> >

> Instead of naming as 'other', corresponding channel type should be

> declared.

> Adapter may have multiple 'other' channel. It's better to name them.

> Naming them will be useful, if one want to configure them.

> So I will rather leave this open and let people to declared channel

> type as they required.

>


I will declare 'other' channel type, which will mean channel other than
declared in ethtool_channel_type (not other than RX/TX).

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.
Anirban Chakraborty April 2, 2011, 5:47 a.m. UTC | #5 
On Apr 1, 2011, at 7:55 PM, Ben Hutchings wrote:

> On Fri, 2011-04-01 at 21:36 -0500, Amit Salecha wrote:
>>> I'm not sure why you reduced this to a single count.  If if the driver
>>> or hardware doesn't allow certain combinations of counts, it might be
>>> necessary to configure several types at the same time
>>>
>>>> +/* Channel ID is made up of a type */
>>>> +enum ethtool_channel_id {
>>>> +   ETH_CHAN_TYPE_RX = 0x1,
>>>> +   ETH_CHAN_TYPE_TX = 0x2
>>>> +};
>>> [...]
>>>
>>> enum ethtool_channel_id was meant to be an identifier of a specific
>>> channel.  An enumeration of channel types should be named differently.
>>>
>>
>> I will name it as ethtool_channel_type. Any other suggestion ?
>>
>>> This also omits the 'combined' and 'other' types.  Most multiqueue
>>> drivers pair up RX and TX queues so that most channels combine RX and
>>> TX
>>> work.
>>
>> 'combined' is ok, what is use of 'other' ?
>
> Could be link interrupts, SR-IOV coordination, or something else.  Not
> something you'd likely be able to change, but it could be useful to know
> that some interrupts are allocated to them.  Actually, that does mean it
> might be helpful for the 'get' operation to return a minimum value along
> with the maximum value.

Are you thinking of using the 'other' field as a way to a represent a 'virtual port'
that a VF could have. A virtual port could have a set of rx/tx rings, interrupts,
QoS parameters, MAC filters, VLAN ids etc. etc. A VF could have one or many such
channels. If thats the case, I would think that configuring these channels should
be done via a PF rather than on a VF. It is possible I could get you totally wrong here,
however it would be good to hear your thoughts.

-Anirban

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.

--
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
Ben Hutchings April 6, 2011, 10:18 p.m. UTC | #6 
On Fri, 2011-04-01 at 22:47 -0700, Anirban Chakraborty wrote:
> On Apr 1, 2011, at 7:55 PM, Ben Hutchings wrote:
> 
> > On Fri, 2011-04-01 at 21:36 -0500, Amit Salecha wrote:
> >>> I'm not sure why you reduced this to a single count.  If if the driver
> >>> or hardware doesn't allow certain combinations of counts, it might be
> >>> necessary to configure several types at the same time
> >>>
> >>>> +/* Channel ID is made up of a type */
> >>>> +enum ethtool_channel_id {
> >>>> +   ETH_CHAN_TYPE_RX = 0x1,
> >>>> +   ETH_CHAN_TYPE_TX = 0x2
> >>>> +};
> >>> [...]
> >>>
> >>> enum ethtool_channel_id was meant to be an identifier of a specific
> >>> channel.  An enumeration of channel types should be named differently.
> >>>
> >>
> >> I will name it as ethtool_channel_type. Any other suggestion ?
> >>
> >>> This also omits the 'combined' and 'other' types.  Most multiqueue
> >>> drivers pair up RX and TX queues so that most channels combine RX and
> >>> TX
> >>> work.
> >>
> >> 'combined' is ok, what is use of 'other' ?
> >
> > Could be link interrupts, SR-IOV coordination, or something else.  Not
> > something you'd likely be able to change, but it could be useful to know
> > that some interrupts are allocated to them.  Actually, that does mean it
> > might be helpful for the 'get' operation to return a minimum value along
> > with the maximum value.
> 
> Are you thinking of using the 'other' field as a way to a represent a 'virtual port'
> that a VF could have. A virtual port could have a set of rx/tx rings, interrupts,
> QoS parameters, MAC filters, VLAN ids etc. etc. A VF could have one or many such
> channels. If thats the case, I would think that configuring these channels should
> be done via a PF rather than on a VF. It is possible I could get you totally wrong here,
> however it would be good to hear your thoughts.

The net device for a VF could have all sorts of channels, and their
numbers may or may not be configurable depending on limitations of the
hardware, firmware, driver or hypervisor.

The channel counts reported by a net device should include all those
IRQs allocated by the net device driver for its parent device (e.g. a
PCI device).

To be more concrete, here is how I would count channels:

1. RX queue with IRQ, exposed to the network stack => RX channel
2. TX queue with IRQ, exposed to the network stack => TX channel
3. RX and TX queue sharing IRQ, exposed to the network stack => combined channel
4. Link change IRQ => other channel
5. Queue(s) and IRQ for iSCSI traffic, not exposed to the network stack => other channel
6. Qeuue(s) and IRQ for coordination between PCI functions => other channel
7. Queue(s) and IRQ allocated to other PCI function => not counted

Ben.
diff mbox

Patch

diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index c8fcbdd..cdb69d6 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -229,6 +229,28 @@  struct ethtool_ringparam {
 	__u32	tx_pending;
 };
 
+/* for configuring number of network channel */
+struct ethtool_channels {
+	__u32	cmd;	/* ETHTOOL_{G,S}CHANNELS */
+	__u32	type;	/* Channel type defined in ethtool_channel_id */
+
+	/* Read only attributes.  These indicate the maximum number
+	 * of channel the driver will allow the user to set.
+	 */
+	__u32	max;
+
+	/* Values changeable by the user.  The valid values are
+	 * in the range 1 to the "*_max" counterpart above.
+	 */
+	__u32	pending;
+};
+
+/* Channel ID is made up of a type */
+enum ethtool_channel_id {
+	ETH_CHAN_TYPE_RX = 0x1,
+	ETH_CHAN_TYPE_TX = 0x2
+};
+
 /* for configuring link flow control parameters */
 struct ethtool_pauseparam {
 	__u32	cmd;	/* ETHTOOL_{G,S}PAUSEPARAM */
@@ -802,6 +824,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 +895,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;
 	}