[RFC 1/5] net/ethtool: introduce a new ioctl for per queue setting

[RFC 1/5] net/ethtool: introduce a new ioctl for per queue setting

[kan.liang]

From: Kan Liang <kan.liang@intel.com>

Introduce a new ioctl ETHTOOL_PERQUEUE for per queue parameters setting.
The following patches will enable some SUB_COMMANDs for per queue
setting.

Signed-off-by: Kan Liang <kan.liang@intel.com>
---
 include/uapi/linux/ethtool.h | 18 ++++++++++++++++++
 net/core/ethtool.c           | 17 +++++++++++++++++
 2 files changed, 35 insertions(+)

diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
index cd16291..05bc92a 100644
--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h
@@ -1144,6 +1144,22 @@ enum ethtool_sfeatures_retval_bits {
 #define ETHTOOL_F_WISH          (1 << ETHTOOL_F_WISH__BIT)
 #define ETHTOOL_F_COMPAT        (1 << ETHTOOL_F_COMPAT__BIT)
 
+#define MAX_QUEUE		4096
+#define MAX_QUEUE_MASK		(MAX_QUEUE / 64)
+
+/**
+ * struct ethtool_per_queue_op - apply sub command to the queues in mask.
+ * @cmd: ETHTOOL_PERQUEUE
+ * @queue_mask: Mask the queues which sub command apply to
+ * @sub_command: the sub command
+ * @data: parameters of the command
+ */
+struct ethtool_per_queue_op {
+	__u32	cmd;
+	__u64	queue_mask[MAX_QUEUE_MASK];
+	__u32	sub_command;
+	char	data[];
+};
 
 /* CMDs currently supported */
 #define ETHTOOL_GSET		0x00000001 /* Get settings. */
@@ -1226,6 +1242,8 @@ enum ethtool_sfeatures_retval_bits {
 #define ETHTOOL_GTUNABLE	0x00000048 /* Get tunable configuration */
 #define ETHTOOL_STUNABLE	0x00000049 /* Set tunable configuration */
 
+#define ETHTOOL_PERQUEUE	0x0000004a /* Set per queue options */
+
 /* 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 29edf74..125fb32 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -1748,6 +1748,20 @@ out:
 	return ret;
 }
 
+static int ethtool_set_per_queue(struct net_device *dev, void __user *useraddr)
+{
+	struct ethtool_per_queue_op per_queue_opt;
+
+	if (copy_from_user(&per_queue_opt, useraddr, sizeof(per_queue_opt)))
+		return -EFAULT;
+
+	switch (per_queue_opt.sub_command) {
+
+	default:
+		return -EOPNOTSUPP;
+	};
+}
+
 /* The main entry point in this file.  Called from net/core/dev_ioctl.c */
 
 int dev_ethtool(struct net *net, struct ifreq *ifr)
@@ -1991,6 +2005,9 @@ int dev_ethtool(struct net *net, struct ifreq *ifr)
 	case ETHTOOL_STUNABLE:
 		rc = ethtool_set_tunable(dev, useraddr);
 		break;
+	case ETHTOOL_PERQUEUE:
+		rc = ethtool_set_per_queue(dev, useraddr);
+		break;
 	default:
 		rc = -EOPNOTSUPP;
 	}
-- 
1.7.11.7

[RFC 2/5] net/ethtool: support get coalesce per queue

[kan.liang]

From: Kan Liang <kan.liang@intel.com>

Device driver has to provide an interface to get per queue coalesce.
The interrupt coalescing parameters of each masked queue will be
copied back to user space one by one.

Signed-off-by: Kan Liang <kan.liang@intel.com>
---
 include/linux/ethtool.h |  5 ++++-
 net/core/ethtool.c      | 33 ++++++++++++++++++++++++++++++++-
 2 files changed, 36 insertions(+), 2 deletions(-)

diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 653dc9c..107f75f 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -201,6 +201,8 @@ static inline u32 ethtool_rxfh_indir_default(u32 index, u32 n_rx_rings)
  * @get_module_eeprom: Get the eeprom information from the plug-in module
  * @get_eee: Get Energy-Efficient (EEE) supported and status.
  * @set_eee: Set EEE status (enable/disable) as well as LPI timers.
+ * @get_per_queue_coalesce: Get interrupt coalescing parameters per queue.
+ * 	Returns a negative error code or zero.
  *
  * All operations are optional (i.e. the function pointer may be set
  * to %NULL) and callers must take this into account.  Callers must
@@ -279,7 +281,8 @@ struct ethtool_ops {
 			       const struct ethtool_tunable *, void *);
 	int	(*set_tunable)(struct net_device *,
 			       const struct ethtool_tunable *, const void *);
-
+	int	(*get_per_queue_coalesce)(struct net_device *, int,
+					  struct ethtool_coalesce *);
 
 };
 #endif /* _LINUX_ETHTOOL_H */
diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index 125fb32..22ff69a 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -1748,6 +1748,36 @@ out:
 	return ret;
 }
 
+static int ethtool_get_per_queue_coalesce(struct net_device *dev,
+					  void __user *useraddr,
+					  struct ethtool_per_queue_op *per_queue_opt)
+{
+	u64 queue_mask;
+	int bit, i, ret;
+
+	if (!dev->ethtool_ops->get_per_queue_coalesce)
+		return -EOPNOTSUPP;
+
+	useraddr += sizeof(*per_queue_opt);
+	for (i = 0; i < MAX_QUEUE_MASK; i++) {
+		queue_mask = per_queue_opt->queue_mask[i];
+		if (queue_mask > 0) {
+			for_each_set_bit(bit, (unsigned long *)&queue_mask, 64) {
+				struct ethtool_coalesce coalesce = { .cmd = ETHTOOL_GCOALESCE };
+
+				ret = dev->ethtool_ops->get_per_queue_coalesce(dev, bit + i * 64, &coalesce);
+				if (ret != 0)
+					return ret;
+				if (copy_to_user(useraddr, &coalesce, sizeof(coalesce)))
+					return -EFAULT;
+				useraddr += sizeof(coalesce);
+			}
+		}
+	}
+
+	return 0;
+}
+
 static int ethtool_set_per_queue(struct net_device *dev, void __user *useraddr)
 {
 	struct ethtool_per_queue_op per_queue_opt;
@@ -1756,7 +1786,8 @@ static int ethtool_set_per_queue(struct net_device *dev, void __user *useraddr)
 		return -EFAULT;
 
 	switch (per_queue_opt.sub_command) {
-
+	case ETHTOOL_GCOALESCE:
+		return ethtool_get_per_queue_coalesce(dev, useraddr, &per_queue_opt);
 	default:
 		return -EOPNOTSUPP;
 	};
-- 
1.7.11.7

[RFC 3/5] net/ethtool: support set coalesce per queue

[kan.liang]

From: Kan Liang <kan.liang@intel.com>

Device driver has to provide an interface to set per queue coalesce. The
wanted coalesce information are stored in "data" for each masked queue,
which can copy from userspace.

Signed-off-by: Kan Liang <kan.liang@intel.com>
---
 include/linux/ethtool.h |  4 ++++
 net/core/ethtool.c      | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 37 insertions(+)

diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 107f75f..b3bbbcb 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -203,6 +203,8 @@ static inline u32 ethtool_rxfh_indir_default(u32 index, u32 n_rx_rings)
  * @set_eee: Set EEE status (enable/disable) as well as LPI timers.
  * @get_per_queue_coalesce: Get interrupt coalescing parameters per queue.
  * 	Returns a negative error code or zero.
+ * @set_per_queue_coalesce: Set interrupt coalescing parameters per queue.
+ * 	Returns a negative error code or zero.
  *
  * All operations are optional (i.e. the function pointer may be set
  * to %NULL) and callers must take this into account.  Callers must
@@ -283,6 +285,8 @@ struct ethtool_ops {
 			       const struct ethtool_tunable *, const void *);
 	int	(*get_per_queue_coalesce)(struct net_device *, int,
 					  struct ethtool_coalesce *);
+	int	(*set_per_queue_coalesce)(struct net_device *, int,
+					  struct ethtool_coalesce *);
 
 };
 #endif /* _LINUX_ETHTOOL_H */
diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index 22ff69a..c8c5726 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -1778,6 +1778,37 @@ static int ethtool_get_per_queue_coalesce(struct net_device *dev,
 	return 0;
 }
 
+static int ethtool_set_per_queue_coalesce(struct net_device *dev,
+					  void __user *useraddr,
+					  struct ethtool_per_queue_op *per_queue_opt)
+{
+	u64 queue_mask;
+	int bit, i, ret;
+
+	if (!dev->ethtool_ops->set_per_queue_coalesce)
+		return -EOPNOTSUPP;
+
+	useraddr += sizeof(*per_queue_opt);
+	for (i = 0; i < MAX_QUEUE_MASK; i++) {
+		queue_mask = per_queue_opt->queue_mask[i];
+		if (queue_mask > 0) {
+			for_each_set_bit(bit, (unsigned long *)&queue_mask, 64) {
+				struct ethtool_coalesce coalesce;
+
+				if (copy_from_user(&coalesce, useraddr, sizeof(coalesce)))
+					return -EFAULT;
+
+				ret = dev->ethtool_ops->set_per_queue_coalesce(dev, bit + i * 64, &coalesce);
+				if (ret != 0)
+					return ret;
+				useraddr += sizeof(coalesce);
+			}
+		}
+	}
+
+	return 0;
+}
+
 static int ethtool_set_per_queue(struct net_device *dev, void __user *useraddr)
 {
 	struct ethtool_per_queue_op per_queue_opt;
@@ -1788,6 +1819,8 @@ static int ethtool_set_per_queue(struct net_device *dev, void __user *useraddr)
 	switch (per_queue_opt.sub_command) {
 	case ETHTOOL_GCOALESCE:
 		return ethtool_get_per_queue_coalesce(dev, useraddr, &per_queue_opt);
+	case ETHTOOL_SCOALESCE:
+		return ethtool_set_per_queue_coalesce(dev, useraddr, &per_queue_opt);
 	default:
 		return -EOPNOTSUPP;
 	};
-- 
1.7.11.7

[RFC 4/5] i40e/ethtool: support coalesce getting by queue

[kan.liang]

From: Kan Liang <kan.liang@intel.com>

This patch implements get_per_queue_coalesce for i40e driver.
For i40e driver, only rx and tx usecs has per queue value. So only these
two parameters are read from specific registers. For other interrupt
coalescing parameters, they are shared among queues. The values which
are stored in vsi will be return.

Signed-off-by: Kan Liang <kan.liang@intel.com>
---
 drivers/net/ethernet/intel/i40e/i40e_ethtool.c | 34 ++++++++++++++++++++++++--
 1 file changed, 32 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
index 3f385ff..b41f0be 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
@@ -1843,11 +1843,16 @@ static int i40e_set_phys_id(struct net_device *netdev,
  * 125us (8000 interrupts per second) == ITR(62)
  */
 
-static int i40e_get_coalesce(struct net_device *netdev,
-			     struct ethtool_coalesce *ec)
+static int __i40e_get_coalesce(struct net_device *netdev,
+			       struct ethtool_coalesce *ec,
+			       int queue)
 {
 	struct i40e_netdev_priv *np = netdev_priv(netdev);
 	struct i40e_vsi *vsi = np->vsi;
+	struct i40e_pf *pf = vsi->back;
+	struct i40e_hw *hw = &pf->hw;
+	struct i40e_q_vector *q_vector;
+	u16 vector;
 
 	ec->tx_max_coalesced_frames_irq = vsi->work_limit;
 	ec->rx_max_coalesced_frames_irq = vsi->work_limit;
@@ -1869,9 +1874,33 @@ static int i40e_get_coalesce(struct net_device *netdev,
 	ec->rx_coalesce_usecs_high = vsi->int_rate_limit;
 	ec->tx_coalesce_usecs_high = vsi->int_rate_limit;
 
+	if (queue > 0) {
+		if (queue >= vsi->num_q_vectors) {
+			netif_info(pf, drv, netdev, "Invalid queue number\n");
+			return -EINVAL;
+		}
+
+		q_vector = vsi->q_vectors[queue];
+		vector = vsi->base_vector + queue;
+
+		ec->rx_coalesce_usecs = ITR_REG_TO_USEC(rd32(hw, I40E_PFINT_ITRN(0, vector - 1)));
+		ec->tx_coalesce_usecs = ITR_REG_TO_USEC(rd32(hw, I40E_PFINT_ITRN(1, vector - 1)));
+	}
 	return 0;
 }
 
+static int i40e_get_coalesce(struct net_device *netdev,
+			     struct ethtool_coalesce *ec)
+{
+	return __i40e_get_coalesce(netdev, ec, -1);
+}
+
+static int i40e_get_per_queue_coalesce(struct net_device *netdev, int queue,
+				       struct ethtool_coalesce *ec)
+{
+	return __i40e_get_coalesce(netdev, ec, queue);
+}
+
 static int i40e_set_coalesce(struct net_device *netdev,
 			     struct ethtool_coalesce *ec)
 {
@@ -2788,6 +2817,7 @@ static const struct ethtool_ops i40e_ethtool_ops = {
 	.get_ts_info		= i40e_get_ts_info,
 	.get_priv_flags		= i40e_get_priv_flags,
 	.set_priv_flags		= i40e_set_priv_flags,
+	.get_per_queue_coalesce	= i40e_get_per_queue_coalesce,
 };
 
 void i40e_set_ethtool_ops(struct net_device *netdev)
-- 
1.7.11.7

[RFC 5/5] i40e/ethtool: support coalesce setting by queue

[kan.liang]

From: Kan Liang <kan.liang@intel.com>

This patch implements set_per_queue_coalesce for i40e driver.
For i40e driver, only rx and tx usecs has per queue value. Changing
these two parameters only impact the specific queue. For other interrupt
coalescing parameters, they are shared among queues. The change to one
queue will impact all queues.

Signed-off-by: Kan Liang <kan.liang@intel.com>
---
 drivers/net/ethernet/intel/i40e/i40e_ethtool.c | 55 +++++++++++++++++++-------
 1 file changed, 41 insertions(+), 14 deletions(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
index b41f0be..5a35fdb 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
@@ -1901,14 +1901,29 @@ static int i40e_get_per_queue_coalesce(struct net_device *netdev, int queue,
 	return __i40e_get_coalesce(netdev, ec, queue);
 }
 
-static int i40e_set_coalesce(struct net_device *netdev,
-			     struct ethtool_coalesce *ec)
+static void i40e_set_itr_for_queue(struct i40e_vsi *vsi, int queue, u16 vector)
 {
-	struct i40e_netdev_priv *np = netdev_priv(netdev);
 	struct i40e_q_vector *q_vector;
-	struct i40e_vsi *vsi = np->vsi;
 	struct i40e_pf *pf = vsi->back;
 	struct i40e_hw *hw = &pf->hw;
+	u16 intrl = INTRL_USEC_TO_REG(vsi->int_rate_limit);
+
+	q_vector = vsi->q_vectors[queue];
+	q_vector->rx.itr = ITR_TO_REG(vsi->rx_itr_setting);
+	wr32(hw, I40E_PFINT_ITRN(0, vector - 1), q_vector->rx.itr);
+	q_vector->tx.itr = ITR_TO_REG(vsi->tx_itr_setting);
+	wr32(hw, I40E_PFINT_ITRN(1, vector - 1), q_vector->tx.itr);
+	wr32(hw, I40E_PFINT_RATEN(vector - 1), intrl);
+	i40e_flush(hw);
+}
+
+static int __i40e_set_coalesce(struct net_device *netdev,
+			       struct ethtool_coalesce *ec,
+			       int queue)
+{
+	struct i40e_netdev_priv *np = netdev_priv(netdev);
+	struct i40e_vsi *vsi = np->vsi;
+	struct i40e_pf *pf = vsi->back;
 	u16 vector;
 	int i;
 
@@ -1964,21 +1979,32 @@ static int i40e_set_coalesce(struct net_device *netdev,
 	else
 		vsi->tx_itr_setting &= ~I40E_ITR_DYNAMIC;
 
-	for (i = 0; i < vsi->num_q_vectors; i++, vector++) {
-		u16 intrl = INTRL_USEC_TO_REG(vsi->int_rate_limit);
-
-		q_vector = vsi->q_vectors[i];
-		q_vector->rx.itr = ITR_TO_REG(vsi->rx_itr_setting);
-		wr32(hw, I40E_PFINT_ITRN(0, vector - 1), q_vector->rx.itr);
-		q_vector->tx.itr = ITR_TO_REG(vsi->tx_itr_setting);
-		wr32(hw, I40E_PFINT_ITRN(1, vector - 1), q_vector->tx.itr);
-		wr32(hw, I40E_PFINT_RATEN(vector - 1), intrl);
-		i40e_flush(hw);
+	if (queue < 0) {
+		for (i = 0; i < vsi->num_q_vectors; i++, vector++)
+			i40e_set_itr_for_queue(vsi, i, vector);
+	} else {
+		if (queue >= vsi->num_q_vectors) {
+			netif_info(pf, drv, netdev, "Invalid queue value, queue range is 0 - %d\n", vsi->num_q_vectors - 1);
+			return -EINVAL;
+		}
+		i40e_set_itr_for_queue(vsi, queue, vector + queue);
 	}
 
 	return 0;
 }
 
+static int i40e_set_coalesce(struct net_device *netdev,
+			     struct ethtool_coalesce *ec)
+{
+	return __i40e_set_coalesce(netdev, ec, -1);
+}
+
+static int i40e_set_per_queue_coalesce(struct net_device *netdev, int queue,
+				       struct ethtool_coalesce *ec)
+{
+	return __i40e_set_coalesce(netdev, ec, queue);
+}
+
 /**
  * i40e_get_rss_hash_opts - Get RSS hash Input Set for each flow type
  * @pf: pointer to the physical function struct
@@ -2818,6 +2844,7 @@ static const struct ethtool_ops i40e_ethtool_ops = {
 	.get_priv_flags		= i40e_get_priv_flags,
 	.set_priv_flags		= i40e_set_priv_flags,
 	.get_per_queue_coalesce	= i40e_get_per_queue_coalesce,
+	.set_per_queue_coalesce	= i40e_set_per_queue_coalesce,
 };
 
 void i40e_set_ethtool_ops(struct net_device *netdev)
-- 
1.7.11.7

RE: [RFC 4/5] i40e/ethtool: support coalesce getting by queue

[Nelson, Shannon]

> From: Kan Liang <kan.liang@intel.com>
> 
> This patch implements get_per_queue_coalesce for i40e driver.
> For i40e driver, only rx and tx usecs has per queue value. So only these
> two parameters are read from specific registers. For other interrupt
> coalescing parameters, they are shared among queues. The values which
> are stored in vsi will be return.
> 
> Signed-off-by: Kan Liang <kan.liang@intel.com>
> ---
>  drivers/net/ethernet/intel/i40e/i40e_ethtool.c | 34
> ++++++++++++++++++++++++--
>  1 file changed, 32 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
> b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
> index 3f385ff..b41f0be 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
> @@ -1843,11 +1843,16 @@ static int i40e_set_phys_id(struct net_device
> *netdev,
>   * 125us (8000 interrupts per second) == ITR(62)
>   */
> 
> -static int i40e_get_coalesce(struct net_device *netdev,
> -			     struct ethtool_coalesce *ec)
> +static int __i40e_get_coalesce(struct net_device *netdev,
> +			       struct ethtool_coalesce *ec,
> +			       int queue)
>  {
>  	struct i40e_netdev_priv *np = netdev_priv(netdev);
>  	struct i40e_vsi *vsi = np->vsi;
> +	struct i40e_pf *pf = vsi->back;
> +	struct i40e_hw *hw = &pf->hw;
> +	struct i40e_q_vector *q_vector;
> +	u16 vector;
> 
>  	ec->tx_max_coalesced_frames_irq = vsi->work_limit;
>  	ec->rx_max_coalesced_frames_irq = vsi->work_limit;
> @@ -1869,9 +1874,33 @@ static int i40e_get_coalesce(struct net_device
> *netdev,
>  	ec->rx_coalesce_usecs_high = vsi->int_rate_limit;
>  	ec->tx_coalesce_usecs_high = vsi->int_rate_limit;
> 
> +	if (queue > 0) {
> +		if (queue >= vsi->num_q_vectors) {
> +			netif_info(pf, drv, netdev, "Invalid queue number\n");
> +			return -EINVAL;
> +		}
> +
> +		q_vector = vsi->q_vectors[queue];

The problem here is that there can be more queues than vectors and they're not spread round-robin style.  For example, if the VSI was only given 4 vectors, but has 8 queues, there will be 2 queues per vector paired up such that V0 <= {Q0, Q1}, V1 <= {Q2, Q3} ...

You'll need to check the requested queue number against vsi->num_queue_pairs for the max queue check, then look at vsi->tx_rings[queue].q_vector and/or vsi->rx_rings[queue].q_vector to find the actual q_vector for the requested queue.

sln

> +		vector = vsi->base_vector + queue;
> +
> +		ec->rx_coalesce_usecs = ITR_REG_TO_USEC(rd32(hw,
> I40E_PFINT_ITRN(0, vector - 1)));
> +		ec->tx_coalesce_usecs = ITR_REG_TO_USEC(rd32(hw,
> I40E_PFINT_ITRN(1, vector - 1)));
> +	}
>  	return 0;
>  }
> 
> +static int i40e_get_coalesce(struct net_device *netdev,
> +			     struct ethtool_coalesce *ec)
> +{
> +	return __i40e_get_coalesce(netdev, ec, -1);
> +}
> +
> +static int i40e_get_per_queue_coalesce(struct net_device *netdev, int
> queue,
> +				       struct ethtool_coalesce *ec)
> +{
> +	return __i40e_get_coalesce(netdev, ec, queue);
> +}
> +
>  static int i40e_set_coalesce(struct net_device *netdev,
>  			     struct ethtool_coalesce *ec)
>  {
> @@ -2788,6 +2817,7 @@ static const struct ethtool_ops i40e_ethtool_ops = {
>  	.get_ts_info		= i40e_get_ts_info,
>  	.get_priv_flags		= i40e_get_priv_flags,
>  	.set_priv_flags		= i40e_set_priv_flags,
> +	.get_per_queue_coalesce	= i40e_get_per_queue_coalesce,
>  };
> 
>  void i40e_set_ethtool_ops(struct net_device *netdev)
> --
> 1.7.11.7

RE: [RFC 5/5] i40e/ethtool: support coalesce setting by queue

[Nelson, Shannon]

> From: Kan Liang <kan.liang@intel.com>
> 
> This patch implements set_per_queue_coalesce for i40e driver.
> For i40e driver, only rx and tx usecs has per queue value. Changing
> these two parameters only impact the specific queue. For other interrupt
> coalescing parameters, they are shared among queues. The change to one
> queue will impact all queues.
> 
> Signed-off-by: Kan Liang <kan.liang@intel.com>
> ---
>  drivers/net/ethernet/intel/i40e/i40e_ethtool.c | 55 +++++++++++++++++++--
> -----
>  1 file changed, 41 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
> b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
> index b41f0be..5a35fdb 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
> @@ -1901,14 +1901,29 @@ static int i40e_get_per_queue_coalesce(struct
> net_device *netdev, int queue,
>  	return __i40e_get_coalesce(netdev, ec, queue);
>  }
> 
> -static int i40e_set_coalesce(struct net_device *netdev,
> -			     struct ethtool_coalesce *ec)
> +static void i40e_set_itr_for_queue(struct i40e_vsi *vsi, int queue, u16
> vector)
>  {
> -	struct i40e_netdev_priv *np = netdev_priv(netdev);
>  	struct i40e_q_vector *q_vector;
> -	struct i40e_vsi *vsi = np->vsi;
>  	struct i40e_pf *pf = vsi->back;
>  	struct i40e_hw *hw = &pf->hw;
> +	u16 intrl = INTRL_USEC_TO_REG(vsi->int_rate_limit);
> +
> +	q_vector = vsi->q_vectors[queue];

Again, the problem here is that there can be more queues than vectors, so you'll need to check the requested queue number against vsi->num_queue_pairs for the max queue check, then look at vsi->tx_rings[queue].q_vector and/or vsi->rx_rings[queue].q_vector to find the actual q_vector for the requested queue.

Also, this means that it would be possible for the user to try to assign different values to queues on the same vector.  How do you want to handle that?  My first inclination is to not do anything and just let the last value assigned win.

sln

> +	q_vector->rx.itr = ITR_TO_REG(vsi->rx_itr_setting);
> +	wr32(hw, I40E_PFINT_ITRN(0, vector - 1), q_vector->rx.itr);
> +	q_vector->tx.itr = ITR_TO_REG(vsi->tx_itr_setting);
> +	wr32(hw, I40E_PFINT_ITRN(1, vector - 1), q_vector->tx.itr);
> +	wr32(hw, I40E_PFINT_RATEN(vector - 1), intrl);
> +	i40e_flush(hw);
> +}
> +
> +static int __i40e_set_coalesce(struct net_device *netdev,
> +			       struct ethtool_coalesce *ec,
> +			       int queue)
> +{
> +	struct i40e_netdev_priv *np = netdev_priv(netdev);
> +	struct i40e_vsi *vsi = np->vsi;
> +	struct i40e_pf *pf = vsi->back;
>  	u16 vector;
>  	int i;
> 
> @@ -1964,21 +1979,32 @@ static int i40e_set_coalesce(struct net_device
> *netdev,
>  	else
>  		vsi->tx_itr_setting &= ~I40E_ITR_DYNAMIC;
> 
> -	for (i = 0; i < vsi->num_q_vectors; i++, vector++) {
> -		u16 intrl = INTRL_USEC_TO_REG(vsi->int_rate_limit);
> -
> -		q_vector = vsi->q_vectors[i];
> -		q_vector->rx.itr = ITR_TO_REG(vsi->rx_itr_setting);
> -		wr32(hw, I40E_PFINT_ITRN(0, vector - 1), q_vector->rx.itr);
> -		q_vector->tx.itr = ITR_TO_REG(vsi->tx_itr_setting);
> -		wr32(hw, I40E_PFINT_ITRN(1, vector - 1), q_vector->tx.itr);
> -		wr32(hw, I40E_PFINT_RATEN(vector - 1), intrl);
> -		i40e_flush(hw);
> +	if (queue < 0) {
> +		for (i = 0; i < vsi->num_q_vectors; i++, vector++)
> +			i40e_set_itr_for_queue(vsi, i, vector);
> +	} else {
> +		if (queue >= vsi->num_q_vectors) {
> +			netif_info(pf, drv, netdev, "Invalid queue value, queue
> range is 0 - %d\n", vsi->num_q_vectors - 1);
> +			return -EINVAL;
> +		}
> +		i40e_set_itr_for_queue(vsi, queue, vector + queue);
>  	}
> 
>  	return 0;
>  }
> 
> +static int i40e_set_coalesce(struct net_device *netdev,
> +			     struct ethtool_coalesce *ec)
> +{
> +	return __i40e_set_coalesce(netdev, ec, -1);
> +}
> +
> +static int i40e_set_per_queue_coalesce(struct net_device *netdev, int
> queue,
> +				       struct ethtool_coalesce *ec)
> +{
> +	return __i40e_set_coalesce(netdev, ec, queue);
> +}
> +
>  /**
>   * i40e_get_rss_hash_opts - Get RSS hash Input Set for each flow type
>   * @pf: pointer to the physical function struct
> @@ -2818,6 +2844,7 @@ static const struct ethtool_ops i40e_ethtool_ops = {
>  	.get_priv_flags		= i40e_get_priv_flags,
>  	.set_priv_flags		= i40e_set_priv_flags,
>  	.get_per_queue_coalesce	= i40e_get_per_queue_coalesce,
> +	.set_per_queue_coalesce	= i40e_set_per_queue_coalesce,
>  };
> 
>  void i40e_set_ethtool_ops(struct net_device *netdev)
> --
> 1.7.11.7

RE: [RFC 5/5] i40e/ethtool: support coalesce setting by queue

[Liang, Kan]

> > diff --git a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
> > b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
> > index b41f0be..5a35fdb 100644
> > --- a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
> > +++ b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
> > @@ -1901,14 +1901,29 @@ static int
> i40e_get_per_queue_coalesce(struct
> > net_device *netdev, int queue,
> >  	return __i40e_get_coalesce(netdev, ec, queue);  }
> >
> > -static int i40e_set_coalesce(struct net_device *netdev,
> > -			     struct ethtool_coalesce *ec)
> > +static void i40e_set_itr_for_queue(struct i40e_vsi *vsi, int queue,
> > +u16
> > vector)
> >  {
> > -	struct i40e_netdev_priv *np = netdev_priv(netdev);
> >  	struct i40e_q_vector *q_vector;
> > -	struct i40e_vsi *vsi = np->vsi;
> >  	struct i40e_pf *pf = vsi->back;
> >  	struct i40e_hw *hw = &pf->hw;
> > +	u16 intrl = INTRL_USEC_TO_REG(vsi->int_rate_limit);
> > +
> > +	q_vector = vsi->q_vectors[queue];
> 
> Again, the problem here is that there can be more queues than vectors, so
> you'll need to check the requested queue number against vsi-
> >num_queue_pairs for the max queue check, then look at vsi-
> >tx_rings[queue].q_vector and/or vsi->rx_rings[queue].q_vector to find
> the actual q_vector for the requested queue.
> 
> Also, this means that it would be possible for the user to try to assign
> different values to queues on the same vector.  How do you want to
> handle that?  My first inclination is to not do anything and just let the last
> value assigned win.
> 

Hi sln,

Thanks for the comments. I will fix it in V2.

Could you please check if the following code can find and set/get vector correctly?

Get rx_usecs and tx_usecs per queue

+	if (queue > 0) {
+		if (queue >= vsi->num_queue_pairs) {
+			netif_info(pf, drv, netdev, "Invalid queue number\n");
+			return -EINVAL;
+		}
+
+		q_vector = vsi->rx_rings[queue]->q_vector;
+		vector = vsi->base_vector + q_vector->v_idx;
+		ec->rx_coalesce_usecs = ITR_REG_TO_USEC(rd32(hw, I40E_PFINT_ITRN(0, vector - 1)));
+
+		q_vector = vsi->tx_rings[queue]->q_vector;
+		vector = vsi->base_vector + q_vector->v_idx;
+		ec->tx_coalesce_usecs = ITR_REG_TO_USEC(rd32(hw, I40E_PFINT_ITRN(1, vector - 1)));
+	}

Set rx_usecs and tx_usecs per queue

+		if (queue >= vsi->num_queue_pairs) {
+			netif_info(pf, drv, netdev, "Invalid queue value, queue range is 0 - %d\n", vsi->num_queue_pairs - 1);
+			return -EINVAL;
+		}
+		intrl = INTRL_USEC_TO_REG(vsi->int_rate_limit);
 
-		q_vector = vsi->q_vectors[i];
+		q_vector = vsi->rx_rings[queue]->q_vector;
+		vector = vsi->base_vector + q_vector->v_idx;
 		q_vector->rx.itr = ITR_TO_REG(vsi->rx_itr_setting);
 		wr32(hw, I40E_PFINT_ITRN(0, vector - 1), q_vector->rx.itr);
+
+		q_vector = vsi->tx_rings[queue]->q_vector;
+		vector = vsi->base_vector + q_vector->v_idx;
 		q_vector->tx.itr = ITR_TO_REG(vsi->tx_itr_setting);
 		wr32(hw, I40E_PFINT_ITRN(1, vector - 1), q_vector->tx.itr);
+
 		wr32(hw, I40E_PFINT_RATEN(vector - 1), intrl);
 		i40e_flush(hw);


Thanks,
Kan

Re: [RFC 1/5] net/ethtool: introduce a new ioctl for per queue setting

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 3408 bytes --]

On Thu, 2015-12-17 at 06:51 +0000, kan.liang@intel.com wrote:
> From: Kan Liang <kan.liang@intel.com>
> 
> Introduce a new ioctl ETHTOOL_PERQUEUE for per queue parameters setting.
> The following patches will enable some SUB_COMMANDs for per queue
> setting.
> 
> Signed-off-by: Kan Liang <kan.liang@intel.com>
> ---
>  include/uapi/linux/ethtool.h | 18 ++++++++++++++++++
>  net/core/ethtool.c           | 17 +++++++++++++++++
>  2 files changed, 35 insertions(+)
> 
> diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
> index cd16291..05bc92a 100644
> --- a/include/uapi/linux/ethtool.h
> +++ b/include/uapi/linux/ethtool.h
> @@ -1144,6 +1144,22 @@ enum ethtool_sfeatures_retval_bits {
>  #define ETHTOOL_F_WISH          (1 << ETHTOOL_F_WISH__BIT)
>  #define ETHTOOL_F_COMPAT        (1 << ETHTOOL_F_COMPAT__BIT)
>  
> +#define MAX_QUEUE		4096
> +#define MAX_QUEUE_MASK		(MAX_QUEUE / 64)
> +
> +/**
> + * struct ethtool_per_queue_op - apply sub command to the queues in mask.
> + * @cmd: ETHTOOL_PERQUEUE
> + * @queue_mask: Mask the queues which sub command apply to
> + * @sub_command: the sub command
> + * @data: parameters of the command
> + */
> +struct ethtool_per_queue_op {
> +	__u32	cmd;
> +	__u64	queue_mask[MAX_QUEUE_MASK];
> +	__u32	sub_command;

This leaves a hole in the structure on i386, which would then require
conversion in the compat ioctl implementation.  I suggest you move the
sub_command next to cmd.

> +	char	data[];
> +};
>  
>  /* CMDs currently supported */
>  #define ETHTOOL_GSET		0x00000001 /* Get settings. */
> @@ -1226,6 +1242,8 @@ enum ethtool_sfeatures_retval_bits {
>  #define ETHTOOL_GTUNABLE	0x00000048 /* Get tunable configuration */
>  #define ETHTOOL_STUNABLE	0x00000049 /* Set tunable configuration */
>  
> +#define ETHTOOL_PERQUEUE	0x0000004a /* Set per queue options */

But it will actually be used for both set and get, right?

> +
>  /* 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 29edf74..125fb32 100644
> --- a/net/core/ethtool.c
> +++ b/net/core/ethtool.c
> @@ -1748,6 +1748,20 @@ out:
>  	return ret;
>  }
>  
> +static int ethtool_set_per_queue(struct net_device *dev, void __user *useraddr)
> +{
> +	struct ethtool_per_queue_op per_queue_opt;
> +
> +	if (copy_from_user(&per_queue_opt, useraddr, sizeof(per_queue_opt)))
> +		return -EFAULT;
> +
> +	switch (per_queue_opt.sub_command) {
> +
> +	default:
> +		return -EOPNOTSUPP;
> +	};
> +}
> +
>  /* The main entry point in this file.  Called from net/core/dev_ioctl.c */
>  
>  int dev_ethtool(struct net *net, struct ifreq *ifr)
> @@ -1991,6 +2005,9 @@ int dev_ethtool(struct net *net, struct ifreq *ifr)
>  	case ETHTOOL_STUNABLE:
>  		rc = ethtool_set_tunable(dev, useraddr);
>  		break;
> +	case ETHTOOL_PERQUEUE:
> +		rc = ethtool_set_per_queue(dev, useraddr);
> +		break;
>  	default:
>  		rc = -EOPNOTSUPP;
>  	}

I think the first switch statement to determine whether the command
requires CAP_NET_ADMIN should also handle ETHTOOL_PERQUEUE, checking
the sub-command number.

Ben.

-- 
Ben Hutchings
Always try to do things in chronological order;
it's less confusing that way.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

Re: [RFC 2/5] net/ethtool: support get coalesce per queue

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 2889 bytes --]

On Thu, 2015-12-17 at 06:51 +0000, kan.liang@intel.com wrote:
> From: Kan Liang <kan.liang@intel.com>
> 
> Device driver has to provide an interface to get per queue coalesce.
> The interrupt coalescing parameters of each masked queue will be
> copied back to user space one by one.
> 
> Signed-off-by: Kan Liang <kan.liang@intel.com>
> ---
>  include/linux/ethtool.h |  5 ++++-
>  net/core/ethtool.c      | 33 ++++++++++++++++++++++++++++++++-
>  2 files changed, 36 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
> index 653dc9c..107f75f 100644
> --- a/include/linux/ethtool.h
> +++ b/include/linux/ethtool.h
> @@ -201,6 +201,8 @@ static inline u32 ethtool_rxfh_indir_default(u32 index, u32 n_rx_rings)
>   * @get_module_eeprom: Get the eeprom information from the plug-in module
>   * @get_eee: Get Energy-Efficient (EEE) supported and status.
>   * @set_eee: Set EEE status (enable/disable) as well as LPI timers.
> + * @get_per_queue_coalesce: Get interrupt coalescing parameters per queue.
> + * 	Returns a negative error code or zero.

Is the ethtool core or the driver responsible for range-checking the
queue number?

What is reported if the queue number is in range for RX queues but not
for TX queues (or vice versa)?

>   *
>   * All operations are optional (i.e. the function pointer may be set
>   * to %NULL) and callers must take this into account.  Callers must
> @@ -279,7 +281,8 @@ struct ethtool_ops {
>  			       const struct ethtool_tunable *, void *);
>  	int	(*set_tunable)(struct net_device *,
>  			       const struct ethtool_tunable *, const void *);
> -
> +	int	(*get_per_queue_coalesce)(struct net_device *, int,
> +					  struct ethtool_coalesce *);
>  
>  };
>  #endif /* _LINUX_ETHTOOL_H */
> diff --git a/net/core/ethtool.c b/net/core/ethtool.c
> index 125fb32..22ff69a 100644
> --- a/net/core/ethtool.c
> +++ b/net/core/ethtool.c
> @@ -1748,6 +1748,36 @@ out:
>  	return ret;
>  }
>  
> +static int ethtool_get_per_queue_coalesce(struct net_device *dev,
> +					  void __user *useraddr,
> +					  struct ethtool_per_queue_op *per_queue_opt)
> +{
> +	u64 queue_mask;
> +	int bit, i, ret;
> +
> +	if (!dev->ethtool_ops->get_per_queue_coalesce)
> +		return -EOPNOTSUPP;
> +
> +	useraddr += sizeof(*per_queue_opt);
> +	for (i = 0; i < MAX_QUEUE_MASK; i++) {
> +		queue_mask = per_queue_opt->queue_mask[i];
> +		if (queue_mask > 0) {
> +			for_each_set_bit(bit, (unsigned long *)&queue_mask, 64) {
[...]

This assumes a little-endian system.

You should probably build on top of David Decotigny's recent patch
series that adds conversion of u32-based bitmaps to kernel bitmaps.

Ben.

-- 
Ben Hutchings
Always try to do things in chronological order;
it's less confusing that way.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

Re: [RFC 1/5] net/ethtool: introduce a new ioctl for per queue setting

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 1666 bytes --]

On Sat, 2015-12-19 at 03:27 +0000, Ben Hutchings wrote:
> On Thu, 2015-12-17 at 06:51 +0000, kan.liang@intel.com wrote:
> > From: Kan Liang <kan.liang@intel.com>
> > 
> > Introduce a new ioctl ETHTOOL_PERQUEUE for per queue parameters setting.
> > The following patches will enable some SUB_COMMANDs for per queue
> > setting.
> > 
> > Signed-off-by: Kan Liang <kan.liang@intel.com>
> > ---
> >  include/uapi/linux/ethtool.h | 18 ++++++++++++++++++
> >  net/core/ethtool.c           | 17 +++++++++++++++++
> >  2 files changed, 35 insertions(+)
> > 
> > diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
> > index cd16291..05bc92a 100644
> > --- a/include/uapi/linux/ethtool.h
> > +++ b/include/uapi/linux/ethtool.h
> > @@ -1144,6 +1144,22 @@ enum ethtool_sfeatures_retval_bits {
> >  #define ETHTOOL_F_WISH          (1 << ETHTOOL_F_WISH__BIT)
> >  #define ETHTOOL_F_COMPAT        (1 << ETHTOOL_F_COMPAT__BIT)
> >  
> > +#define MAX_QUEUE		4096
> > +#define MAX_QUEUE_MASK		(MAX_QUEUE / 64)
> > +
> > +/**
> > + * struct ethtool_per_queue_op - apply sub command to the queues in mask.
> > + * @cmd: ETHTOOL_PERQUEUE
> > + * @queue_mask: Mask the queues which sub command apply to
> > + * @sub_command: the sub command
> > + * @data: parameters of the command
[...]

You also need to make it clear that there's a complete command
structure following for each of the queues addressed, both for get and
set.  I.e. set does not apply the same settings to all queues.

Ben.

-- 
Ben Hutchings
Always try to do things in chronological order;
it's less confusing that way.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

Re: [RFC 3/5] net/ethtool: support set coalesce per queue

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 1302 bytes --]

On Thu, 2015-12-17 at 06:51 +0000, kan.liang@intel.com wrote:
[...]
> --- a/net/core/ethtool.c
> +++ b/net/core/ethtool.c
> @@ -1778,6 +1778,37 @@ static int ethtool_get_per_queue_coalesce(struct net_device *dev,
>  	return 0;
>  }
>  
> +static int ethtool_set_per_queue_coalesce(struct net_device *dev,
> +					  void __user *useraddr,
> +					  struct ethtool_per_queue_op *per_queue_opt)
> +{
> +	u64 queue_mask;
> +	int bit, i, ret;
> +
> +	if (!dev->ethtool_ops->set_per_queue_coalesce)
> +		return -EOPNOTSUPP;
> +
> +	useraddr += sizeof(*per_queue_opt);
> +	for (i = 0; i < MAX_QUEUE_MASK; i++) {
> +		queue_mask = per_queue_opt->queue_mask[i];
> +		if (queue_mask > 0) {
> +			for_each_set_bit(bit, (unsigned long *)&queue_mask, 64) {
> +				struct ethtool_coalesce coalesce;
> +
> +				if (copy_from_user(&coalesce, useraddr, sizeof(coalesce)))
> +					return -EFAULT;
> +
> +				ret = dev->ethtool_ops->set_per_queue_coalesce(dev, bit + i * 64, &coalesce);
> +				if (ret != 0)
> +					return ret;
[...]

So there's no attempt to roll back on failure?

Then, what is the benefit of doing this iteration in the kernel rather
than userland?

Ben.

-- 
Ben Hutchings
Always try to do things in chronological order;
it's less confusing that way.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

RE: [RFC 5/5] i40e/ethtool: support coalesce setting by queue

[Nelson, Shannon]

> From: Liang, Kan
> Sent: Friday, December 18, 2015 1:06 PM
> 
> Hi sln,
> 
> Thanks for the comments. I will fix it in V2.
> 
> Could you please check if the following code can find and set/get vector
> correctly?
> 
> Get rx_usecs and tx_usecs per queue
> 
> +	if (queue > 0) {
> +		if (queue >= vsi->num_queue_pairs) {
> +			netif_info(pf, drv, netdev, "Invalid queue number\n");

Adding queue range information here like in the next chunk would be nice.

Otherwise, I think the rest of this looks reasonable.

sln

RE: [RFC 1/5] net/ethtool: introduce a new ioctl for per queue setting

[Liang, Kan]

> > +#define MAX_QUEUE		4096
> > +#define MAX_QUEUE_MASK		(MAX_QUEUE / 64)
> > +
> > +/**
> > + * struct ethtool_per_queue_op - apply sub command to the queues in
> mask.
> > + * @cmd: ETHTOOL_PERQUEUE
> > + * @queue_mask: Mask the queues which sub command apply to
> > + * @sub_command: the sub command
> > + * @data: parameters of the command
> > + */
> > +struct ethtool_per_queue_op {
> > +	__u32	cmd;
> > +	__u64	queue_mask[MAX_QUEUE_MASK];
> > +	__u32	sub_command;
> 
> This leaves a hole in the structure on i386, which would then require
> conversion in the compat ioctl implementation.  I suggest you move the
> sub_command next to cmd.

OK. I will modify it in V2.

> 
> > +	char	data[];
> > +};
> >
> >  /* CMDs currently supported */
> >  #define ETHTOOL_GSET		0x00000001 /* Get settings. */
> > @@ -1226,6 +1242,8 @@ enum ethtool_sfeatures_retval_bits {
> >  #define ETHTOOL_GTUNABLE	0x00000048 /* Get tunable
> configuration */
> >  #define ETHTOOL_STUNABLE	0x00000049 /* Set tunable
> configuration */
> >
> > +#define ETHTOOL_PERQUEUE	0x0000004a /* Set per queue
> options */
> 
> But it will actually be used for both set and get, right?

Right.

> 
> > +
> >  /* 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
> > 29edf74..125fb32 100644
> > --- a/net/core/ethtool.c
> > +++ b/net/core/ethtool.c
> > @@ -1748,6 +1748,20 @@ out:
> >  	return ret;
> >  }
> >
> > +static int ethtool_set_per_queue(struct net_device *dev, void __user
> > +*useraddr) {
> > +	struct ethtool_per_queue_op per_queue_opt;
> > +
> > +	if (copy_from_user(&per_queue_opt, useraddr,
> sizeof(per_queue_opt)))
> > +		return -EFAULT;
> > +
> > +	switch (per_queue_opt.sub_command) {
> > +
> > +	default:
> > +		return -EOPNOTSUPP;
> > +	};
> > +}
> > +
> >  /* The main entry point in this file.  Called from
> > net/core/dev_ioctl.c */
> >
> >  int dev_ethtool(struct net *net, struct ifreq *ifr) @@ -1991,6
> > +2005,9 @@ int dev_ethtool(struct net *net, struct ifreq *ifr)
> >  	case ETHTOOL_STUNABLE:
> >  		rc = ethtool_set_tunable(dev, useraddr);
> >  		break;
> > +	case ETHTOOL_PERQUEUE:
> > +		rc = ethtool_set_per_queue(dev, useraddr);
> > +		break;
> >  	default:
> >  		rc = -EOPNOTSUPP;
> >  	}
> 
> I think the first switch statement to determine whether the command
> requires CAP_NET_ADMIN should also handle ETHTOOL_PERQUEUE,
> checking the sub-command number.
>

OK. I will add it in V2. Only 'get' can be done by anyone.

Thanks,
Kan

 

RE: [RFC 2/5] net/ethtool: support get coalesce per queue

[Liang, Kan]

> On Thu, 2015-12-17 at 06:51 +0000, kan.liang@intel.com wrote:
> > From: Kan Liang <kan.liang@intel.com>
> >
> > Device driver has to provide an interface to get per queue coalesce.
> > The interrupt coalescing parameters of each masked queue will be
> > copied back to user space one by one.
> >
> > Signed-off-by: Kan Liang <kan.liang@intel.com>
> > ---
> >  include/linux/ethtool.h |  5 ++++-
> >  net/core/ethtool.c      | 33 ++++++++++++++++++++++++++++++++-
> >  2 files changed, 36 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h index
> > 653dc9c..107f75f 100644
> > --- a/include/linux/ethtool.h
> > +++ b/include/linux/ethtool.h
> > @@ -201,6 +201,8 @@ static inline u32 ethtool_rxfh_indir_default(u32
> > index, u32 n_rx_rings)
> >   * @get_module_eeprom: Get the eeprom information from the plug-in
> > module
> >   * @get_eee: Get Energy-Efficient (EEE) supported and status.
> >   * @set_eee: Set EEE status (enable/disable) as well as LPI timers.
> > + * @get_per_queue_coalesce: Get interrupt coalescing parameters per
> queue.
> > + * 	Returns a negative error code or zero.
> 
> Is the ethtool core or the driver responsible for range-checking the queue
> number?
> 
> What is reported if the queue number is in range for RX queues but not for
> TX queues (or vice versa)?
> 
The driver should do the range-checking.
I think it's also driver to determine which should be reported
in tx_coalesce_usecs, if RX in range but TX not. Maybe ~0 is a good return
for tx_coalesce_usecs.


> >   *
> >   * All operations are optional (i.e. the function pointer may be set
> >   * to %NULL) and callers must take this into account.  Callers must
> > @@ -279,7 +281,8 @@ struct ethtool_ops {
> >  			       const struct ethtool_tunable *, void *);
> >  	int	(*set_tunable)(struct net_device *,
> >  			       const struct ethtool_tunable *, const void *);
> > -
> > +	int	(*get_per_queue_coalesce)(struct net_device *, int,
> > +					  struct ethtool_coalesce *);
> >
> >  };
> >  #endif /* _LINUX_ETHTOOL_H */
> > diff --git a/net/core/ethtool.c b/net/core/ethtool.c index
> > 125fb32..22ff69a 100644
> > --- a/net/core/ethtool.c
> > +++ b/net/core/ethtool.c
> > @@ -1748,6 +1748,36 @@ out:
> >  	return ret;
> >  }
> >
> > +static int ethtool_get_per_queue_coalesce(struct net_device *dev,
> > +					  void __user *useraddr,
> > +					  struct ethtool_per_queue_op
> *per_queue_opt) {
> > +	u64 queue_mask;
> > +	int bit, i, ret;
> > +
> > +	if (!dev->ethtool_ops->get_per_queue_coalesce)
> > +		return -EOPNOTSUPP;
> > +
> > +	useraddr += sizeof(*per_queue_opt);
> > +	for (i = 0; i < MAX_QUEUE_MASK; i++) {
> > +		queue_mask = per_queue_opt->queue_mask[i];
> > +		if (queue_mask > 0) {
> > +			for_each_set_bit(bit, (unsigned long
> *)&queue_mask, 64) {
> [...]
> 
> This assumes a little-endian system.
> 
> You should probably build on top of David Decotigny's recent patch series
> that adds conversion of u32-based bitmaps to kernel bitmaps.
> 
Thanks for pointing me to the patch.
I will change the code based on his patch.

Thanks,
Kan

RE: [RFC 3/5] net/ethtool: support set coalesce per queue

[Liang, Kan]

> On Thu, 2015-12-17 at 06:51 +0000, kan.liang@intel.com wrote:
> [...]
> > --- a/net/core/ethtool.c
> > +++ b/net/core/ethtool.c
> > @@ -1778,6 +1778,37 @@ static int
> ethtool_get_per_queue_coalesce(struct net_device *dev,
> >  	return 0;
> >  }
> >
> > +static int ethtool_set_per_queue_coalesce(struct net_device *dev,
> > +					  void __user *useraddr,
> > +					  struct ethtool_per_queue_op
> *per_queue_opt) {
> > +	u64 queue_mask;
> > +	int bit, i, ret;
> > +
> > +	if (!dev->ethtool_ops->set_per_queue_coalesce)
> > +		return -EOPNOTSUPP;
> > +
> > +	useraddr += sizeof(*per_queue_opt);
> > +	for (i = 0; i < MAX_QUEUE_MASK; i++) {
> > +		queue_mask = per_queue_opt->queue_mask[i];
> > +		if (queue_mask > 0) {
> > +			for_each_set_bit(bit, (unsigned long
> *)&queue_mask, 64) {
> > +				struct ethtool_coalesce coalesce;
> > +
> > +				if (copy_from_user(&coalesce, useraddr,
> sizeof(coalesce)))
> > +					return -EFAULT;
> > +
> > +				ret = dev->ethtool_ops-
> >set_per_queue_coalesce(dev, bit + i * 64, &coalesce);
> > +				if (ret != 0)
> > +					return ret;
> [...]
> 
> So there's no attempt to roll back on failure?
> 
No, ethtool core doesn’t save the old value for this version. So it's hard to
Rollback.
If we want this feature, the kernel will alloc a chunk of memory to store
coalesce value for masked queues. Is it OK?

> Then, what is the benefit of doing this iteration in the kernel rather than
> userland?
> 
It should save many ioctls.

Thanks,
Kan