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

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

Govindarajulu Varadarajan (3):
  enic: implement rx_copybreak
  ethtool: add rx_copybreak support
  enic: add ethtool support for set/get rx_copybreak

 drivers/net/ethernet/cisco/enic/enic.h         |  1 +
 drivers/net/ethernet/cisco/enic/enic_ethtool.c | 31 ++++++++++++++++
 drivers/net/ethernet/cisco/enic/enic_main.c    | 50 ++++++++++++++++++++++++--
 include/uapi/linux/ethtool.h                   |  2 ++
 4 files changed, 81 insertions(+), 3 deletions(-)

-- 
2.0.2

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

[PATCH net-next 2/3] ethtool: add rx_copybreak support

[Govindarajulu Varadarajan]

This patch adds rx_copybreak support to ethtool.h header file.
This is used to show/set rx_copybreak using ethtool.

Signed-off-by: Govindarajulu Varadarajan <_govind@gmx.com>
---
 include/uapi/linux/ethtool.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
index e3c7a71..d2912ad 100644
--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h
@@ -433,10 +433,12 @@ struct ethtool_ringparam {
 	__u32	rx_mini_max_pending;
 	__u32	rx_jumbo_max_pending;
 	__u32	tx_max_pending;
+	__u32	rx_max_copybreak_pending;
 	__u32	rx_pending;
 	__u32	rx_mini_pending;
 	__u32	rx_jumbo_pending;
 	__u32	tx_pending;
+	__u32	rx_copybreak_pending;
 };
 
 /**
-- 
2.0.2

[PATCH net-next 3/3] enic: add ethtool support for set/get rx_copybreak

[Govindarajulu Varadarajan]

This patch provides set_ringparam & get_ringparam ethtool_ops to set/get
rx_copybreak.

As of now, enic does not support change of rx/tx ring size. Allow change of
rx_copybreak alone.

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

diff --git a/drivers/net/ethernet/cisco/enic/enic_ethtool.c b/drivers/net/ethernet/cisco/enic/enic_ethtool.c
index 523c9ce..6780719 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,34 @@ static int enic_get_rxnfc(struct net_device *dev, struct ethtool_rxnfc *cmd,
 	return ret;
 }
 
+static void enic_get_ringparam(struct net_device *netdev,
+			       struct ethtool_ringparam *ring)
+{
+	struct enic *enic = netdev_priv(netdev);
+
+	ring->rx_max_pending = enic->rq[0].ring.desc_count;
+	ring->rx_pending = enic->rq[0].ring.desc_count;
+	ring->tx_max_pending = enic->wq[0].ring.desc_count;
+	ring->tx_pending = enic->wq[0].ring.desc_count;
+	ring->rx_copybreak_pending = enic->rx_copybreak;
+	ring->rx_max_copybreak_pending = netdev->mtu + VLAN_ETH_HLEN;
+}
+
+static int enic_set_ringparam(struct net_device *netdev,
+			      struct ethtool_ringparam *ring)
+{
+	struct enic *enic = netdev_priv(netdev);
+
+	if (ring->rx_copybreak_pending > (netdev->mtu + VLAN_ETH_HLEN) ||
+	    enic->rq[0].ring.desc_count != ring->rx_pending ||
+	    enic->wq[0].ring.desc_count != ring->tx_pending ||
+	    ring->rx_mini_pending || ring->rx_jumbo_pending)
+		return -EINVAL;
+	enic->rx_copybreak = ring->rx_copybreak_pending;
+
+	return 0;
+}
+
 static const struct ethtool_ops enic_ethtool_ops = {
 	.get_settings = enic_get_settings,
 	.get_drvinfo = enic_get_drvinfo,
@@ -391,6 +420,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_ringparam = enic_get_ringparam,
+	.set_ringparam = enic_set_ringparam,
 };
 
 void enic_set_ethtool_ops(struct net_device *netdev)
-- 
2.0.2

Re: [PATCH net-next 2/3] ethtool: add rx_copybreak support

[Ben Hutchings]

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

On Tue, 2014-07-22 at 23:01 +0530, Govindarajulu Varadarajan wrote:
> This patch adds rx_copybreak support to ethtool.h header file.
> This is used to show/set rx_copybreak using ethtool.
> 
> Signed-off-by: Govindarajulu Varadarajan <_govind@gmx.com>
> ---
>  include/uapi/linux/ethtool.h | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
> index e3c7a71..d2912ad 100644
> --- a/include/uapi/linux/ethtool.h
> +++ b/include/uapi/linux/ethtool.h
> @@ -433,10 +433,12 @@ struct ethtool_ringparam {
>  	__u32	rx_mini_max_pending;
>  	__u32	rx_jumbo_max_pending;
>  	__u32	tx_max_pending;
> +	__u32	rx_max_copybreak_pending;
>  	__u32	rx_pending;
>  	__u32	rx_mini_pending;
>  	__u32	rx_jumbo_pending;
>  	__u32	tx_pending;
> +	__u32	rx_copybreak_pending;
>  };
>  
>  /**

No, you can't rearrange UAPI structures like this.

Ben.

-- 
Ben Hutchings
Beware of programmers who carry screwdrivers. - Leonard Brandwein

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

Re: [PATCH net-next 2/3] ethtool: add rx_copybreak support

[Govindarajulu Varadarajan]

On Tue, 22 Jul 2014, Ben Hutchings wrote:

> On Tue, 2014-07-22 at 23:01 +0530, Govindarajulu Varadarajan wrote:
>> This patch adds rx_copybreak support to ethtool.h header file.
>> This is used to show/set rx_copybreak using ethtool.
>>
>> Signed-off-by: Govindarajulu Varadarajan <_govind@gmx.com>
>> ---
>>  include/uapi/linux/ethtool.h | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
>> index e3c7a71..d2912ad 100644
>> --- a/include/uapi/linux/ethtool.h
>> +++ b/include/uapi/linux/ethtool.h
>> @@ -433,10 +433,12 @@ struct ethtool_ringparam {
>>  	__u32	rx_mini_max_pending;
>>  	__u32	rx_jumbo_max_pending;
>>  	__u32	tx_max_pending;
>> +	__u32	rx_max_copybreak_pending;
>>  	__u32	rx_pending;
>>  	__u32	rx_mini_pending;
>>  	__u32	rx_jumbo_pending;
>>  	__u32	tx_pending;
>> +	__u32	rx_copybreak_pending;
>>  };
>>
>>  /**
>
> No, you can't rearrange UAPI structures like this.
>

So I will move these 2 variables to the end of the struct?
Also in ethtool-copy.h in ethtool.

Thanks

Re: [PATCH net-next 2/3] ethtool: add rx_copybreak support

[Ben Hutchings]

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

On Wed, 2014-07-23 at 00:05 +0530, Govindarajulu Varadarajan wrote:
> On Tue, 22 Jul 2014, Ben Hutchings wrote:
> 
> > On Tue, 2014-07-22 at 23:01 +0530, Govindarajulu Varadarajan wrote:
> >> This patch adds rx_copybreak support to ethtool.h header file.
> >> This is used to show/set rx_copybreak using ethtool.
> >>
> >> Signed-off-by: Govindarajulu Varadarajan <_govind@gmx.com>
> >> ---
> >>  include/uapi/linux/ethtool.h | 2 ++
> >>  1 file changed, 2 insertions(+)
> >>
> >> diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
> >> index e3c7a71..d2912ad 100644
> >> --- a/include/uapi/linux/ethtool.h
> >> +++ b/include/uapi/linux/ethtool.h
> >> @@ -433,10 +433,12 @@ struct ethtool_ringparam {
> >>  	__u32	rx_mini_max_pending;
> >>  	__u32	rx_jumbo_max_pending;
> >>  	__u32	tx_max_pending;
> >> +	__u32	rx_max_copybreak_pending;
> >>  	__u32	rx_pending;
> >>  	__u32	rx_mini_pending;
> >>  	__u32	rx_jumbo_pending;
> >>  	__u32	tx_pending;
> >> +	__u32	rx_copybreak_pending;
> >>  };
> >>
> >>  /**
> >
> > No, you can't rearrange UAPI structures like this.
> >
> 
> So I will move these 2 variables to the end of the struct?
> Also in ethtool-copy.h in ethtool.

No, you can't do that either.  Basically you can't change the size or
layout of any of the ethtool structures.

Ben.

-- 
Ben Hutchings
Beware of programmers who carry screwdrivers. - Leonard Brandwein

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

Re: [PATCH net-next 2/3] ethtool: add rx_copybreak support

[Govindarajulu Varadarajan]

On Tue, 22 Jul 2014, Ben Hutchings wrote:
> No, you can't do that either.  Basically you can't change the size or
> layout of any of the ethtool structures.

Ok. Can you suggest me how I should add rx_copybreak to ethtool?

May be add another function, set/get_rxcopybreak(), to ethtool_ops?

Thanks

Re: [PATCH net-next 2/3] ethtool: add rx_copybreak support

[Govindarajulu Varadarajan]

On Wed, 23 Jul 2014, Govindarajulu Varadarajan wrote:

> On Tue, 22 Jul 2014, Ben Hutchings wrote:
>> No, you can't do that either.  Basically you can't change the size or
>> layout of any of the ethtool structures.
>
> Ok. Can you suggest me how I should add rx_copybreak to ethtool?
>
> May be add another function, set/get_rxcopybreak(), to ethtool_ops?
>
> Thanks
>

Should I drop the idea of ethtool and resubmit just the rx_copybreak changes?
(use module parameter to change rx_copybreak value).

Or should I try something like this
3de0b592394d17b 'ethtool: Support for configurable RSS hash key', Create new
ethtool cmd for changing driver rx_copybreak value.

Thanks

Re: [PATCH net-next 2/3] ethtool: add rx_copybreak support

[Ben Hutchings]

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

On Sat, 2014-07-26 at 16:19 +0530, Govindarajulu Varadarajan wrote:
> On Wed, 23 Jul 2014, Govindarajulu Varadarajan wrote:
> 
> > On Tue, 22 Jul 2014, Ben Hutchings wrote:
> >> No, you can't do that either.  Basically you can't change the size or
> >> layout of any of the ethtool structures.
> >
> > Ok. Can you suggest me how I should add rx_copybreak to ethtool?
> >
> > May be add another function, set/get_rxcopybreak(), to ethtool_ops?
> >
> > Thanks
> >
> 
> Should I drop the idea of ethtool and resubmit just the rx_copybreak changes?
> (use module parameter to change rx_copybreak value).
> 
> Or should I try something like this
> 3de0b592394d17b 'ethtool: Support for configurable RSS hash key', Create new
> ethtool cmd for changing driver rx_copybreak value.

I think that you will need to add new ethtool operations to get and set
this parameter.  At the same time, please consider whether there are
other parameters for buffer allocation and use that might usefully be
configurable in some drivers, so they can be grouped together.

Some parameters that could be included are:

- Maximum frame size that should be copied to a pre-allocated DMA
  buffer rather than DMA-mapped directly from the skb.

  ibmveth has the module parameter 'tx_copybreak' for this.

- Maximum frame size to be transmitted by PIO rather than DMA, where
  the hardware supports both methods.

  The mlx4_en and sfc drivers have hardcoded values for this (MAX_BF
  and EFX_PIOBUF_SIZE_DEF respectively).

- Size of each RX DMA buffer, which is independent of MTU if
  scatter/gather is enabled.

  benet has the module parameter 'rx_frag_size' for this.

Others on netdev might have some other suggestions for inclusion.  You
could also add some reserved space for further expansion.

You should probably include maximum fields for all these, which the
'get' operation would set to 0 if the parameter isn't applicable to the
device.

Ben.

-- 
Ben Hutchings
73.46% of all statistics are made up.

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