Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1396180 > unrolled thread

[PATCH net-next 5/7] Driver: Vmxnet3: Add support for get_coalesce, set_coalesce ethtool operations

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH net-next 5/7] Driver: Vmxnet3: Add support for get_coalesce, set_coalesce ethtool operations Shrikrishna Khare <skhare@vmware.com> - 2016-05-07 01:20 +0200
    Re: [PATCH net-next 5/7] Driver: Vmxnet3: Add support for  get_coalesce, set_coalesce ethtool operations Ben Hutchings <ben@decadent.org.uk> - 2016-05-07 14:10 +0200
      Re: [PATCH net-next 5/7] Driver: Vmxnet3: Add support for  get_coalesce, set_coalesce ethtool operations David Miller <davem@davemloft.net> - 2016-05-07 19:50 +0200
      Re: [PATCH net-next 5/7] Driver: Vmxnet3: Add support for get_coalesce,  set_coalesce ethtool operations Shrikrishna Khare <skhare@vmware.com> - 2016-05-08 23:00 +0200
        Re: [PATCH net-next 5/7] Driver: Vmxnet3: Add support for  get_coalesce, set_coalesce ethtool operations Ben Hutchings <ben@decadent.org.uk> - 2016-05-09 02:20 +0200
          RE: [PATCH net-next 5/7] Driver: Vmxnet3: Add support for  get_coalesce, set_coalesce ethtool operations David Laight <David.Laight@ACULAB.COM> - 2016-05-10 13:30 +0200
            Re: [PATCH net-next 5/7] Driver: Vmxnet3: Add support for  get_coalesce, set_coalesce ethtool operations Ben Hutchings <ben@decadent.org.uk> - 2016-05-10 14:10 +0200

#1396180 — [PATCH net-next 5/7] Driver: Vmxnet3: Add support for get_coalesce, set_coalesce ethtool operations

Signed-off-by: Keyong Sun <sunk@vmware.com>
Signed-off-by: Manoj Tammali <tammalim@vmware.com>
Signed-off-by: Shrikrishna Khare <skhare@vmware.com>
---
 drivers/net/vmxnet3/vmxnet3_defs.h    |  32 +++++++-
 drivers/net/vmxnet3/vmxnet3_drv.c     |  47 ++++++++++++
 drivers/net/vmxnet3/vmxnet3_ethtool.c | 135 ++++++++++++++++++++++++++++++++++
 drivers/net/vmxnet3/vmxnet3_int.h     |   5 ++
 4 files changed, 218 insertions(+), 1 deletion(-)

diff --git a/drivers/net/vmxnet3/vmxnet3_defs.h b/drivers/net/vmxnet3/vmxnet3_defs.h
index f3b31c2..a0fdaac 100644
--- a/drivers/net/vmxnet3/vmxnet3_defs.h
+++ b/drivers/net/vmxnet3/vmxnet3_defs.h
@@ -80,6 +80,7 @@ enum {
 	VMXNET3_CMD_LOAD_PLUGIN,
 	VMXNET3_CMD_RESERVED2,
 	VMXNET3_CMD_RESERVED3,
+	VMXNET3_CMD_SET_COALESCE,
 
 	VMXNET3_CMD_FIRST_GET = 0xF00D0000,
 	VMXNET3_CMD_GET_QUEUE_STATUS = VMXNET3_CMD_FIRST_GET,
@@ -637,6 +638,36 @@ struct Vmxnet3_SetPolling {
 	u8					enablePolling;
 };
 
+#define VMXNET3_COAL_STATIC_MAX_DEPTH		128
+#define VMXNET3_COAL_RBC_MIN_RATE		100
+#define VMXNET3_COAL_RBC_MAX_RATE		100000
+
+enum Vmxnet3_CoalesceMode {
+	VMXNET3_COALESCE_DEFAULT    = 0,
+	VMXNET3_COALESCE_DISABLED   = 1,
+	VMXNET3_COALESCE_ADAPT      = 2,
+	VMXNET3_COALESCE_STATIC     = 3,
+	VMXNET3_COALESCE_RBC        = 4
+};
+
+struct Vmxnet3_CoalesceRbc {
+	u32					rbc_rate;
+};
+
+struct Vmxnet3_CoalesceStatic {
+	u32					tx_depth;
+	u32					tx_comp_depth;
+	u32					rx_depth;
+};
+
+struct Vmxnet3_CoalesceScheme {
+	enum Vmxnet3_CoalesceMode		coalMode;
+	union {
+		struct Vmxnet3_CoalesceRbc	coalRbc;
+		struct Vmxnet3_CoalesceStatic	coalStatic;
+	} coalPara;
+};
+
 /* If the command data <= 16 bytes, use the shared memory directly.
  * otherwise, use variable length configuration descriptor.
  */
@@ -673,7 +704,6 @@ struct Vmxnet3_DriverShared {
 	} cu;
 };
 
