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

[PATCH net-next v2 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.

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 support for DMA buffer settings
  enic: add ethtool support set/show rx_copybreak

 drivers/net/ethernet/cisco/enic/enic.h         |  1 +
 drivers/net/ethernet/cisco/enic/enic_ethtool.c | 24 +++++++++++++
 drivers/net/ethernet/cisco/enic/enic_main.c    | 50 ++++++++++++++++++++++++--
 include/linux/ethtool.h                        |  7 ++++
 include/uapi/linux/ethtool.h                   | 16 +++++++++
 net/core/ethtool.c                             | 32 +++++++++++++++++
 6 files changed, 127 insertions(+), 3 deletions(-)

-- 
2.0.3

[PATCH net-next v2 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.3

[PATCH net-next v2 2/3] ethtool: Add support for DMA buffer settings

[Govindarajulu Varadarajan]

This patch adds new ethtool_cmd for setting DMA buffer relate settings.
As of now rx_copybreak is the only parameter support.

Reserving more space in struct ethtool_buffparam for upcoming parameters like
tx_copybreak etc.

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

diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index e658229..5893034 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -184,6 +184,9 @@ 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_buffparam: Get DMA buffer related settings.
+ * @set_buffparam: Set DMA buffer related settings. 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
@@ -257,6 +260,10 @@ 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 *);
+	void	(*get_buffparam)(struct net_device *,
+				 struct ethtool_buffparam *);
+	int	(*set_buffparam)(struct net_device *,
+				 struct ethtool_buffparam *);
 
 
 };
diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
index e3c7a71..8c44104 100644
--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h
@@ -440,6 +440,20 @@ struct ethtool_ringparam {
 };
 
 /**
+ * struct ethtool_buffparam - DMA buffer parameters
+ * @rx_copybreak_cur: current receive DMA buff rx_copybreak.
+ * @rx_copybreak_min: min rx_copybreak set by driver.
+ * @rx_copybreak_max: Max rx_copybreak set by driver.
+ * @reserved: reserve room for future use.
+ */
+struct ethtool_buffparam {
+	__u32	cmd;
+	__u32	rx_copybreak_cur;
+	__u32	rx_copybreak_max;
+	__u8	reserved[84];
+};
+
+/**
  * struct ethtool_channels - configuring number of network channel
  * @cmd: ETHTOOL_{G,S}CHANNELS
  * @max_rx: Read only. Maximum number of receive channel the driver support.
@@ -1152,6 +1166,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_GBUFFPARAM	0x00000048 /* Get DMA buff parameter */
+#define ETHTOOL_SBUFFPARAM	0x00000049 /* Set DMA buff parameter */
 
 /* compatibility with older code */
 #define SPARC_ETH_GSET		ETHTOOL_GSET
diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index 17cb912..24fce27 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -1146,6 +1146,31 @@ static int ethtool_set_ringparam(struct net_device *dev, void __user *useraddr)
 	return dev->ethtool_ops->set_ringparam(dev, &ringparam);
 }
 
+static int ethtool_get_buffparam(struct net_device *dev, void __user *useraddr)
+{
+	struct ethtool_buffparam buffparam;
+
+	if (!dev->ethtool_ops->get_buffparam)
+		return -EOPNOTSUPP;
+	dev->ethtool_ops->get_buffparam(dev, &buffparam);
+	if (copy_to_user(useraddr, &buffparam, sizeof(buffparam)))
+		return -EFAULT;
+
+	return 0;
+}
+
+static int ethtool_set_buffparam(struct net_device *dev, void __user *useraddr)
+{
+	struct ethtool_buffparam buffparam;
+
+	if (!dev->ethtool_ops->set_buffparam)
+		return -EOPNOTSUPP;
+	if (copy_from_user(&buffparam, useraddr, sizeof(buffparam)))
+		return -EFAULT;
+
+	return dev->ethtool_ops->set_buffparam(dev, &buffparam);
+}
+
 static noinline_for_stack int ethtool_get_channels(struct net_device *dev,
 						   void __user *useraddr)
 {
@@ -1670,6 +1695,7 @@ int dev_ethtool(struct net *net, struct ifreq *ifr)
 	case ETHTOOL_GCHANNELS:
 	case ETHTOOL_GET_TS_INFO:
 	case ETHTOOL_GEEE:
+	case ETHTOOL_GBUFFPARAM:
 		break;
 	default:
 		if (!ns_capable(net->user_ns, CAP_NET_ADMIN))
@@ -1857,6 +1883,12 @@ int dev_ethtool(struct net *net, struct ifreq *ifr)
 	case ETHTOOL_GMODULEEEPROM:
 		rc = ethtool_get_module_eeprom(dev, useraddr);
 		break;
+	case ETHTOOL_GBUFFPARAM:
+		rc = ethtool_get_buffparam(dev, useraddr);
+		break;
+	case ETHTOOL_SBUFFPARAM:
+		rc = ethtool_set_buffparam(dev, useraddr);
+		break;
 	default:
 		rc = -EOPNOTSUPP;
 	}
