[PATCH net-next v3 0/3] enic: Add support for rx_copybreak

[PATCH net-next v3 0/3] enic: Add support for rx_copybreak

[Govindarajulu Varadarajan]

The following series implements rx_copybreak.

dma_map_single()/dma_unmap_single() is more expensive than alloc_skb & memcpy
for smaller packets. By doing this we can reuse the dma buff which is already
mapped. This is very useful when iommu is on. The default skb copybreak value
is 256.

When iommu is on, we can go much higher than 256. All the drivers that supports
rx_copybreak provides module parameter to change this value. Since module
parameter is the least preferred way for changing driver values, this series
adds ethtool support for setting rx_copybreak.

v3:
Add tunable namespace to ethtool. Use new ethtool cmd ETHTOOL_S/GTUNABLE to
set/get rx_copybreak from userspace.

v2:
Add new ethtool_cmd for DMA buffer parameters, instead of adding new members to
existing ethtool_ringparam.

Govindarajulu Varadarajan (3):
  enic: implement rx_copybreak
  ethtool: Add generic options for tunables
  enic: Add tunable_ops support for rx_copybreak

 drivers/net/ethernet/cisco/enic/enic.h         |  1 +
 drivers/net/ethernet/cisco/enic/enic_ethtool.c | 30 +++++++++++++++
 drivers/net/ethernet/cisco/enic/enic_main.c    | 50 +++++++++++++++++++++++--
 include/linux/ethtool.h                        |  6 +++
 include/uapi/linux/ethtool.h                   | 17 +++++++++
 net/core/ethtool.c                             | 51 ++++++++++++++++++++++++++
 6 files changed, 152 insertions(+), 3 deletions(-)

-- 
2.0.4

[PATCH net-next v3 1/3] enic: implement rx_copybreak

[Govindarajulu Varadarajan]

Calling dma_map_single()/dma_unmap_single() is quite expensive compared
to copying a small packet. So let's copy short frames and keep the buffers
mapped.

Signed-off-by: Govindarajulu Varadarajan <_govind@gmx.com>
---
 drivers/net/ethernet/cisco/enic/enic.h      |  1 +
 drivers/net/ethernet/cisco/enic/enic_main.c | 50 +++++++++++++++++++++++++++--
 2 files changed, 48 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/cisco/enic/enic.h b/drivers/net/ethernet/cisco/enic/enic.h
index 962510f..5ba5ad0 100644
--- a/drivers/net/ethernet/cisco/enic/enic.h
+++ b/drivers/net/ethernet/cisco/enic/enic.h
@@ -186,6 +186,7 @@ struct enic {
 	____cacheline_aligned struct vnic_cq cq[ENIC_CQ_MAX];
 	unsigned int cq_count;
 	struct enic_rfs_flw_tbl rfs_h;
+	u32 rx_copybreak;
 };
 
 static inline struct device *enic_get_dev(struct enic *enic)
diff --git a/drivers/net/ethernet/cisco/enic/enic_main.c b/drivers/net/ethernet/cisco/enic/enic_main.c
index 9348feb..b7e69fd 100644
--- a/drivers/net/ethernet/cisco/enic/enic_main.c
+++ b/drivers/net/ethernet/cisco/enic/enic_main.c
@@ -66,6 +66,8 @@
 #define PCI_DEVICE_ID_CISCO_VIC_ENET_DYN     0x0044  /* enet dynamic vnic */
 #define PCI_DEVICE_ID_CISCO_VIC_ENET_VF      0x0071  /* enet SRIOV VF */
 
+#define RX_COPYBREAK_DEFAULT		256
+
 /* Supported devices */
 static DEFINE_PCI_DEVICE_TABLE(enic_id_table) = {
 	{ PCI_VDEVICE(CISCO, PCI_DEVICE_ID_CISCO_VIC_ENET) },
@@ -924,6 +926,7 @@ static void enic_free_rq_buf(struct vnic_rq *rq, struct vnic_rq_buf *buf)
 	pci_unmap_single(enic->pdev, buf->dma_addr,
 		buf->len, PCI_DMA_FROMDEVICE);
 	dev_kfree_skb_any(buf->os_buf);
+	buf->os_buf = NULL;
 }
 
 static int enic_rq_alloc_buf(struct vnic_rq *rq)