-
 #define VMXNET3_ECR_RQERR       (1 << 0)
 #define VMXNET3_ECR_TQERR       (1 << 1)
 #define VMXNET3_ECR_LINK        (1 << 2)
diff --git a/drivers/net/vmxnet3/vmxnet3_drv.c b/drivers/net/vmxnet3/vmxnet3_drv.c
index a6dc7c7..fe1c6ad 100644
--- a/drivers/net/vmxnet3/vmxnet3_drv.c
+++ b/drivers/net/vmxnet3/vmxnet3_drv.c
@@ -2491,6 +2491,26 @@ vmxnet3_setup_driver_shared(struct vmxnet3_adapter *adapter)
 	/* the rest are already zeroed */
 }
 
+static void
+vmxnet3_init_coalesce(struct vmxnet3_adapter *adapter)
+{
+	struct Vmxnet3_DriverShared *shared = adapter->shared;
+	union Vmxnet3_CmdInfo *cmdInfo = &shared->cu.cmdInfo;
+	unsigned long flags;
+
+	if (!VMXNET3_VERSION_GE_3(adapter) ||
+	    adapter->coal_conf->coalMode == VMXNET3_COALESCE_DEFAULT) {
+		return;
+	}
+
+	spin_lock_irqsave(&adapter->cmd_lock, flags);
+	cmdInfo->varConf.confVer = 1;
+	cmdInfo->varConf.confLen = cpu_to_le32(sizeof(*adapter->coal_conf));
+	cmdInfo->varConf.confPA  = cpu_to_le64(adapter->coal_conf_pa);
+	VMXNET3_WRITE_BAR1_REG(adapter, VMXNET3_REG_CMD,
+			       VMXNET3_CMD_SET_COALESCE);
+	spin_unlock_irqrestore(&adapter->cmd_lock, flags);
+}
 
 int
 vmxnet3_activate_dev(struct vmxnet3_adapter *adapter)
@@ -2540,6 +2560,8 @@ vmxnet3_activate_dev(struct vmxnet3_adapter *adapter)
 		goto activate_err;
 	}
 