-- 
2.0.3

[PATCH net-next v2 3/3] enic: add ethtool support set/show rx_copybreak

[Govindarajulu Varadarajan]

Add ethtools support for get/set_buffparam.

This is used to set the driver rx_copybreak value. Do not allow rx_copybreak
value to be set more than max frame size.

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

diff --git a/drivers/net/ethernet/cisco/enic/enic_ethtool.c b/drivers/net/ethernet/cisco/enic/enic_ethtool.c
index 523c9ce..4c9eb52 100644
--- a/drivers/net/ethernet/cisco/enic/enic_ethtool.c
+++ b/drivers/net/ethernet/cisco/enic/enic_ethtool.c
@@ -18,6 +18,7 @@
 
 #include <linux/netdevice.h>
 #include <linux/ethtool.h>
+#include <linux/if_vlan.h>
 
 #include "enic_res.h"
 #include "enic.h"
@@ -379,6 +380,27 @@ static int enic_get_rxnfc(struct net_device *dev, struct ethtool_rxnfc *cmd,
 	return ret;
 }
 
+static void enic_get_buffparam(struct net_device *netdev,
+			       struct ethtool_buffparam *data)
+{
+	struct enic *enic = netdev_priv(netdev);
+
+	data->rx_copybreak_cur = enic->rx_copybreak;
+	data->rx_copybreak_max = netdev->mtu + VLAN_ETH_HLEN;
+}
+
+static int enic_set_buffparam(struct net_device *netdev,
+			      struct ethtool_buffparam *data)
+{
+	struct enic *enic = netdev_priv(netdev);
+
+	if (data->rx_copybreak_cur > netdev->mtu + VLAN_ETH_HLEN)
+		return -EINVAL;
+	enic->rx_copybreak = data->rx_copybreak_cur;
+
+	return 0;
+}
+
 static const struct ethtool_ops enic_ethtool_ops = {
 	.get_settings = enic_get_settings,
 	.get_drvinfo = enic_get_drvinfo,
@@ -391,6 +413,8 @@ static const struct ethtool_ops enic_ethtool_ops = {
 	.get_coalesce = enic_get_coalesce,
 	.set_coalesce = enic_set_coalesce,
 	.get_rxnfc = enic_get_rxnfc,
+	.get_buffparam = enic_get_buffparam,
+	.set_buffparam = enic_set_buffparam,
 };
 
 void enic_set_ethtool_ops(struct net_device *netdev)
-- 
2.0.3

Re: [PATCH net-next v2 2/3] ethtool: Add support for DMA buffer settings

[David Miller]

From: Govindarajulu Varadarajan <_govind@gmx.com>
Date: Tue, 29 Jul 2014 17:10:38 +0530

> @@ -440,6 +440,20 @@ struct ethtool_ringparam {
>  };
>  
>  /**
> + * struct ethtool_buffparam - DMA buffer parameters
> + * @rx_copybreak_cur: current receive DMA buff rx_copybreak.
> + * @rx_copybreak_min: min rx_copybreak set by driver.
> + * @rx_copybreak_max: Max rx_copybreak set by driver.
> + * @reserved: reserve room for future use.
> + */
> +struct ethtool_buffparam {
> +	__u32	cmd;
> +	__u32	rx_copybreak_cur;
> +	__u32	rx_copybreak_max;
> +	__u8	reserved[84];
> +};

I don't understand the reasoning behind this reserved field.

You can't use it to add more fields later, because right now
we'll let the user put any garbage there and thus if you add
more fields that garbage from older apps would be interpreted
as one of the new values.

Largely we have not been adding reserved fields to new ethtool
structures, and this is the primary reason I guess.

It's a shame that when we want to add a new 32-bit knob we have
to add an entire new struct.

We have ethtool_value, but that's only good for one knob at a time and
we have to allocate an entire new ethtool command value for each such
knob.

I really don't know what the recommend here.

However I wonder what value that "max" thing has, I think setting the
value to infinity is just fine, it just means every packet will be
copied.

So if we remove the 'max', we just have the copybreak itself, and you
can therefore use ethtool_value and allocate the ethtool command
numbers.

What do you think?

Re: [PATCH net-next v2 2/3] ethtool: Add support for DMA buffer settings

[Govindarajulu Varadarajan]

On Wed, 30 Jul 2014, David Miller wrote:
> However I wonder what value that "max" thing has, I think setting the
> value to infinity is just fine, it just means every packet will be
> copied.
>
> So if we remove the 'max', we just have the copybreak itself, and you
> can therefore use ethtool_value and allocate the ethtool command
> numbers.

Yes, we can drop the max thing.

I will use ethtool_value and allocate new ethtool command for rx_copybreak
and send the patch soon.

Thanks

Re: [PATCH net-next v2 2/3] ethtool: Add support for DMA buffer settings

[Ben Hutchings]

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

On Wed, 2014-07-30 at 17:53 -0700, David Miller wrote:
> From: Govindarajulu Varadarajan <_govind@gmx.com>
> Date: Tue, 29 Jul 2014 17:10:38 +0530
> 
> > @@ -440,6 +440,20 @@ struct ethtool_ringparam {
> >  };
> >  
> >  /**
> > + * struct ethtool_buffparam - DMA buffer parameters
> > + * @rx_copybreak_cur: current receive DMA buff rx_copybreak.
> > + * @rx_copybreak_min: min rx_copybreak set by driver.
> > + * @rx_copybreak_max: Max rx_copybreak set by driver.
> > + * @reserved: reserve room for future use.
> > + */
> > +struct ethtool_buffparam {
> > +	__u32	cmd;
> > +	__u32	rx_copybreak_cur;
> > +	__u32	rx_copybreak_max;
> > +	__u8	reserved[84];
> > +};
> 
> I don't understand the reasoning behind this reserved field.
> 
> You can't use it to add more fields later, because right now
> we'll let the user put any garbage there and thus if you add
> more fields that garbage from older apps would be interpreted
> as one of the new values.

That's OK, we can test that they're all zero.  Or add flags.

> Largely we have not been adding reserved fields to new ethtool
> structures, and this is the primary reason I guess.

Yes we have, but not consistently.

> It's a shame that when we want to add a new 32-bit knob we have
> to add an entire new struct.
> 
> We have ethtool_value, but that's only good for one knob at a time and
> we have to allocate an entire new ethtool command value for each such
> knob.

Two commands and two function pointers!

> I really don't know what the recommend here.
> 
> However I wonder what value that "max" thing has, I think setting the
> value to infinity is just fine, it just means every packet will be
> copied.
> 
> So if we remove the 'max', we just have the copybreak itself, and you
> can therefore use ethtool_value and allocate the ethtool command
> numbers.
> 
> What do you think?

How about adding a generic operation for independent tunables:

struct ethtool_get_tunable {
	u32	cmd;
	u32	id;
	u64	value, min, max;
};

struct ethtool_set_tunable {
	u32	cmd;
	u32	id;
	u64	value;
};

	int (*get_tunable)(struct net_device *, struct ethtool_get_tunable *);
	int (*set_tunable)(struct net_device *, const struct ethtool_set_tunable *);

The id to name mapping could be provided either through a stringset or
macros in <uapi/linux/ethtool.h>.  And perhaps we could split the id
space to allow for driver-specific tunables (while strongly discouraging
those for in-tree drivers).

Ben.

-- 
Ben Hutchings
Reality is just a crutch for people who can't handle science fiction.

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

Re: [PATCH net-next v2 2/3] ethtool: Add support for DMA buffer settings

[Stephen Hemminger]

On Sat, 02 Aug 2014 14:56:22 +0100
Ben Hutchings <ben@decadent.org.uk> wrote:

> On Wed, 2014-07-30 at 17:53 -0700, David Miller wrote:
> > From: Govindarajulu Varadarajan <_govind@gmx.com>
> > Date: Tue, 29 Jul 2014 17:10:38 +0530
> > 
> > > @@ -440,6 +440,20 @@ struct ethtool_ringparam {
> > >  };
> > >  
> > >  /**
> > > + * struct ethtool_buffparam - DMA buffer parameters
> > > + * @rx_copybreak_cur: current receive DMA buff rx_copybreak.
> > > + * @rx_copybreak_min: min rx_copybreak set by driver.
> > > + * @rx_copybreak_max: Max rx_copybreak set by driver.
> > > + * @reserved: reserve room for future use.
> > > + */
> > > +struct ethtool_buffparam {
> > > +	__u32	cmd;
> > > +	__u32	rx_copybreak_cur;
> > > +	__u32	rx_copybreak_max;
> > > +	__u8	reserved[84];
> > > +};
> > 
> > I don't understand the reasoning behind this reserved field.
> > 
> > You can't use it to add more fields later, because right now
> > we'll let the user put any garbage there and thus if you add
> > more fields that garbage from older apps would be interpreted
> > as one of the new values.
> 
> That's OK, we can test that they're all zero.  Or add flags.
> 
> > Largely we have not been adding reserved fields to new ethtool
> > structures, and this is the primary reason I guess.
> 
> Yes we have, but not consistently.
> 
> > It's a shame that when we want to add a new 32-bit knob we have
> > to add an entire new struct.
> > 
> > We have ethtool_value, but that's only good for one knob at a time and
> > we have to allocate an entire new ethtool command value for each such
> > knob.
> 
> Two commands and two function pointers!
> 
> > I really don't know what the recommend here.
> > 
> > However I wonder what value that "max" thing has, I think setting the
> > value to infinity is just fine, it just means every packet will be
> > copied.
> > 
> > So if we remove the 'max', we just have the copybreak itself, and you
> > can therefore use ethtool_value and allocate the ethtool command
> > numbers.
> > 
> > What do you think?
> 
> How about adding a generic operation for independent tunables:
> 
> struct ethtool_get_tunable {
> 	u32	cmd;
> 	u32	id;
> 	u64	value, min, max;
> };
> 
> struct ethtool_set_tunable {
> 	u32	cmd;
> 	u32	id;
> 	u64	value;
> };
> 
> 	int (*get_tunable)(struct net_device *, struct ethtool_get_tunable *);
> 	int (*set_tunable)(struct net_device *, const struct ethtool_set_tunable *);
> 
> The id to name mapping could be provided either through a stringset or
> macros in <uapi/linux/ethtool.h>.  And perhaps we could split the id
> space to allow for driver-specific tunables (while strongly discouraging
> those for in-tree drivers).
> 
> Ben.
> 

Looks like you just reinvented netlink :-)