@@ -934,7 +937,24 @@ static int enic_rq_alloc_buf(struct vnic_rq *rq)
 	unsigned int len = netdev->mtu + VLAN_ETH_HLEN;
 	unsigned int os_buf_index = 0;
 	dma_addr_t dma_addr;
+	struct vnic_rq_buf *buf = rq->to_use;
+
+	if (buf->os_buf) {
+		buf = buf->next;
+		rq->to_use = buf;
+		rq->ring.desc_avail--;
+		if ((buf->index & VNIC_RQ_RETURN_RATE) == 0) {
+			/* Adding write memory barrier prevents compiler and/or
+			 * CPU reordering, thus avoiding descriptor posting
+			 * before descriptor is initialized. Otherwise, hardware
+			 * can read stale descriptor fields.
+			 */
+			wmb();
+			iowrite32(buf->index, &rq->ctrl->posted_index);
+		}
 
+		return 0;
+	}
 	skb = netdev_alloc_skb_ip_align(netdev, len);
 	if (!skb)
 		return -ENOMEM;
@@ -957,6 +977,25 @@ static void enic_intr_update_pkt_size(struct vnic_rx_bytes_counter *pkt_size,
 		pkt_size->small_pkt_bytes_cnt += pkt_len;
 }
 
+static bool enic_rxcopybreak(struct net_device *netdev, struct sk_buff **skb,
+			     struct vnic_rq_buf *buf, u16 len)
+{
+	struct enic *enic = netdev_priv(netdev);
+	struct sk_buff *new_skb;
+
+	if (len > enic->rx_copybreak)
+		return false;
+	new_skb = netdev_alloc_skb_ip_align(netdev, len);
+	if (!new_skb)
+		return false;
+	pci_dma_sync_single_for_cpu(enic->pdev, buf->dma_addr, len,
+				    DMA_FROM_DEVICE);
+	memcpy(new_skb->data, (*skb)->data, len);
+	*skb = new_skb;
+
+	return true;
+}
+
 static void enic_rq_indicate_buf(struct vnic_rq *rq,
 	struct cq_desc *cq_desc, struct vnic_rq_buf *buf,
 	int skipped, void *opaque)
@@ -978,9 +1017,6 @@ static void enic_rq_indicate_buf(struct vnic_rq *rq,
 		return;
 
 	skb = buf->os_buf;
-	prefetch(skb->data - NET_IP_ALIGN);
-	pci_unmap_single(enic->pdev, buf->dma_addr,
-		buf->len, PCI_DMA_FROMDEVICE);
 
 	cq_enet_rq_desc_dec((struct cq_enet_rq_desc *)cq_desc,
 		&type, &color, &q_number, &completed_index,
@@ -1011,6 +1047,13 @@ static void enic_rq_indicate_buf(struct vnic_rq *rq,
 		/* Good receive
 		 */
 
+		if (!enic_rxcopybreak(netdev, &skb, buf, bytes_written)) {
+			buf->os_buf = NULL;
+			pci_unmap_single(enic->pdev, buf->dma_addr, buf->len,
+					 PCI_DMA_FROMDEVICE);
+		}
+		prefetch(skb->data - NET_IP_ALIGN);
+
 		skb_put(skb, bytes_written);
 		skb->protocol = eth_type_trans(skb, netdev);
 		skb_record_rx_queue(skb, q_number);
@@ -2531,6 +2574,7 @@ static int enic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 		dev_err(dev, "Cannot register net device, aborting\n");
 		goto err_out_dev_deinit;
 	}
+	enic->rx_copybreak = RX_COPYBREAK_DEFAULT;
 
 	return 0;
 
-- 
2.0.4

[PATCH net-next v3 2/3] ethtool: Add generic options for tunables

[Govindarajulu Varadarajan]

This patch adds new ethtool cmd, ETHTOOL_GTUNABLE & ETHTOOL_STUNABLE for getting
tunable values from driver.

Add ethtool_tunable_ops array to ethtool_ops. The array index will be
the tunable tcmd as defined in enum tunable_cmd. ethtool_tunable_ops has two
function pointers set & get. This is set by the driver for getting/setting
particular tunable.

Signed-off-by: Govindarajulu Varadarajan <_govind@gmx.com>
---
 include/linux/ethtool.h      |  6 ++++++
 include/uapi/linux/ethtool.h | 17 +++++++++++++++
 net/core/ethtool.c           | 51 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 74 insertions(+)

diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index e658229..33bff94 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -77,6 +77,11 @@ static inline u32 ethtool_rxfh_indir_default(u32 index, u32 n_rx_rings)
 	return index % n_rx_rings;
 }
 
+struct ethtool_tunable_ops {
+	int (*set)(struct net_device *, struct ethtool_tunable *);
+	int (*get)(struct net_device *, struct ethtool_tunable *);
+};
+
 /**
  * struct ethtool_ops - optional netdev operations
  * @get_settings: Get various device settings including Ethernet link
@@ -257,6 +262,7 @@ struct ethtool_ops {
 				     struct ethtool_eeprom *, u8 *);
 	int	(*get_eee)(struct net_device *, struct ethtool_eee *);
 	int	(*set_eee)(struct net_device *, struct ethtool_eee *);
+	struct ethtool_tunable_ops tunable_ops[ETHTOOL_TUNABLE_MAX];
 
 
 };
diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
index e3c7a71..99e43ca 100644
--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h
@@ -209,6 +209,21 @@ struct ethtool_value {
 	__u32	data;
 };
 
+enum tunable_cmd {
+	ETHTOOL_RX_COPYBREAK = 0,
+	ETHTOOL_TUNABLE_MAX,
+};
+
+struct ethtool_tunable {
+	u32 cmd;
+	u32 tcmd;
+	u32 len;
+	union {
+		u32 rx_copybreak;
+		u32 data[48];
+	} data;
+};
+
 /**
  * struct ethtool_regs - hardware register dump
  * @cmd: Command number = %ETHTOOL_GREGS
@@ -1152,6 +1167,8 @@ enum ethtool_sfeatures_retval_bits {
 
 #define ETHTOOL_GRSSH		0x00000046 /* Get RX flow hash configuration */
 #define ETHTOOL_SRSSH		0x00000047 /* Set RX flow hash configuration */
+#define ETHTOOL_GTUNABLE	0x00000048 /* Get Tunable  */
+#define ETHTOOL_STUNABLE	0x00000049 /* Set Tunable */
 
 /* compatibility with older code */
 #define SPARC_ETH_GSET		ETHTOOL_GSET
diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index 17cb912..0574c66 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -1621,6 +1621,50 @@ static int ethtool_get_module_eeprom(struct net_device *dev,
 				      modinfo.eeprom_len);
 }
 
