| Message ID | 1301720283-25038-1-git-send-email-amit.salecha@qlogic.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
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. -- 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
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.
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. -- 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
> > 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. -- 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
> > +/* 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.
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; }