I wish ethtool would die.
It has fixed size static structures and has no notification mechanism.

If we could just get all the ethtool features into netlink, under IF_LINK
then most of this could go away.

Re: [PATCH net-next v2 2/3] ethtool: Add support for DMA buffer settings

[Ben Hutchings]

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

On Sat, 2014-08-02 at 07:26 -0700, Stephen Hemminger wrote:
[...]
> Looks like you just reinvented netlink :-)

Badly? ;-)

> I wish ethtool would die.
> It has fixed size static structures and has no notification mechanism.
>
> If we could just get all the ethtool features into netlink, under IF_LINK
> then most of this could go away.

It doesn't go away, it just stops growing.

I know we've talked about this before and I agree in principle.  *But*
neither you nor anyone else proposed how to get there from here.

Ben.

-- 
Ben Hutchings
Reality is just a crutch for people who can't handle science fiction.

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

Re: [PATCH net-next v2 2/3] ethtool: Add support for DMA buffer settings

[David Miller]

From: Ben Hutchings <ben@decadent.org.uk>
Date: Sat, 02 Aug 2014 14:56:22 +0100

> How about adding a generic operation for independent tunables:
> 
> struct ethtool_get_tunable {
> 	u32	cmd;
> 	u32	id;
> 	u64	value, min, max;
> };
> 
> struct ethtool_set_tunable {
> 	u32	cmd;
> 	u32	id;
> 	u64	value;
> };
> 
> 	int (*get_tunable)(struct net_device *, struct ethtool_get_tunable *);
> 	int (*set_tunable)(struct net_device *, const struct ethtool_set_tunable *);
> 
> The id to name mapping could be provided either through a stringset or
> macros in <uapi/linux/ethtool.h>.  And perhaps we could split the id
> space to allow for driver-specific tunables (while strongly discouraging
> those for in-tree drivers).