+static int ethtool_get_tunable(struct net_device *dev, void __user *useraddr)
+{
+	int ret;
+	struct ethtool_tunable tuna;
+	const struct ethtool_tunable_ops *ops;
+
+	if (copy_from_user(&tuna, useraddr, sizeof(tuna)))
+		return -EFAULT;
+	if (tuna.tcmd >= ETHTOOL_TUNABLE_MAX)
+		return -EOPNOTSUPP;
+	ops = &dev->ethtool_ops->tunable_ops[tuna.tcmd];
+	if (!ops->get)
+		return -EOPNOTSUPP;
+	ret = ops->get(dev, &tuna);
+	if (ret)
+		return ret;
+	if (copy_to_user(useraddr, &tuna, sizeof(tuna)))
+		return -EFAULT;
+
+	return 0;
+}
+
+static int ethtool_set_tunable(struct net_device *dev, void __user *useraddr)
+{
+	int ret;
+	struct ethtool_tunable tuna;
+	const struct ethtool_tunable_ops *ops;
+
+	if (copy_from_user(&tuna, useraddr, sizeof(tuna)))
+		return -EFAULT;
+	if (tuna.tcmd >= ETHTOOL_TUNABLE_MAX)
+		return -EOPNOTSUPP;
+	ops = &dev->ethtool_ops->tunable_ops[tuna.tcmd];
+	if (!ops->set)
+		return -EOPNOTSUPP;
+	ret = ops->set(dev, &tuna);
+	if (ret)
+		return ret;
+	if (copy_to_user(useraddr, &tuna, sizeof(tuna)))
+		return -EFAULT;
+
+	return 0;
+}
+
 /* The main entry point in this file.  Called from net/core/dev_ioctl.c */
 
 int dev_ethtool(struct net *net, struct ifreq *ifr)
@@ -1670,6 +1714,7 @@ int dev_ethtool(struct net *net, struct ifreq *ifr)
 	case ETHTOOL_GCHANNELS:
 	case ETHTOOL_GET_TS_INFO:
 	case ETHTOOL_GEEE:
+	case ETHTOOL_GTUNABLE:
 		break;
 	default:
 		if (!ns_capable(net->user_ns, CAP_NET_ADMIN))
@@ -1857,6 +1902,12 @@ int dev_ethtool(struct net *net, struct ifreq *ifr)
 	case ETHTOOL_GMODULEEEPROM:
 		rc = ethtool_get_module_eeprom(dev, useraddr);
 		break;
+	case ETHTOOL_GTUNABLE:
+		rc = ethtool_get_tunable(dev, useraddr);
+		break;
+	case ETHTOOL_STUNABLE:
+		rc = ethtool_set_tunable(dev, useraddr);
+		break;
 	default:
 		rc = -EOPNOTSUPP;
 	}
-- 
2.0.4

[PATCH net-next v3 3/3] enic: Add tunable_ops support for rx_copybreak

[Govindarajulu Varadarajan]

This patch adds support for setting/getting rx_copybreak using
tunable_ops.

Defines enic_get_rx_copybreak() & enic_set_rx_copybreak() tunable_ops.

Signed-off-by: Govindarajulu Varadarajan <_govind@gmx.com>
---
 drivers/net/ethernet/cisco/enic/enic_ethtool.c | 30 ++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/drivers/net/ethernet/cisco/enic/enic_ethtool.c b/drivers/net/ethernet/cisco/enic/enic_ethtool.c
index 523c9ce..9bf53fc 100644
--- a/drivers/net/ethernet/cisco/enic/enic_ethtool.c
+++ b/drivers/net/ethernet/cisco/enic/enic_ethtool.c
@@ -379,6 +379,30 @@ static int enic_get_rxnfc(struct net_device *dev, struct ethtool_rxnfc *cmd,
 	return ret;
 }
 