+	vmxnet3_init_coalesce(adapter);
+
 	for (i = 0; i < adapter->num_rx_queues; i++) {
 		VMXNET3_WRITE_BAR0_REG(adapter,
 				VMXNET3_REG_RXPROD + i * VMXNET3_REG_ALIGN,
@@ -3345,6 +3367,21 @@ vmxnet3_probe_device(struct pci_dev *pdev,
 		goto err_ver;
 	}
 
+	if (VMXNET3_VERSION_GE_3(adapter)) {
+		adapter->coal_conf =
+			dma_alloc_coherent(&adapter->pdev->dev,
+					   sizeof(struct Vmxnet3_CoalesceScheme)
+					   ,
+					   &adapter->coal_conf_pa,
+					   GFP_KERNEL);
+		if (!adapter->coal_conf) {
+			err = -ENOMEM;
+			goto err_ver;
+		}
+		memset(adapter->coal_conf, 0, sizeof(*adapter->coal_conf));
+		adapter->coal_conf->coalMode = VMXNET3_COALESCE_DEFAULT;
+	}
+
 	SET_NETDEV_DEV(netdev, &pdev->dev);
 	vmxnet3_declare_features(adapter, dma64);
 
@@ -3407,6 +3444,11 @@ vmxnet3_probe_device(struct pci_dev *pdev,
 	return 0;
 
 err_register:
+	if (VMXNET3_VERSION_GE_3(adapter)) {
+		dma_free_coherent(&adapter->pdev->dev,
+				  sizeof(struct Vmxnet3_CoalesceScheme),
+				  adapter->coal_conf, adapter->coal_conf_pa);
+	}
 	vmxnet3_free_intr_resources(adapter);
 err_ver:
 	vmxnet3_free_pci_resources(adapter);
@@ -3457,6 +3499,11 @@ vmxnet3_remove_device(struct pci_dev *pdev)
 
 	vmxnet3_free_intr_resources(adapter);
 	vmxnet3_free_pci_resources(adapter);
+	if (VMXNET3_VERSION_GE_3(adapter)) {
+		dma_free_coherent(&adapter->pdev->dev,
+				  sizeof(struct Vmxnet3_CoalesceScheme),
+				  adapter->coal_conf, adapter->coal_conf_pa);
+	}
 #ifdef VMXNET3_RSS
 	dma_free_coherent(&adapter->pdev->dev, sizeof(struct UPT1_RSSConf),
 			  adapter->rss_conf, adapter->rss_conf_pa);
diff --git a/drivers/net/vmxnet3/vmxnet3_ethtool.c b/drivers/net/vmxnet3/vmxnet3_ethtool.c
index 38f7c79..fc52312 100644
--- a/drivers/net/vmxnet3/vmxnet3_ethtool.c
+++ b/drivers/net/vmxnet3/vmxnet3_ethtool.c
@@ -725,6 +725,139 @@ vmxnet3_set_rss(struct net_device *netdev, const u32 *p, const u8 *key,
 }
 #endif
 
+static int
+vmxnet3_get_coalesce(struct net_device *netdev, struct ethtool_coalesce *ec)
+{
+	struct vmxnet3_adapter *adapter = netdev_priv(netdev);
+
+	if (!VMXNET3_VERSION_GE_3(adapter))
+		return -EOPNOTSUPP;
+
+	switch (adapter->coal_conf->coalMode) {
+	case VMXNET3_COALESCE_DEFAULT:
+	case VMXNET3_COALESCE_DISABLED:
+	case VMXNET3_COALESCE_ADAPT:
+	case VMXNET3_COALESCE_STATIC:
+		ec->rx_coalesce_usecs = adapter->coal_conf->coalMode;
+		break;
+	case VMXNET3_COALESCE_RBC:
+		ec->rx_coalesce_usecs =
+			adapter->coal_conf->coalPara.coalRbc.rbc_rate;
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	return 0;
+}
+
+static int
+vmxnet3_set_coalesce(struct net_device *netdev, struct ethtool_coalesce *ec)
+{
+	struct vmxnet3_adapter *adapter = netdev_priv(netdev);
+	struct Vmxnet3_DriverShared *shared = adapter->shared;
+	union Vmxnet3_CmdInfo *cmdInfo = &shared->cu.cmdInfo;
+	unsigned long flags;
+
+	if (!VMXNET3_VERSION_GE_3(adapter))
+		return -EOPNOTSUPP;
+
+	if (ec->rx_max_coalesced_frames ||
+	    ec->rx_coalesce_usecs_irq ||
+	    ec->tx_coalesce_usecs ||
+	    ec->tx_coalesce_usecs_irq ||
+	    ec->stats_block_coalesce_usecs ||
+	    ec->use_adaptive_rx_coalesce ||
+	    ec->use_adaptive_tx_coalesce ||
+	    ec->pkt_rate_low ||
+	    ec->rx_coalesce_usecs_low ||
+	    ec->rx_max_coalesced_frames_low ||
+	    ec->tx_coalesce_usecs_low ||
+	    ec->tx_max_coalesced_frames_low ||
+	    ec->pkt_rate_high ||
+	    ec->rx_coalesce_usecs_high ||
+	    ec->rx_max_coalesced_frames_high ||
+	    ec->tx_coalesce_usecs_high ||
+	    ec->tx_max_coalesced_frames_high) {
+		return -EINVAL;
+	}
+
+	switch (ec->rx_coalesce_usecs) {
+	case VMXNET3_COALESCE_DEFAULT:
+	case VMXNET3_COALESCE_DISABLED:
+	case VMXNET3_COALESCE_ADAPT:
+		if (ec->tx_max_coalesced_frames ||
+		    ec->tx_max_coalesced_frames_irq ||
+		    ec->rx_max_coalesced_frames_irq) {
+			return -EINVAL;
+		}
+		memset(adapter->coal_conf, 0, sizeof(*adapter->coal_conf));
+		adapter->coal_conf->coalMode = ec->rx_coalesce_usecs;
+		break;
+	case VMXNET3_COALESCE_STATIC:
+		if (ec->tx_max_coalesced_frames >
+		    VMXNET3_COAL_STATIC_MAX_DEPTH) {
+			return -EINVAL;
+		}
+
+		if (ec->tx_max_coalesced_frames_irq >
+		    VMXNET3_COAL_STATIC_MAX_DEPTH) {
+			return -EINVAL;
+		}
+
+		if (ec->rx_max_coalesced_frames_irq >
+		    VMXNET3_COAL_STATIC_MAX_DEPTH) {
+			return -EINVAL;
+		}
+
+		memset(adapter->coal_conf, 0, sizeof(*adapter->coal_conf));
+		adapter->coal_conf->coalMode = ec->rx_coalesce_usecs;
+
+		adapter->coal_conf->coalPara.coalStatic.tx_depth =
+			(ec->tx_max_coalesced_frames ?
+			 ec->tx_max_coalesced_frames :
+			 VMXNET3_COAL_STATIC_DEFAULT_DEPTH);
+
+		adapter->coal_conf->coalPara.coalStatic.tx_comp_depth =
+			(ec->tx_max_coalesced_frames_irq ?
+			 ec->tx_max_coalesced_frames_irq :
+			 VMXNET3_COAL_STATIC_DEFAULT_DEPTH);
+
+		adapter->coal_conf->coalPara.coalStatic.rx_depth =
+			(ec->rx_max_coalesced_frames_irq ?
+			 ec->rx_max_coalesced_frames_irq :
+			 VMXNET3_COAL_STATIC_DEFAULT_DEPTH);
+		break;
+	case VMXNET3_COAL_RBC_MIN_RATE ... VMXNET3_COAL_RBC_MAX_RATE:
+		if (ec->tx_max_coalesced_frames ||
+		    ec->tx_max_coalesced_frames_irq ||
+		    ec->rx_max_coalesced_frames_irq) {
+			return -EINVAL;
+		}
+		memset(adapter->coal_conf, 0, sizeof(*adapter->coal_conf));
+		adapter->coal_conf->coalMode = VMXNET3_COALESCE_RBC;
+
+		adapter->coal_conf->coalPara.coalRbc.rbc_rate =
+			ec->rx_coalesce_usecs;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	if (netif_running(netdev)) {
+		spin_lock_irqsave(&adapter->cmd_lock, flags);
+		cmdInfo->varConf.confVer = 1;
+		cmdInfo->varConf.confLen =
+			cpu_to_le32(sizeof(*adapter->coal_conf));
+		cmdInfo->varConf.confPA  = cpu_to_le64(adapter->coal_conf_pa);
+		VMXNET3_WRITE_BAR1_REG(adapter, VMXNET3_REG_CMD,
+				       VMXNET3_CMD_SET_COALESCE);
+		spin_unlock_irqrestore(&adapter->cmd_lock, flags);
+	}
+
+	return 0;
+}
+
 static const struct ethtool_ops vmxnet3_ethtool_ops = {
 	.get_settings      = vmxnet3_get_settings,
 	.get_drvinfo       = vmxnet3_get_drvinfo,
@@ -733,6 +866,8 @@ static const struct ethtool_ops vmxnet3_ethtool_ops = {
 	.get_wol           = vmxnet3_get_wol,
 	.set_wol           = vmxnet3_set_wol,
 	.get_link          = ethtool_op_get_link,
+	.get_coalesce      = vmxnet3_get_coalesce,
+	.set_coalesce      = vmxnet3_set_coalesce,
 	.get_strings       = vmxnet3_get_strings,
 	.get_sset_count	   = vmxnet3_get_sset_count,
 	.get_ethtool_stats = vmxnet3_get_ethtool_stats,
diff --git a/drivers/net/vmxnet3/vmxnet3_int.h b/drivers/net/vmxnet3/vmxnet3_int.h
index 72ef5a3..8cd1851 100644
--- a/drivers/net/vmxnet3/vmxnet3_int.h
+++ b/drivers/net/vmxnet3/vmxnet3_int.h
@@ -358,6 +358,7 @@ struct vmxnet3_adapter {
 	int		rx_buf_per_pkt;  /* only apply to the 1st ring */
 	dma_addr_t			shared_pa;
 	dma_addr_t queue_desc_pa;
+	dma_addr_t coal_conf_pa;
 
 	/* Wake-on-LAN */
 	u32     wol;
@@ -384,6 +385,8 @@ struct vmxnet3_adapter {
 
 	int share_intr;
 
+	struct Vmxnet3_CoalesceScheme *coal_conf;
+
 	dma_addr_t adapter_pa;
 	dma_addr_t pm_conf_pa;
 	dma_addr_t rss_conf_pa;
@@ -429,6 +432,8 @@ struct vmxnet3_adapter {
 	(rqID >= 2 * adapter->num_rx_queues &&		\
 	rqID < 3 * adapter->num_rx_queues)
 
+#define VMXNET3_COAL_STATIC_DEFAULT_DEPTH   64
+
 int
 vmxnet3_quiesce_dev(struct vmxnet3_adapter *adapter);
 
-- 
1.9.1

[toc] | [next] | [standalone]

#1396302 — Re: [PATCH net-next 5/7] Driver: Vmxnet3: Add support for get_coalesce, set_coalesce ethtool operations

[Multipart message — attachments visible in raw view] — view raw

On Fri, 2016-05-06 at 16:12 -0700, Shrikrishna Khare wrote:
[...]
> +static int
> +vmxnet3_set_coalesce(struct net_device *netdev, struct ethtool_coalesce *ec)
> +{
[...]
> +	switch (ec->rx_coalesce_usecs) {
> +	case VMXNET3_COALESCE_DEFAULT:
> +	case VMXNET3_COALESCE_DISABLED:
> +	case VMXNET3_COALESCE_ADAPT:
> +		if (ec->tx_max_coalesced_frames ||
> +		    ec->tx_max_coalesced_frames_irq ||
> +		    ec->rx_max_coalesced_frames_irq) {
> +			return -EINVAL;
> +		}
> +		memset(adapter->coal_conf, 0, sizeof(*adapter->coal_conf));
> +		adapter->coal_conf->coalMode = ec->rx_coalesce_usecs;
> +		break;
> +	case VMXNET3_COALESCE_STATIC:
[...]

I don't want to see drivers introducing magic values for fields that
are denominated in microseconds (especially not for 0, which is the
correct way to specify 'no coalescing').  If the current
ethtool_coalesce structure is inadequate, propose an extension.

Ben.

-- 
Ben Hutchings
Editing code like this is akin to sticking plasters on the bleeding stump
of a severed limb. - me, 29 June 1999

[toc] | [prev] | [next] | [standalone]

#1396331 — Re: [PATCH net-next 5/7] Driver: Vmxnet3: Add support for get_coalesce, set_coalesce ethtool operations

From: Ben Hutchings <ben@decadent.org.uk>
Date: Sat, 07 May 2016 13:04:46 +0100

> On Fri, 2016-05-06 at 16:12 -0700, Shrikrishna Khare wrote:
> [...]
>> +static int
>> +vmxnet3_set_coalesce(struct net_device *netdev, struct ethtool_coalesce *ec)
>> +{
> [...]
>> +	switch (ec->rx_coalesce_usecs) {
>> +	case VMXNET3_COALESCE_DEFAULT:
>> +	case VMXNET3_COALESCE_DISABLED:
>> +	case VMXNET3_COALESCE_ADAPT:
>> +		if (ec->tx_max_coalesced_frames ||
>> +		    ec->tx_max_coalesced_frames_irq ||
>> +		    ec->rx_max_coalesced_frames_irq) {
>> +			return -EINVAL;
>> +		}
>> +		memset(adapter->coal_conf, 0, sizeof(*adapter->coal_conf));
>> +		adapter->coal_conf->coalMode = ec->rx_coalesce_usecs;
>> +		break;
>> +	case VMXNET3_COALESCE_STATIC:
> [...]
> 
> I don't want to see drivers introducing magic values for fields that
> are denominated in microseconds (especially not for 0, which is the
> correct way to specify 'no coalescing').  If the current
> ethtool_coalesce structure is inadequate, propose an extension.

Agreed.

[toc] | [prev] | [next] | [standalone]

#1396488 — Re: [PATCH net-next 5/7] Driver: Vmxnet3: Add support for get_coalesce, set_coalesce ethtool operations

[Multipart message — attachments visible in raw view] — view raw

On Sat, 7 May 2016, Ben Hutchings wrote:

> On Fri, 2016-05-06 at 16:12 -0700, Shrikrishna Khare wrote:
> [...]
> > +static int
> > +vmxnet3_set_coalesce(struct net_device *netdev, struct ethtool_coalesce *ec)
> > +{
> [...]
> > +	switch (ec->rx_coalesce_usecs) {
> > +	case VMXNET3_COALESCE_DEFAULT:
> > +	case VMXNET3_COALESCE_DISABLED:
> > +	case VMXNET3_COALESCE_ADAPT:
> > +		if (ec->tx_max_coalesced_frames ||
> > +		    ec->tx_max_coalesced_frames_irq ||
> > +		    ec->rx_max_coalesced_frames_irq) {
> > +			return -EINVAL;
> > +		}
> > +		memset(adapter->coal_conf, 0, sizeof(*adapter->coal_conf));
> > +		adapter->coal_conf->coalMode = ec->rx_coalesce_usecs;
> > +		break;
> > +	case VMXNET3_COALESCE_STATIC:
> [...]
> 
> I don't want to see drivers introducing magic values for fields that
> are denominated in microseconds (especially not for 0, which is the
> correct way to specify 'no coalescing').  If the current
> ethtool_coalesce structure is inadequate, propose an extension.

For vmxnet3, we need an ethtool mechanism to indicate coalescing mode to 
the device.

Would a patch that maps 0 to 'no coalescing' be acceptable? That is:

rx-usecs = 0 -> coalescing disabled.
rx-usecs = 1 -> default (chosen by the device).
rx-usecs = 2 -> adaptive coalescing.
rx-usecs = 3 -> static coalescing.
all other rx-usecs values -> rate based coalescing where rx-usecs denotes 
rate.

Alternatively: I don't think new members could be added to struct 
ethtool_coalesce without breaking compatibility. Thus, I could extend 
coalescing as follows:
- new struct ethtool_coalesce_v2 with coalesce_mode (along with all the 
members of struct ethtool_coalesce).
- introduce new ETHTOOL_{G,S}COALESCE_V2 commands.
- extend userspace ethtool to invoke new commands.

Could you please advice?

Thanks,
Shri

[toc] | [prev] | [next] | [standalone]

#1396520 — Re: [PATCH net-next 5/7] Driver: Vmxnet3: Add support for get_coalesce, set_coalesce ethtool operations

[Multipart message — attachments visible in raw view] — view raw

On Sun, 2016-05-08 at 13:55 -0700, Shrikrishna Khare wrote:
> 
> On Sat, 7 May 2016, Ben Hutchings wrote:
> 
> > On Fri, 2016-05-06 at 16:12 -0700, Shrikrishna Khare wrote:
> > [...]
> > > +static int
> > > +vmxnet3_set_coalesce(struct net_device *netdev, struct ethtool_coalesce *ec)
> > > +{
> > [...]
> > > +   switch (ec->rx_coalesce_usecs) {
> > > +   case VMXNET3_COALESCE_DEFAULT:
> > > +   case VMXNET3_COALESCE_DISABLED:
> > > +   case VMXNET3_COALESCE_ADAPT:
> > > +           if (ec->tx_max_coalesced_frames ||
> > > +               ec->tx_max_coalesced_frames_irq ||
> > > +               ec->rx_max_coalesced_frames_irq) {
> > > +                   return -EINVAL;
> > > +           }
> > > +           memset(adapter->coal_conf, 0, sizeof(*adapter->coal_conf));
> > > +           adapter->coal_conf->coalMode = ec->rx_coalesce_usecs;
> > > +           break;
> > > +   case VMXNET3_COALESCE_STATIC:
> > [...]
> > 
> > I don't want to see drivers introducing magic values for fields that
> > are denominated in microseconds (especially not for 0, which is the
> > correct way to specify 'no coalescing').  If the current
> > ethtool_coalesce structure is inadequate, propose an extension.
> 
> For vmxnet3, we need an ethtool mechanism to indicate coalescing mode to 
> the device.
> 
> Would a patch that maps 0 to 'no coalescing' be acceptable? That is:
> 
> rx-usecs = 0 -> coalescing disabled.
> rx-usecs = 1 -> default (chosen by the device).
> rx-usecs = 2 -> adaptive coalescing.
> rx-usecs = 3 -> static coalescing.

I still don't like it much.  For the 3 special values (0 isn't really
special):

1 = default: When the driver sets the virtual device to this mode, can it then read back what the actual settings are, or are they hidden?  If it can, then userland can also read the defaults and explicitly return to them later.  But I do see the usefulness of an explicit request to reset to defaults.

2 = adaptive coalescing: There are already fields to request adaptive coalescing; you should support them.

3 = static coalescing: I don't understand what this means.

> all other rx-usecs values -> rate based coalescing where rx-usecs denotes 
> rate.
> 
> Alternatively: I don't think new members could be added to struct 
> ethtool_coalesce without breaking compatibility.

That's right, unfortunately.

> Thus, I could extend coalescing as follows:
> - new struct ethtool_coalesce_v2 with coalesce_mode (along with all the 
> members of struct ethtool_coalesce).
> - introduce new ETHTOOL_{G,S}COALESCE_V2 commands.
> - extend userspace ethtool to invoke new commands.
> 
> Could you please advice?

That's roughly how you would extend it.  Though we would probably want
to consider making other extensions to interrupt coalescing control at
the same time.

Ben.

>  
Ben Hutchings
If you seem to know what you are doing, you'll be given more to do.

[toc] | [prev] | [next] | [standalone]

#1398000 — RE: [PATCH net-next 5/7] Driver: Vmxnet3: Add support for get_coalesce, set_coalesce ethtool operations

From: Ben Hutchings
> Sent: 09 May 2016 01:17
> On Sun, 2016-05-08 at 13:55 -0700, Shrikrishna Khare wrote:
> >
> > On Sat, 7 May 2016, Ben Hutchings wrote:
> >
> > > On Fri, 2016-05-06 at 16:12 -0700, Shrikrishna Khare wrote:
> > > [...]
> > > > +static int
> > > > +vmxnet3_set_coalesce(struct net_device *netdev, struct ethtool_coalesce *ec)
> > > > +{
> > > [...]
> > > > +   switch (ec->rx_coalesce_usecs) {
> > > > +   case VMXNET3_COALESCE_DEFAULT:
> > > > +   case VMXNET3_COALESCE_DISABLED:
> > > > +   case VMXNET3_COALESCE_ADAPT:
> > > > +           if (ec->tx_max_coalesced_frames ||
> > > > +               ec->tx_max_coalesced_frames_irq ||
> > > > +               ec->rx_max_coalesced_frames_irq) {
> > > > +                   return -EINVAL;
> > > > +           }
> > > > +           memset(adapter->coal_conf, 0, sizeof(*adapter->coal_conf));
> > > > +           adapter->coal_conf->coalMode = ec->rx_coalesce_usecs;
> > > > +           break;
> > > > +   case VMXNET3_COALESCE_STATIC:
> > > [...]
> > >
> > > I don't want to see drivers introducing magic values for fields that
> > > are denominated in microseconds (especially not for 0, which is the
> > > correct way to specify 'no coalescing').  If the current
> > > ethtool_coalesce structure is inadequate, propose an extension.
> >
> > For vmxnet3, we need an ethtool mechanism to indicate coalescing mode to
> > the device.
> >
> > Would a patch that maps 0 to 'no coalescing' be acceptable? That is:
> >
> > rx-usecs = 0 -> coalescing disabled.
> > rx-usecs = 1 -> default (chosen by the device).
> > rx-usecs = 2 -> adaptive coalescing.
> > rx-usecs = 3 -> static coalescing.

Would it be better to use stupidly large values for the non-zero special values?
That would less problematic if someone expects 2 to mean '2 usecs'.

	David

[toc] | [prev] | [next] | [standalone]

#1398033 — Re: [PATCH net-next 5/7] Driver: Vmxnet3: Add support for get_coalesce, set_coalesce ethtool operations

[Multipart message — attachments visible in raw view] — view raw

On Tue, 2016-05-10 at 11:24 +0000, David Laight wrote:
> From: Ben Hutchings
> > 
> > Sent: 09 May 2016 01:17
> > On Sun, 2016-05-08 at 13:55 -0700, Shrikrishna Khare wrote:
> > > 
> > > 
> > > On Sat, 7 May 2016, Ben Hutchings wrote:
> > > 
> > > > 
> > > > On Fri, 2016-05-06 at 16:12 -0700, Shrikrishna Khare wrote:
> > > > [...]
> > > > > 
> > > > > +static int
> > > > > +vmxnet3_set_coalesce(struct net_device *netdev, struct ethtool_coalesce *ec)
> > > > > +{
> > > > [...]
> > > > > 
> > > > > +   switch (ec->rx_coalesce_usecs) {
> > > > > +   case VMXNET3_COALESCE_DEFAULT:
> > > > > +   case VMXNET3_COALESCE_DISABLED:
> > > > > +   case VMXNET3_COALESCE_ADAPT:
> > > > > +           if (ec->tx_max_coalesced_frames ||
> > > > > +               ec->tx_max_coalesced_frames_irq ||
> > > > > +               ec->rx_max_coalesced_frames_irq) {
> > > > > +                   return -EINVAL;
> > > > > +           }
> > > > > +           memset(adapter->coal_conf, 0, sizeof(*adapter->coal_conf));
> > > > > +           adapter->coal_conf->coalMode = ec->rx_coalesce_usecs;
> > > > > +           break;
> > > > > +   case VMXNET3_COALESCE_STATIC:
> > > > [...]
> > > > 
> > > > I don't want to see drivers introducing magic values for fields that
> > > > are denominated in microseconds (especially not for 0, which is the
> > > > correct way to specify 'no coalescing').  If the current
> > > > ethtool_coalesce structure is inadequate, propose an extension.
> > > For vmxnet3, we need an ethtool mechanism to indicate coalescing mode to
> > > the device.
> > > 
> > > Would a patch that maps 0 to 'no coalescing' be acceptable? That is:
> > > 
> > > rx-usecs = 0 -> coalescing disabled.
> > > rx-usecs = 1 -> default (chosen by the device).
> > > rx-usecs = 2 -> adaptive coalescing.
> > > rx-usecs = 3 -> static coalescing.
> Would it be better to use stupidly large values for the non-zero special values?
> That would less problematic if someone expects 2 to mean '2 usecs'.

That seems less problematic, but I think we'd have to check that
current drivers (or at least the most widely used ones) reject those
values.

We should still need to standardise names and definitions for those
special values, so that the ethtool utility can provide keywords and
documentation for them.

Ben.

-- 
Ben Hutchings
If you seem to know what you are doing, you'll be given more to do.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web