That would certainly work, and another approach would be that we could
also create a tunable namespace of sorts.

struct ethtool_tunable {
	u32	cmd;
	u32	len;
	u32	data[];
};

#define ETHTOOL_GTUNABLE	x
#define ETHTOOL_STUNABLE	y

...

#define ETHTOOL_TUNABLE_COPYBREAK	0x00000001	/* u32 */

etc.

I understand the reason to gravitate towards stringset, in that it would
minimize the amount of code needed in the ethtool utility itself.

Re: [PATCH net-next v2 2/3] ethtool: Add support for DMA buffer settings

[Govindarajulu Varadarajan]

On Sat, 2 Aug 2014, Ben Hutchings wrote:

> On Sat, 2014-08-02 at 07:26 -0700, Stephen Hemminger wrote:
> [...]
>> Looks like you just reinvented netlink :-)
>
> Badly? ;-)
>
>> I wish ethtool would die.
>> It has fixed size static structures and has no notification mechanism.
>>
>> If we could just get all the ethtool features into netlink, under IF_LINK
>> then most of this could go away.
>
> It doesn't go away, it just stops growing.
>
> I know we've talked about this before and I agree in principle.  *But*
> neither you nor anyone else proposed how to get there from here.
>

If netlink is the way to go, I think we should stop adding new features to
ethtool and add new features only through netlink.