+static int enic_get_rx_copybreak(struct net_device *dev,
+				 struct ethtool_tunable *tuna)
+{
+	struct enic *enic = netdev_priv(dev);
+
+	if (tuna->len < sizeof(tuna->data.rx_copybreak))
+		return -ENOSPC;
+	tuna->data.rx_copybreak = enic->rx_copybreak;
+
+	return 0;
+}
+
+static int enic_set_rx_copybreak(struct net_device *dev,
+				 struct ethtool_tunable *tuna)
+{
+	struct enic *enic = netdev_priv(dev);
+
+	if (tuna->len != sizeof(tuna->data.rx_copybreak))
+		return -EINVAL;
+	enic->rx_copybreak = tuna->data.rx_copybreak;
+
+	return 0;
+}
+
 static const struct ethtool_ops enic_ethtool_ops = {
 	.get_settings = enic_get_settings,
 	.get_drvinfo = enic_get_drvinfo,
@@ -391,6 +415,12 @@ static const struct ethtool_ops enic_ethtool_ops = {
 	.get_coalesce = enic_get_coalesce,
 	.set_coalesce = enic_set_coalesce,
 	.get_rxnfc = enic_get_rxnfc,
+	.tunable_ops = {
+		[ETHTOOL_RX_COPYBREAK] = {
+			.set = enic_set_rx_copybreak,
+			.get = enic_get_rx_copybreak,
+		},
+	},
 };
 
 void enic_set_ethtool_ops(struct net_device *netdev)
-- 
2.0.4

RE: [PATCH net-next v3 0/3] enic: Add support for rx_copybreak

[David Laight]

From: Govindarajulu Varadarajan
> dma_map_single()/dma_unmap_single() is more expensive than alloc_skb & memcpy
> for smaller packets. By doing this we can reuse the dma buff which is already
> mapped. This is very useful when iommu is on. The default skb copybreak value
> is 256.
> 
> When iommu is on, we can go much higher than 256. All the drivers that supports
> rx_copybreak provides module parameter to change this value. Since module
> parameter is the least preferred way for changing driver values, this series
> adds ethtool support for setting rx_copybreak.

Is there any mileage in having a system-wide default for rx_copybreak?
I'd have thought that the value is really driver independent since it
(mostly) depends on the comparative cost of dma_map and copy operations.

The same also applies to any equivalent copy done during transmit.
(Not sure if I've seen any drivers that keep a permanently mapped
buffer for transmitting small fragments.)

	David

RE: [PATCH net-next v3 0/3] enic: Add support for rx_copybreak

[Govindarajulu Varadarajan]

On Thu, 14 Aug 2014, David Laight wrote:
> Is there any mileage in having a system-wide default for rx_copybreak?
> I'd have thought that the value is really driver independent since it
> (mostly) depends on the comparative cost of dma_map and copy operations.
>

The cost skb_alloc(small_size) and memcpy(small_size) should be completely
independent of driver.

The cost of dma_unmap(mtu) , skb_alloc(mtu) & dma_map(mtu) determines the break
point. This operation should take less time for smaller mtu size,
and more time for larger mtu size. (especially for 9k mtu as we do not have
generic kmem_cache for size above 8192)

mtu of 9k is quite common for enic in data center. So rx_copybreak can go till
254. Most of the drivers use 100 as the copybreak, I guess thats because 1500
mtu is common for those nics and probably 100 is the break point.

Some driver allocates frags of pages, instead of contiguous memory.
I think it's better to leave the default to driver.

Re: [PATCH net-next v3 3/3] enic: Add tunable_ops support for rx_copybreak

[Michał Mirosław]

2014-08-14 11:29 GMT+02:00 Govindarajulu Varadarajan <_govind@gmx.com>:
> This patch adds support for setting/getting rx_copybreak using
> tunable_ops.
>
> Defines enic_get_rx_copybreak() & enic_set_rx_copybreak() tunable_ops.
>
> Signed-off-by: Govindarajulu Varadarajan <_govind@gmx.com>
> ---
>  drivers/net/ethernet/cisco/enic/enic_ethtool.c | 30 ++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
>
> diff --git a/drivers/net/ethernet/cisco/enic/enic_ethtool.c b/drivers/net/ethernet/cisco/enic/enic_ethtool.c
> index 523c9ce..9bf53fc 100644
> --- a/drivers/net/ethernet/cisco/enic/enic_ethtool.c
> +++ b/drivers/net/ethernet/cisco/enic/enic_ethtool.c
> @@ -379,6 +379,30 @@ static int enic_get_rxnfc(struct net_device *dev, struct ethtool_rxnfc *cmd,
>         return ret;
>  }
>
> +static int enic_get_rx_copybreak(struct net_device *dev,
> +                                struct ethtool_tunable *tuna)
> +{
> +       struct enic *enic = netdev_priv(dev);
> +
> +       if (tuna->len < sizeof(tuna->data.rx_copybreak))
> +               return -ENOSPC;
> +       tuna->data.rx_copybreak = enic->rx_copybreak;
> +
> +       return 0;
> +}
> +
> +static int enic_set_rx_copybreak(struct net_device *dev,
> +                                struct ethtool_tunable *tuna)
> +{
> +       struct enic *enic = netdev_priv(dev);
> +
> +       if (tuna->len != sizeof(tuna->data.rx_copybreak))
> +               return -EINVAL;
> +       enic->rx_copybreak = tuna->data.rx_copybreak;
> +
> +       return 0;
> +}
> +
>  static const struct ethtool_ops enic_ethtool_ops = {
>         .get_settings = enic_get_settings,
>         .get_drvinfo = enic_get_drvinfo,
> @@ -391,6 +415,12 @@ static const struct ethtool_ops enic_ethtool_ops = {
>         .get_coalesce = enic_get_coalesce,
>         .set_coalesce = enic_set_coalesce,
>         .get_rxnfc = enic_get_rxnfc,
> +       .tunable_ops = {
> +               [ETHTOOL_RX_COPYBREAK] = {
> +                       .set = enic_set_rx_copybreak,
> +                       .get = enic_get_rx_copybreak,
> +               },
> +       },
>  };
>
>  void enic_set_ethtool_ops(struct net_device *netdev)

This is a lot of boilerplate code to write for such a simple variable.

BTW, there are a lot of drivers that use rx_copybreak idea. Some time
ago, I tried to unify rx packet handling, but got into too many leafs
of the theme.You could google up 'rx_copybreak handling' if you'd like
to revive the idea.

Best Regards,
Michał Mirosław

Re: [PATCH net-next v3 2/3] ethtool: Add generic options for tunables

[David Miller]

From: Govindarajulu Varadarajan <_govind@gmx.com>
Date: Thu, 14 Aug 2014 14:59:19 +0530

> @@ -1621,6 +1621,50 @@ static int ethtool_get_module_eeprom(struct net_device *dev,
>  				      modinfo.eeprom_len);
>  }
>  
> +static int ethtool_get_tunable(struct net_device *dev, void __user *useraddr)
> +{
> +	int ret;
> +	struct ethtool_tunable tuna;
> +	const struct ethtool_tunable_ops *ops;
> +
> +	if (copy_from_user(&tuna, useraddr, sizeof(tuna)))
> +		return -EFAULT;
> +	if (tuna.tcmd >= ETHTOOL_TUNABLE_MAX)
> +		return -EOPNOTSUPP;
> +	ops = &dev->ethtool_ops->tunable_ops[tuna.tcmd];
> +	if (!ops->get)
> +		return -EOPNOTSUPP;
> +	ret = ops->get(dev, &tuna);
> +	if (ret)
> +		return ret;
> +	if (copy_to_user(useraddr, &tuna, sizeof(tuna)))
> +		return -EFAULT;
> +
> +	return 0;
> +}

You should be validating tuna.len here, it should not be happening in the drivers
as I see you are doing in your enic implementation.

Also, having a seperate OP for each tunable might be overkill.
Consider instead one "get_tunable" and one "set_tunable" callback,
which contains a switch statement over 'tcmd'.  The generic
ethtool_{g,s}et_tunable() would still validate the length, so that the
driver method does not need to do so.

Re: [PATCH net-next v3 2/3] ethtool: Add generic options for tunables

[Ben Hutchings]

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

On Thu, 2014-08-14 at 14:59 +0530, Govindarajulu Varadarajan wrote:
> This patch adds new ethtool cmd, ETHTOOL_GTUNABLE & ETHTOOL_STUNABLE for getting
> tunable values from driver.
> 
> Add ethtool_tunable_ops array to ethtool_ops. The array index will be
> the tunable tcmd as defined in enum tunable_cmd. ethtool_tunable_ops has two
> function pointers set & get. This is set by the driver for getting/setting
> particular tunable.
> 
> Signed-off-by: Govindarajulu Varadarajan <_govind@gmx.com>
> ---
>  include/linux/ethtool.h      |  6 ++++++
>  include/uapi/linux/ethtool.h | 17 +++++++++++++++
>  net/core/ethtool.c           | 51 ++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 74 insertions(+)
> 
> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
> index e658229..33bff94 100644
> --- a/include/linux/ethtool.h
> +++ b/include/linux/ethtool.h
> @@ -77,6 +77,11 @@ static inline u32 ethtool_rxfh_indir_default(u32 index, u32 n_rx_rings)
>  	return index % n_rx_rings;
>  }
>  
> +struct ethtool_tunable_ops {
> +	int (*set)(struct net_device *, struct ethtool_tunable *);

The second pointer type should be const-qualified.

> +	int (*get)(struct net_device *, struct ethtool_tunable *);
> +};
> +
>  /**
>   * struct ethtool_ops - optional netdev operations
>   * @get_settings: Get various device settings including Ethernet link
> @@ -257,6 +262,7 @@ struct ethtool_ops {
>  				     struct ethtool_eeprom *, u8 *);
>  	int	(*get_eee)(struct net_device *, struct ethtool_eee *);
>  	int	(*set_eee)(struct net_device *, struct ethtool_eee *);
> +	struct ethtool_tunable_ops tunable_ops[ETHTOOL_TUNABLE_MAX];

This is OK but if we add a lot of tunables then it bloats up each driver
with a (probably quite sparse) array of function pointers.

>  
> 
>  };
> diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
> index e3c7a71..99e43ca 100644
> --- a/include/uapi/linux/ethtool.h
> +++ b/include/uapi/linux/ethtool.h
> @@ -209,6 +209,21 @@ struct ethtool_value {
>  	__u32	data;
>  };
>  
> +enum tunable_cmd {
> +	ETHTOOL_RX_COPYBREAK = 0,
> +	ETHTOOL_TUNABLE_MAX,
> +};
> +
> +struct ethtool_tunable {
> +	u32 cmd;
> +	u32 tcmd;
> +	u32 len;
> +	union {
> +		u32 rx_copybreak;
> +		u32 data[48];
> +	} data;
[...]

This is not at all generic.  If tunables don't all have the same type,
then the type - not just the length - should be explicit.

I also think that if the length of a value can vary then we should not
declare a data member at all.  So we would have something like:

struct ethtool_tunable {
	__u32	cmd;
	__u32	id;
	__u32	type_id;
	__u32	len;
	__u8	data[0];
};

Then the value buffer would be passed to the driver functions separately
(as for other variable-length command structures).

By the way, you must use double-underscore prefixes on fixed-width
integer type names in UAPI headers.

Ben.

-- 
Ben Hutchings
Lowery's Law:
             If it jams, force it. If it breaks, it needed replacing anyway.

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

Re: [PATCH net-next v3 2/3] ethtool: Add generic options for tunables

[Govindarajulu Varadarajan]

On Fri, 22 Aug 2014, Ben Hutchings wrote:
> On Thu, 2014-08-14 at 14:59 +0530, Govindarajulu Varadarajan wrote:
>> @@ -257,6 +262,7 @@ struct ethtool_ops {
>>  				     struct ethtool_eeprom *, u8 *);
>>  	int	(*get_eee)(struct net_device *, struct ethtool_eee *);
>>  	int	(*set_eee)(struct net_device *, struct ethtool_eee *);
>> +	struct ethtool_tunable_ops tunable_ops[ETHTOOL_TUNABLE_MAX];
>
> This is OK but if we add a lot of tunables then it bloats up each driver
> with a (probably quite sparse) array of function pointers.
>

Will fix with David's suggestion.

>> +struct ethtool_tunable {
>> +	u32 cmd;
>> +	u32 tcmd;
>> +	u32 len;
>> +	union {
>> +		u32 rx_copybreak;
>> +		u32 data[48];
>> +	} data;
> [...]
>
> This is not at all generic.  If tunables don't all have the same type,
> then the type - not just the length - should be explicit.
>
> I also think that if the length of a value can vary then we should not
> declare a data member at all.  So we would have something like:
>
> struct ethtool_tunable {
> 	__u32	cmd;
> 	__u32	id;
> 	__u32	type_id;
> 	__u32	len;
> 	__u8	data[0];
> };
>
> Then the value buffer would be passed to the driver functions separately
> (as for other variable-length command structures).
>

OK. id show be the tunable command type right? Like RX_COPYBREAK, TX_COPYBREAK
etc. What is the type_id?

> By the way, you must use double-underscore prefixes on fixed-width
> integer type names in UAPI headers.
>

will fix it.

Thanks

Re: [PATCH net-next v3 2/3] ethtool: Add generic options for tunables

[Ben Hutchings]

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

On Wed, 2014-08-27 at 03:03 +0530, Govindarajulu Varadarajan wrote:
> On Fri, 22 Aug 2014, Ben Hutchings wrote:
> > On Thu, 2014-08-14 at 14:59 +0530, Govindarajulu Varadarajan wrote:
> >> @@ -257,6 +262,7 @@ struct ethtool_ops {
> >>  				     struct ethtool_eeprom *, u8 *);
> >>  	int	(*get_eee)(struct net_device *, struct ethtool_eee *);
> >>  	int	(*set_eee)(struct net_device *, struct ethtool_eee *);
> >> +	struct ethtool_tunable_ops tunable_ops[ETHTOOL_TUNABLE_MAX];
> >
> > This is OK but if we add a lot of tunables then it bloats up each driver
> > with a (probably quite sparse) array of function pointers.
> >
> 
> Will fix with David's suggestion.
> 
> >> +struct ethtool_tunable {
> >> +	u32 cmd;
> >> +	u32 tcmd;
> >> +	u32 len;
> >> +	union {
> >> +		u32 rx_copybreak;
> >> +		u32 data[48];
> >> +	} data;
> > [...]
> >
> > This is not at all generic.  If tunables don't all have the same type,
> > then the type - not just the length - should be explicit.
> >
> > I also think that if the length of a value can vary then we should not
> > declare a data member at all.  So we would have something like:
> >
> > struct ethtool_tunable {
> > 	__u32	cmd;
> > 	__u32	id;
> > 	__u32	type_id;
> > 	__u32	len;
> > 	__u8	data[0];
> > };
> >
> > Then the value buffer would be passed to the driver functions separately
> > (as for other variable-length command structures).
> >
> 
> OK. id show be the tunable command type right? Like RX_COPYBREAK, TX_COPYBREAK
> etc. What is the type_id?

A number that identifies the type of the tunable (it might be u32, u64,
link-layer address, text, etc.).

Ben.

> > By the way, you must use double-underscore prefixes on fixed-width
> > integer type names in UAPI headers.
> >
> 
> will fix it.
> 
> Thanks

-- 
Ben Hutchings
No political challenge can be met by shopping. - George Monbiot

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