[RFC 1/2] net/ethtool: Add new coalescing parameter for queue

[RFC 1/2] net/ethtool: Add new coalescing parameter for queue

[kan.liang]

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

Intrdouce "queue" option for coalesce getting and setting.
For coalesce getting, only the coalescing parameters from specific
queue will be passed to user space.
For coalesce setting, the coalescing parameters will only be applied to
specific queue.
If the queue is set to -1, the coalescing parameters will apply to all
queues.

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

diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
index cd16291..f4fc18b 100644
--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h
@@ -384,6 +384,7 @@ struct ethtool_modinfo {
  *	a TX interrupt, when the packet rate is above @pkt_rate_high.
  * @rate_sample_interval: How often to do adaptive coalescing packet rate
  *	sampling, measured in seconds.  Must not be zero.
+ * @queue: The specific queue which coalescing parameters apply to.
  *
  * Each pair of (usecs, max_frames) fields specifies that interrupts
  * should be coalesced until
@@ -434,6 +435,7 @@ struct ethtool_coalesce {
 	__u32	tx_coalesce_usecs_high;
 	__u32	tx_max_coalesced_frames_high;
 	__u32	rate_sample_interval;
+	__s32	queue;
 };
 
 /**
diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index 29edf74..fa8ab7a 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -1123,10 +1123,16 @@ static noinline_for_stack int ethtool_get_coalesce(struct net_device *dev,
 						   void __user *useraddr)
 {
 	struct ethtool_coalesce coalesce = { .cmd = ETHTOOL_GCOALESCE };
+	struct ethtool_coalesce tmp = { .queue = -1 };
 
 	if (!dev->ethtool_ops->get_coalesce)
 		return -EOPNOTSUPP;
 
+	if (copy_from_user(&tmp, useraddr, sizeof(coalesce)))
+		return -EFAULT;
+
+	coalesce.queue = tmp.queue;
+
 	dev->ethtool_ops->get_coalesce(dev, &coalesce);
 
 	if (copy_to_user(useraddr, &coalesce, sizeof(coalesce)))
-- 
1.7.11.7

[RFC 2/2] i40e/ethtool: support per queue coalesce getting/setting

[kan.liang]

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

If queue is not -1, apply rx and tx interrupt moderation value on
specific queue. Otherwise, apply the interrupt moderation value to all
queues.

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

diff --git a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
index 6cb2b34..6f63e11 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
@@ -1849,6 +1849,10 @@ static int i40e_get_coalesce(struct net_device *netdev,
 {
 	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 = vsi->base_vector;
 
 	ec->tx_max_coalesced_frames_irq = vsi->work_limit;
 	ec->rx_max_coalesced_frames_irq = vsi->work_limit;
@@ -1870,17 +1874,44 @@ 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 (ec->queue > -1) {
+		if (ec->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;
+		}
+
+		q_vector = vsi->q_vectors[ec->queue];
+		vector += ec->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 void i40e_set_itr_for_queue(struct i40e_vsi *vsi, int queue, u16 vector)
+{
+	struct i40e_q_vector *q_vector;
+	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)
 {
 	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 vector;
 	int i;
 
@@ -1936,16 +1967,15 @@ 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 (ec->queue < 0) {
+		for (i = 0; i < vsi->num_q_vectors; i++, vector++)
+			i40e_set_itr_for_queue(vsi, i, vector);
+	} else {
+		if (ec->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, ec->queue, vector + ec->queue);
 	}
 
 	return 0;
-- 
1.7.11.7

Re: [RFC 1/2] net/ethtool: Add new coalescing parameter for queue

[Shannon Nelson]

On Mon, Dec 7, 2015 at 8:42 PM,  <kan.liang@intel.com> wrote:
> From: Kan Liang <kan.liang@intel.com>
>
> Intrdouce "queue" option for coalesce getting and setting.

s/Intrdouce/Introduce/

> For coalesce getting, only the coalescing parameters from specific
> queue will be passed to user space.
> For coalesce setting, the coalescing parameters will only be applied to
> specific queue.
> If the queue is set to -1, the coalescing parameters will apply to all
> queues.
>
> Signed-off-by: Kan Liang <kan.liang@intel.com>
> ---
>  include/uapi/linux/ethtool.h | 2 ++
>  net/core/ethtool.c           | 6 ++++++
>  2 files changed, 8 insertions(+)
>
> diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
> index cd16291..f4fc18b 100644
> --- a/include/uapi/linux/ethtool.h
> +++ b/include/uapi/linux/ethtool.h
> @@ -384,6 +384,7 @@ struct ethtool_modinfo {
>   *     a TX interrupt, when the packet rate is above @pkt_rate_high.
>   * @rate_sample_interval: How often to do adaptive coalescing packet rate
>   *     sampling, measured in seconds.  Must not be zero.
> + * @queue: The specific queue which coalescing parameters apply to.
>   *
>   * Each pair of (usecs, max_frames) fields specifies that interrupts
>   * should be coalesced until
> @@ -434,6 +435,7 @@ struct ethtool_coalesce {
>         __u32   tx_coalesce_usecs_high;
>         __u32   tx_max_coalesced_frames_high;
>         __u32   rate_sample_interval;
> +       __s32   queue;
>  };
>
>  /**
> diff --git a/net/core/ethtool.c b/net/core/ethtool.c
> index 29edf74..fa8ab7a 100644
> --- a/net/core/ethtool.c
> +++ b/net/core/ethtool.c
> @@ -1123,10 +1123,16 @@ static noinline_for_stack int ethtool_get_coalesce(struct net_device *dev,
>                                                    void __user *useraddr)
>  {
>         struct ethtool_coalesce coalesce = { .cmd = ETHTOOL_GCOALESCE };
> +       struct ethtool_coalesce tmp = { .queue = -1 };
>
>         if (!dev->ethtool_ops->get_coalesce)
>                 return -EOPNOTSUPP;
>
> +       if (copy_from_user(&tmp, useraddr, sizeof(coalesce)))
> +               return -EFAULT;
> +
> +       coalesce.queue = tmp.queue;
> +

Is this going to do what you expect when you have an older ethtool
program?  It seems to me that the older ethtool program will have the
original coalesce struct without the new queue field, so when you do
the copy_from_user(), you will get fewer bytes than what you asked
for, and the remaining bytes will get set to '0'.  I think this means
tmp.queue will be '0', not the default '-1', so the info returned will
always be for queue 0, not "all the queues".

For that matter, if you've set different coalesce values on various
queues, what does asking for '-1' or "all the queues" mean and how
should it return info? Shouldn't all the specific queues be shown?  Or
do we now have to do an ethtool command for each and every queue?

sln

Re: [RFC 1/2] net/ethtool: Add new coalescing parameter for queue

[Florian Fainelli]

On 07/12/15 20:42, kan.liang@intel.com wrote:
> From: Kan Liang <kan.liang@intel.com>
> 
> Intrdouce "queue" option for coalesce getting and setting.
> For coalesce getting, only the coalescing parameters from specific
> queue will be passed to user space.
> For coalesce setting, the coalescing parameters will only be applied to
> specific queue.
> If the queue is set to -1, the coalescing parameters will apply to all
> queues.

This looks like a good start, but there are a few things that need to be
clarified, in particular:

- if the number of TX and RX queues differ, but the ethtool coalesce
structure contains parameters that affect both the RX and TX side, and
the queue number is invalid/non-existent for one of these sides, what is
the expected outcome? Same question with specifying a queue number, with
RX queue N not belonging to the same queue pair as TX queue N?

- from an user perspective do we want to iterate over all queues to set
their parameters, or should we have a queue bitmask parameter which
allows setting them with the same settings in one shot? What would be
the appropriate bitmask size then (32-bits with 16-bits for TX and
16-bits for RX might be too small)?

Thanks
-- 
Florian

Re: [RFC 1/2] net/ethtool: Add new coalescing parameter for queue

[Ben Hutchings]

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

On Tue, 2015-12-08 at 10:02 -0800, Shannon Nelson wrote:
> On Mon, Dec 7, 2015 at 8:42 PM,  <kan.liang@intel.com> wrote:
> > From: Kan Liang <kan.liang@intel.com>
> > 
> > Intrdouce "queue" option for coalesce getting and setting.
[...]
> > --- a/net/core/ethtool.c
> > +++ b/net/core/ethtool.c
> > @@ -1123,10 +1123,16 @@ static noinline_for_stack int ethtool_get_coalesce(struct net_device *dev,
> >                                                    void __user *useraddr)
> >  {
> >         struct ethtool_coalesce coalesce = { .cmd = ETHTOOL_GCOALESCE };
> > +       struct ethtool_coalesce tmp = { .queue = -1 };
> > 
> >         if (!dev->ethtool_ops->get_coalesce)
> >                 return -EOPNOTSUPP;
> > 
> > +       if (copy_from_user(&tmp, useraddr, sizeof(coalesce)))
> > +               return -EFAULT;
> > +
> > +       coalesce.queue = tmp.queue;
> > +
> 
> Is this going to do what you expect when you have an older ethtool
> program?  It seems to me that the older ethtool program will have the
> original coalesce struct without the new queue field, so when you do
[...]

Indeed, it's not safe to extend UAPI structures like this.

Ben.

-- 
Ben Hutchings
Life would be so much easier if we could look at the source code.

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

RE: [RFC 1/2] net/ethtool: Add new coalescing parameter for queue

[Liang, Kan]

> 
> For that matter, if you've set different coalesce values on various queues,
> what does asking for '-1' or "all the queues" mean and how should it return
> info? Shouldn't all the specific queues be shown?  Or do we now have to
> do an ethtool command for each and every queue?
>

The only way to show the value of specific queue is to apply queue option,
like "ethtool -c DEVNAME queue N"
If using "all the queues", the last value which set by ethtool -C will be
shown.  

Yes, "all the queues" is misleading. I think we may force user to specify the
queue when using --show-coalesce. Queue 0 is the default queue.
If we really want to show all queues by default, we may need to
change the ethtool_get_coalesce interface. However, that will impact all
other drivers.
I'm not sure which one is better.

Thanks,
Kan

RE: [RFC 1/2] net/ethtool: Add new coalescing parameter for queue

[Liang, Kan]

> On 07/12/15 20:42, kan.liang@intel.com wrote:
> > From: Kan Liang <kan.liang@intel.com>
> >
> > Intrdouce "queue" option for coalesce getting and setting.
> > For coalesce getting, only the coalescing parameters from specific
> > queue will be passed to user space.
> > For coalesce setting, the coalescing parameters will only be applied
> > to specific queue.
> > If the queue is set to -1, the coalescing parameters will apply to all
> > queues.
> 
> This looks like a good start, but there are a few things that need to be
> clarified, in particular:
> 
> - if the number of TX and RX queues differ, but the ethtool coalesce
> structure contains parameters that affect both the RX and TX side, and the
> queue number is invalid/non-existent for one of these sides, what is the
> expected outcome? Same question with specifying a queue number, with
> RX queue N not belonging to the same queue pair as TX queue N?
>
I thing I should introduce "tx_queue" and "rx_queue" separately
in next version.
 
> - from an user perspective do we want to iterate over all queues to set
> their parameters, or should we have a queue bitmask parameter which
> allows setting them with the same settings in one shot? What would be
> the appropriate bitmask size then (32-bits with 16-bits for TX and 16-bits
> for RX might be too small)?
> 

It's a good idea to use queue bitmask, however I have no idea how many
Bits should we reserve for TX and RX queues.
For my test platform, there are 48 queue pair. So 48 bits are needed for TX
and another 48 bits for RX.
There could be more queues in other plarforms...

Thanks,
Kan

RE: [RFC 1/2] net/ethtool: Add new coalescing parameter for queue

[Liang, Kan]

> 
> On Tue, 2015-12-08 at 10:02 -0800, Shannon Nelson wrote:
> > On Mon, Dec 7, 2015 at 8:42 PM,  <kan.liang@intel.com> wrote:
> > > From: Kan Liang <kan.liang@intel.com>
> > >
> > > Intrdouce "queue" option for coalesce getting and setting.
> [...]
> > > --- a/net/core/ethtool.c
> > > +++ b/net/core/ethtool.c
> > > @@ -1123,10 +1123,16 @@ static noinline_for_stack int
> > > ethtool_get_coalesce(struct net_device *dev,
> > >                                                    void __user
> > > *useraddr)
> > >  {
> > >         struct ethtool_coalesce coalesce = { .cmd =
> > > ETHTOOL_GCOALESCE };
> > > +       struct ethtool_coalesce tmp = { .queue = -1 };
> > >
> > >         if (!dev->ethtool_ops->get_coalesce)
> > >                 return -EOPNOTSUPP;
> > >
> > > +       if (copy_from_user(&tmp, useraddr, sizeof(coalesce)))
> > > +               return -EFAULT;
> > > +
> > > +       coalesce.queue = tmp.queue;
> > > +
> >
> > Is this going to do what you expect when you have an older ethtool
> > program?  It seems to me that the older ethtool program will have the
> > original coalesce struct without the new queue field, so when you do
> [...]
> 
> Indeed, it's not safe to extend UAPI structures like this.

Any suggestions for that?
If we cannot extend the existing UAPI structures. I guess we have to add
a new Ioctl like ETHTOOL_GCOALESCE_V2 for the new structure?

Thanks,
Kan

Re: [RFC 1/2] net/ethtool: Add new coalescing parameter for queue

[Ben Hutchings]

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

On Tue, 2015-12-08 at 19:43 +0000, Liang, Kan wrote:
> 
> > 
> > On Tue, 2015-12-08 at 10:02 -0800, Shannon Nelson wrote:
> > > On Mon, Dec 7, 2015 at 8:42 PM,  <kan.liang@intel.com> wrote:
> > > > From: Kan Liang <kan.liang@intel.com>
> > > > 
> > > > Intrdouce "queue" option for coalesce getting and setting.
> > [...]
> > > > --- a/net/core/ethtool.c
> > > > +++ b/net/core/ethtool.c
> > > > @@ -1123,10 +1123,16 @@ static noinline_for_stack int
> > > > ethtool_get_coalesce(struct net_device *dev,
> > > >                                                    void __user
> > > > *useraddr)
> > > >  {
> > > >         struct ethtool_coalesce coalesce = { .cmd =
> > > > ETHTOOL_GCOALESCE };
> > > > +       struct ethtool_coalesce tmp = { .queue = -1 };
> > > > 
> > > >         if (!dev->ethtool_ops->get_coalesce)
> > > >                 return -EOPNOTSUPP;
> > > > 
> > > > +       if (copy_from_user(&tmp, useraddr, sizeof(coalesce)))
> > > > +               return -EFAULT;
> > > > +
> > > > +       coalesce.queue = tmp.queue;
> > > > +
> > > 
> > > Is this going to do what you expect when you have an older ethtool
> > > program?  It seems to me that the older ethtool program will have the
> > > original coalesce struct without the new queue field, so when you do
> > [...]
> > 
> > Indeed, it's not safe to extend UAPI structures like this.
> 
> Any suggestions for that?
> If we cannot extend the existing UAPI structures. I guess we have to add
> a new Ioctl like ETHTOOL_GCOALESCE_V2 for the new structure?

Something like that.

While you're at it, add a flags field which will indicate which of the
others are supported.  Also, there was some talk of adding settings for
RX DMA coalescing.

Ben.

-- 
Ben Hutchings
Life would be so much easier if we could look at the source code.

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

Re: [RFC 1/2] net/ethtool: Add new coalescing parameter for queue

[David Miller]

From: Ben Hutchings <ben@decadent.org.uk>
Date: Tue, 08 Dec 2015 19:00:52 +0000

> On Tue, 2015-12-08 at 10:02 -0800, Shannon Nelson wrote:
>> On Mon, Dec 7, 2015 at 8:42 PM,  <kan.liang@intel.com> wrote:
>> > From: Kan Liang <kan.liang@intel.com>
>> > 
>> > Intrdouce "queue" option for coalesce getting and setting.
> [...]
>> > --- a/net/core/ethtool.c
>> > +++ b/net/core/ethtool.c
>> > @@ -1123,10 +1123,16 @@ static noinline_for_stack int ethtool_get_coalesce(struct net_device *dev,
>> >                                                    void __user *useraddr)
>> >  {
>> >         struct ethtool_coalesce coalesce = { .cmd = ETHTOOL_GCOALESCE };
>> > +       struct ethtool_coalesce tmp = { .queue = -1 };
>> > 
>> >         if (!dev->ethtool_ops->get_coalesce)
>> >                 return -EOPNOTSUPP;
>> > 
>> > +       if (copy_from_user(&tmp, useraddr, sizeof(coalesce)))
>> > +               return -EFAULT;
>> > +
>> > +       coalesce.queue = tmp.queue;
>> > +
>> 
>> Is this going to do what you expect when you have an older ethtool
>> program?  It seems to me that the older ethtool program will have the
>> original coalesce struct without the new queue field, so when you do
> [...]
> 
> Indeed, it's not safe to extend UAPI structures like this.

Agreed, and this is really very far from what I suggested.

Please, make a new ethtool command entirely, that is a container.
Name is 'ETHTOOL_PERQUEUE' or something like that.

It's base type is a structure that describes what queue the
encapsulated operation is to be applied to.

struct ethtool_per_queue_op {
	u32	queue_mask[BITS_TO_LONGS(WHATEVER)];
	u32	sub_command;
	char	data[];
};

Then sub_command can be ETHTOOL_SCOALESCE, or any other ethtool
command that might be valid to be specified on a per-queue basis.
And then at 'data' you have and array of objects, whose type is of
the sub-command's expected type, and has one entry for each bit
set in queue_mask.

If you limit this facility to only ETHTOOL_{S,G}COALESCE now, believe
me you'll regret it.