Some thing like this.

* Create new genetlink for ethtool.
* define doit function for set/get driver parameters.
* For setting ethtool parameters, user program sends nlmsg with only variable
   that needs to be set. If some attr is not know to driver, it return error.
* For getting ethtool parameters user program sends nlmsg with get parameters
   type, driver sends nlmsg back with only relevant value. Other are assumed 0
   by user program
* This should provide backward and forward compatibility, and also should
   allow us to add new variables.

What do you guys think?

Signed-off-by: Govindarajulu Varadarajan <_govind@gmx.com>
---
  drivers/net/ethernet/cisco/enic/enic.h         |   1 +
  drivers/net/ethernet/cisco/enic/enic_ethtool.c |  33 ++++++++
  drivers/net/ethernet/cisco/enic/enic_main.c    |   1 +
  include/linux/netdevice.h                      |   2 +
  include/uapi/linux/etnetlink.h                 |  29 +++++++
  net/core/Makefile                              |   2 +-
  net/core/etnetlink.c                           | 109 +++++++++++++++++++++++++
  7 files changed, 176 insertions(+), 1 deletion(-)
  create mode 100644 include/uapi/linux/etnetlink.h
  create mode 100644 net/core/etnetlink.c

diff --git a/drivers/net/ethernet/cisco/enic/enic.h b/drivers/net/ethernet/cisco/enic/enic.h
index 5ba5ad0..1e3c709 100644
--- a/drivers/net/ethernet/cisco/enic/enic.h
+++ b/drivers/net/ethernet/cisco/enic/enic.h
@@ -246,5 +246,6 @@ int enic_sriov_enabled(struct enic *enic);
  int enic_is_valid_vf(struct enic *enic, int vf);
  int enic_is_dynamic(struct enic *enic);
  void enic_set_ethtool_ops(struct net_device *netdev);
+void enic_set_etnl_ops(struct net_device *netdev);

  #endif /* _ENIC_H_ */
diff --git a/drivers/net/ethernet/cisco/enic/enic_ethtool.c b/drivers/net/ethernet/cisco/enic/enic_ethtool.c
index 523c9ce..90662f2 100644
--- a/drivers/net/ethernet/cisco/enic/enic_ethtool.c
+++ b/drivers/net/ethernet/cisco/enic/enic_ethtool.c
@@ -18,6 +18,8 @@

  #include <linux/netdevice.h>
  #include <linux/ethtool.h>
+#include <net/genetlink.h>
+#include <linux/skbuff.h>

  #include "enic_res.h"
  #include "enic.h"
@@ -393,7 +395,38 @@ static const struct ethtool_ops enic_ethtool_ops = {
  	.get_rxnfc = enic_get_rxnfc,
  };

+int enic_buffparam_set(struct net_device *dev, struct sk_buff *skb,
+		       struct genl_info *info)
+{
+	struct enic *enic = netdev_priv(dev);
+
+	if (info->attrs[ETNL_RX_COPYBREAK]) {
+		enic->rx_copybreak = nla_get_u32(info->attrs[ETNL_RX_COPYBREAK]);
+		return 0;
+	}
+
+	return -EINVAL;
+}
+
+int enic_buffparam_get(struct net_device *dev, struct sk_buff *skb,
+			struct genl_info *info)
+{
+	struct enic *enic = netdev_priv(dev);
+
+	return nla_put_u32(skb, ETNL_RX_COPYBREAK, enic->rx_copybreak);
+}
+
+static const struct etnl_ops enic_etnl_ops = {
+	.buffparam_set = enic_buffparam_set,
+	.buffparam_get = enic_buffparam_get,
+};
+
  void enic_set_ethtool_ops(struct net_device *netdev)
  {
  	netdev->ethtool_ops = &enic_ethtool_ops;
  }
+
+void enic_set_etnl_ops(struct net_device *netdev)
+{
+	netdev->etnl_ops = &enic_etnl_ops;
+}
diff --git a/drivers/net/ethernet/cisco/enic/enic_main.c b/drivers/net/ethernet/cisco/enic/enic_main.c
index b7e69fd..aa22277 100644
--- a/drivers/net/ethernet/cisco/enic/enic_main.c
+++ b/drivers/net/ethernet/cisco/enic/enic_main.c
@@ -2540,6 +2540,7 @@ static int enic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)

  	netdev->watchdog_timeo = 2 * HZ;
  	enic_set_ethtool_ops(netdev);
+	enic_set_etnl_ops(netdev);

  	netdev->features |= NETIF_F_HW_VLAN_CTAG_TX | NETIF_F_HW_VLAN_CTAG_RX;
  	if (ENIC_SETTING(enic, LOOP)) {
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 3837739..cfc06bb 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -50,6 +50,7 @@
  #include <linux/netdev_features.h>
  #include <linux/neighbour.h>
  #include <uapi/linux/netdevice.h>
+#include <uapi/linux/etnetlink.h>

  struct netpoll_info;
  struct device;
@@ -1496,6 +1497,7 @@ struct net_device {
  #endif
  	const struct net_device_ops *netdev_ops;
  	const struct ethtool_ops *ethtool_ops;
+	const struct etnl_ops *etnl_ops;
  	const struct forwarding_accel_ops *fwd_ops;

  	const struct header_ops *header_ops;
diff --git a/include/uapi/linux/etnetlink.h b/include/uapi/linux/etnetlink.h
new file mode 100644
index 0000000..653b341
--- /dev/null
+++ b/include/uapi/linux/etnetlink.h
@@ -0,0 +1,29 @@
+#ifndef __NET_CORE_ETNETLINK_H
+#define __NET_CORE_ETNETLINK_H
+
+#include <linux/skbuff.h>
+#include <net/genetlink.h>
+#define ETNL_NAME "etnetlink"
+
+struct etnl_ops {
+	int (*buffparam_set)(struct net_device *netdev, struct sk_buff *skb,
+			     struct genl_info *info);
+	int (*buffparam_get)(struct net_device *netdev, struct sk_buff *skb,
+			     struct genl_info *info);
+};
+
+enum etnl_attr {
+	ETNL_BUFFPARAM_SET = 0,
+	ETNL_BUFFPARAM_GET = 1,
+};
+
+enum etnl_buffparam_enum {
+	ETNL_INVALID = 0,
+	ETNL_IF_INDEX = 1,
+	ETNL_RX_COPYBREAK = 2,
+	ETNL_BUFFPARAM_MAX,
+};
+
+#define ETNL_ATTR_MAX ETNL_BUFFPARAM_MAX + 1
+
+#endif /* __NET_CORE_ETNETLINK_H */
diff --git a/net/core/Makefile b/net/core/Makefile
index 71093d9..49bb55c 100644
--- a/net/core/Makefile
+++ b/net/core/Makefile
@@ -9,7 +9,7 @@ obj-$(CONFIG_SYSCTL) += sysctl_net_core.o

  obj-y		     += dev.o ethtool.o dev_addr_lists.o dst.o netevent.o \
  			neighbour.o rtnetlink.o utils.o link_watch.o filter.o \
-			sock_diag.o dev_ioctl.o tso.o
+			sock_diag.o dev_ioctl.o tso.o etnetlink.o

  obj-$(CONFIG_XFRM) += flow.o
  obj-y += net-sysfs.o
diff --git a/net/core/etnetlink.c b/net/core/etnetlink.c
new file mode 100644
index 0000000..2550a9d
--- /dev/null
+++ b/net/core/etnetlink.c
@@ -0,0 +1,109 @@
+#include <linux/if.h>
+#include <linux/module.h>
+#include <linux/err.h>
+#include <linux/list.h>
+#include <linux/slab.h>
+#include <linux/notifier.h>
+#include <linux/device.h>
+#include <linux/etherdevice.h>
+#include <linux/sched.h>
+#include <net/genetlink.h>
+
+#include <uapi/linux/etnetlink.h>
+
+static struct genl_family etnl_family = {
+	.id = GENL_ID_GENERATE,
+	.name = ETNL_NAME,
+	.version = 1,
+	.maxattr = ETNL_ATTR_MAX,
+	.netnsok = true,
+};
+
+int etnl_buffparam_set(struct sk_buff *skb, struct genl_info *info)
+{
+	struct net *net = genl_info_net(info);
+	struct net_device *netdev = NULL;
+	u32 if_index;
+	int ret = -EOPNOTSUPP;
+
+	if (!info->attrs[ETNL_IF_INDEX]) {
+		return -EINVAL;
+	} else {
+		if_index = nla_get_u32(info->attrs[ETNL_IF_INDEX]);
+		netdev = dev_get_by_index(net, if_index);
+		if (!netdev)
+			return -EINVAL;
+	}
+	if (netdev->etnl_ops && netdev->etnl_ops->buffparam_set) {
+		ret = netdev->etnl_ops->buffparam_set(netdev, skb, info);
+	}
+	dev_put(netdev);
+	return ret;
+}
+
+int etnl_buffparam_get(struct sk_buff *skb, struct genl_info *info)
+{
+	struct sk_buff *skb_reply;
+	struct net *net = genl_info_net(info);
+	struct net_device *netdev = NULL;
+	void *reply;
+	int if_index, ret;
+
+	if (!info->attrs[ETNL_IF_INDEX]) {
+		return -EINVAL;
+	} else {
+		if_index = nla_get_u32(info->attrs[ETNL_IF_INDEX]);
+		netdev = dev_get_by_index(net, if_index);
+		if (!netdev)
+			return -EINVAL;
+	}
+	skb_reply = genlmsg_new(NLMSG_GOODSIZE, GFP_KERNEL);
+	if (!skb_reply)
+		return -ENOMEM;
+	reply = genlmsg_put_reply(skb_reply, info, &etnl_family, 0,
+				  info->genlhdr->cmd);
+	if (!reply)
+		goto nla_put_failure;
+
+	if (netdev->etnl_ops && netdev->etnl_ops->buffparam_get)
+		if (netdev->etnl_ops->buffparam_get(netdev, skb_reply, info))
+			goto nla_put_failure;
+
+	genlmsg_end(skb_reply, reply);
+	return genlmsg_reply(skb_reply, info);
+
+nla_put_failure:
+	ret = -EMSGSIZE;
+	nlmsg_free(skb_reply);
+	return ret;
+}
+
+static const struct nla_policy etnl_buffparam_policy[ETNL_ATTR_MAX] = {
+	[ETNL_IF_INDEX]		= { .type = NLA_U32 },
+	[ETNL_RX_COPYBREAK]	= { .type = NLA_U32 },
+};
+
+static const struct genl_ops etnl_ops[] = {
+	{
+		.cmd = ETNL_BUFFPARAM_SET,
+		.doit = etnl_buffparam_set,
+		.policy = etnl_buffparam_policy,
+	},
+	{
+		.cmd = ETNL_BUFFPARAM_GET,
+		.doit = etnl_buffparam_get,
+		.policy = etnl_buffparam_policy,
+	},
+};
+
+static int __init etnl_init(void)
+{
+	return genl_register_family_with_ops(&etnl_family, etnl_ops);
+}
+//subsys_initcall(etnl_init);
+late_initcall(etnl_init);
+
+static void __exit etnl_exit(void)
+{
+}
+module_exit(etnl_exit);
-- 
2.0.4