Skip to contentSkip to conversation

ibmveth: Enable TCP checksum offload

PATCH In review Brian King 2 participants 13 messages Aug 6, 2007 – Aug 7, 2007

Subsystems (2)
lore
Share & tools
lore

Brian King

author
#0 lore raw

PATCH 1/6 ibmveth: Enable TCP checksum offload

This patchset enables TCP checksum offload support for IPV4 on ibmveth. This completely eliminates the generation and checking of the checksum for packets that are completely virtual and never touch a physical network. A simple TCP_STREAM netperf run on a virtual network with maximum mtu set yielded a ~30% increase in throughput. This feature is enabled by default on systems that support it, but can be disabled with a module option.

---

 linux-2.6-bjking1/drivers/net/ibmveth.c |   53 ++++++++++++++++++++++++++++++++
 linux-2.6-bjking1/drivers/net/ibmveth.h |   41 +++++++++++++++++++++++-
 2 files changed, 92 insertions(+), 2 deletions(-)
drivers/net/ibmveth.c renamed from drivers/net/ibmveth.c~ibmveth_csum_offload +53 -0
diff -puN drivers/net/ibmveth.c~ibmveth_csum_offload drivers/net/ibmveth.c
@@ -47,6 +47,8 @@
 #include <linux/mm.h>
 #include <linux/ethtool.h>
 #include <linux/proc_fs.h>
+#include <linux/in.h>
+#include <linux/ip.h>
 #include <asm/semaphore.h>
 #include <asm/hvcall.h>
 #include <asm/atomic.h>
@@ -131,6 +133,11 @@ static inline int ibmveth_rxq_frame_leng
 	return (adapter->rx_queue.queue_addr[adapter->rx_queue.index].length);
 }
 
+static inline int ibmveth_rxq_csum_good(struct ibmveth_adapter *adapter)
+{
+	return (adapter->rx_queue.queue_addr[adapter->rx_queue.index].csum_good);
+}
+
 /* setup the initial settings for a buffer pool */
 static void ibmveth_init_buffer_pool(struct ibmveth_buff_pool *pool, u32 pool_index, u32 pool_size, u32 buff_size, u32 pool_active)
 {
@@ -684,6 +691,24 @@ static int ibmveth_start_xmit(struct sk_
 					desc[0].fields.length, DMA_TO_DEVICE);
 	desc[0].fields.valid   = 1;
 
+	if (skb->ip_summed == CHECKSUM_PARTIAL &&
+	    ip_hdr(skb)->protocol != IPPROTO_TCP && skb_checksum_help(skb)) {
+		ibmveth_error_printk("tx: failed to checksum packet\n");
+		tx_dropped++;
+		goto out;
+	}
+
+	if (skb->ip_summed == CHECKSUM_PARTIAL) {
+		unsigned char *buf = skb_transport_header(skb) + skb->csum_offset;
+
+		desc[0].fields.no_csum = 1;
+		desc[0].fields.csum_good = 1;
+
+		/* Need to zero out the checksum */
+		buf[0] = 0;
+		buf[1] = 0;
+	}
+
 	if(dma_mapping_error(desc[0].fields.address)) {
 		ibmveth_error_printk("tx: unable to map initial fragment\n");
 		tx_map_failed++;
@@ -702,6 +727,10 @@ static int ibmveth_start_xmit(struct sk_
 				frag->size, DMA_TO_DEVICE);
 		desc[curfrag+1].fields.length = frag->size;
 		desc[curfrag+1].fields.valid  = 1;
+		if (skb->ip_summed == CHECKSUM_PARTIAL) {
+			desc[curfrag+1].fields.no_csum = 1;
+			desc[curfrag+1].fields.csum_good = 1;
+		}
 
 		if(dma_mapping_error(desc[curfrag+1].fields.address)) {
 			ibmveth_error_printk("tx: unable to map fragment %d\n", curfrag);
@@ -792,7 +821,11 @@ static int ibmveth_poll(struct net_devic
 			} else {
 				int length = ibmveth_rxq_frame_length(adapter);
 				int offset = ibmveth_rxq_frame_offset(adapter);
+				int csum_good = ibmveth_rxq_csum_good(adapter);
+
 				skb = ibmveth_rxq_get_buffer(adapter);
+				if (csum_good)
+					skb->ip_summed = CHECKSUM_UNNECESSARY;
 
 				ibmveth_rxq_harvest_buffer(adapter);
 
@@ -962,8 +995,10 @@ static void ibmveth_poll_controller(stru
 static int __devinit ibmveth_probe(struct vio_dev *dev, const struct vio_device_id *id)
 {
 	int rc, i;
+	long ret;
 	struct net_device *netdev;
 	struct ibmveth_adapter *adapter = NULL;
+	union ibmveth_illan_attributes set_attr, ret_attr;
 
 	unsigned char *mac_addr_p;
 	unsigned int *mcastFilterSize_p;
@@ -1058,6 +1093,24 @@ static int __devinit ibmveth_probe(struc
 
 	ibmveth_debug_printk("registering netdev...\n");
 
+	ret = h_illan_attributes(dev->unit_address, 0, 0, &ret_attr.desc);
+
+	if (ret == H_SUCCESS && !ret_attr.fields.active_trunk &&
+	    !ret_attr.fields.trunk_priority &&
+	    ret_attr.fields.csum_offload_padded_pkt_support) {
+		set_attr.desc = 0;
+		set_attr.fields.tcp_csum_offload_ipv4 = 1;
+
+		ret = h_illan_attributes(dev->unit_address, 0, set_attr.desc,
+					 &ret_attr.desc);
+
+		if (ret == H_SUCCESS)
+			netdev->features |= NETIF_F_IP_CSUM;
+		else
+			ret = h_illan_attributes(dev->unit_address, set_attr.desc,
+						 0, &ret_attr.desc);
+	}
+
 	rc = register_netdev(netdev);
 
 	if(rc) {
drivers/net/ibmveth.h renamed from drivers/net/ibmveth.h~ibmveth_csum_offload +39 -2

62 lines not on this page · raw message

_

author
#1 lore raw

PATCH 2/6 ibmveth: Implement ethtool hooks to enable/disable checksum offload

This patch adds the appropriate ethtool hooks to allow for enabling/disabling of hypervisor assisted checksum offload for TCP.

---

 linux-2.6-bjking1/drivers/net/ibmveth.c |  118 +++++++++++++++++++++++++++++++-
 linux-2.6-bjking1/drivers/net/ibmveth.h |    1 
 2 files changed, 117 insertions(+), 2 deletions(-)
drivers/net/ibmveth.c renamed from drivers/net/ibmveth.c~ibmveth_csum_offload_ethtool +116 -2
diff -puN drivers/net/ibmveth.c~ibmveth_csum_offload_ethtool drivers/net/ibmveth.c
@@ -641,12 +641,125 @@ static u32 netdev_get_link(struct net_de
 	return 1;
 }
 
+static void ibmveth_set_rx_csum_flags(struct net_device *dev, u32 data)
+{
+	struct ibmveth_adapter *adapter = dev->priv;
+
+	if (data)
+		adapter->rx_csum = 1;
+	else {
+		adapter->rx_csum = 0;
+		dev->features &= ~NETIF_F_IP_CSUM;
+	}
+}
+
+static void ibmveth_set_tx_csum_flags(struct net_device *dev, u32 data)
+{
+	struct ibmveth_adapter *adapter = dev->priv;
+
+	if (data) {
+		dev->features |= NETIF_F_IP_CSUM;
+		adapter->rx_csum = 1;
+	} else
+		dev->features &= ~NETIF_F_IP_CSUM;
+}
+
+static int ibmveth_set_csum_offload(struct net_device *dev, u32 data,
+				    void (*done) (struct net_device *, u32))
+{
+	struct ibmveth_adapter *adapter = dev->priv;
+	union ibmveth_illan_attributes set_attr, clr_attr, ret_attr;
+	long ret;
+	int rc1 = 0, rc2 = 0;
+	int restart = 0;
+
+	if (netif_running(dev)) {
+		restart = 1;
+		adapter->pool_config = 1;
+		ibmveth_close(dev);
+		adapter->pool_config = 0;
+	}
+
+	set_attr.desc = 0;
+	clr_attr.desc = 0;
+
+	if (data)
+		set_attr.fields.tcp_csum_offload_ipv4 = 1;
+	else
+		clr_attr.fields.tcp_csum_offload_ipv4 = 1;
+
+	ret = h_illan_attributes(adapter->vdev->unit_address, 0, 0, &ret_attr.desc);
+
+	if (ret == H_SUCCESS && !ret_attr.fields.active_trunk &&
+	    !ret_attr.fields.trunk_priority &&
+	    ret_attr.fields.csum_offload_padded_pkt_support) {
+		ret = h_illan_attributes(adapter->vdev->unit_address, clr_attr.desc,
+					 set_attr.desc, &ret_attr.desc);
+
+		if (ret != H_SUCCESS) {
+			rc1 = -EIO;
+			ibmveth_error_printk("unable to change checksum offload settings."
+					     " %d rc=%ld\n", data, ret);
+
+			ret = h_illan_attributes(adapter->vdev->unit_address,
+						 set_attr.desc, clr_attr.desc, &ret_attr.desc);
+		} else
+			done(dev, data);
+	} else {
+		rc1 = -EIO;
+		ibmveth_error_printk("unable to change checksum offload settings."
+				     " %d rc=%ld ret_attr=%lx\n", data, ret, ret_attr.desc);
+	}
+
+	if (restart)
+		rc2 = ibmveth_open(dev);
+
+	return rc1 ? rc1 : rc2;
+}
+
+static int ibmveth_set_rx_csum(struct net_device *dev, u32 data)
+{
+	struct ibmveth_adapter *adapter = dev->priv;
+
+	if ((data && adapter->rx_csum) || (!data && !adapter->rx_csum))
+		return 0;
+
+	return ibmveth_set_csum_offload(dev, data, ibmveth_set_rx_csum_flags);
+}
+
+static int ibmveth_set_tx_csum(struct net_device *dev, u32 data)
+{
+	struct ibmveth_adapter *adapter = dev->priv;
+	int rc = 0;
+
+	if (data && (dev->features & NETIF_F_IP_CSUM))
+		return 0;
+	if (!data && !(dev->features & NETIF_F_IP_CSUM))
+		return 0;
+
+	if (data && !adapter->rx_csum)
+		rc = ibmveth_set_csum_offload(dev, data, ibmveth_set_tx_csum_flags);
+	else
+		ibmveth_set_tx_csum_flags(dev, data);
+
+	return rc;
+}
+
+static u32 ibmveth_get_rx_csum(struct net_device *dev)
+{
+	struct ibmveth_adapter *adapter = dev->priv;
+	return adapter->rx_csum;
+}
+
 static const struct ethtool_ops netdev_ethtool_ops = {
 	.get_drvinfo		= netdev_get_drvinfo,
 	.get_settings		= netdev_get_settings,
 	.get_link		= netdev_get_link,
 	.get_sg			= ethtool_op_get_sg,
 	.get_tx_csum		= ethtool_op_get_tx_csum,
+	.set_tx_csum		= ibmveth_set_tx_csum,
+	.get_rx_csum		= ibmveth_get_rx_csum,
+	.set_rx_csum		= ibmveth_set_rx_csum
 };
 
 static int ibmveth_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
@@ -1104,9 +1217,10 @@ static int __devinit ibmveth_probe(struc
 		ret = h_illan_attributes(dev->unit_address, 0, set_attr.desc,
 					 &ret_attr.desc);
 
-		if (ret == H_SUCCESS)
+		if (ret == H_SUCCESS) {
+			adapter->rx_csum = 1;
 			netdev->features |= NETIF_F_IP_CSUM;
-		else
+		} else
 			ret = h_illan_attributes(dev->unit_address, set_attr.desc,
 						 0, &ret_attr.desc);
 	}
drivers/net/ibmveth.h renamed from drivers/net/ibmveth.h~ibmveth_csum_offload_ethtool +1 -0
diff -puN drivers/net/ibmveth.h~ibmveth_csum_offload_ethtool drivers/net/ibmveth.h
@@ -137,6 +137,7 @@ struct ibmveth_adapter {
     struct ibmveth_buff_pool rx_buff_pool[IbmVethNumBufferPools];
     struct ibmveth_rx_q rx_queue;
     int pool_config;
+    int rx_csum;
 
     /* adapter specific stats */
     u64 replenish_task_cycles;

_

#2 lore raw

Re: [PATCH 2/6] ibmveth: Implement ethtool hooks to enable/disable checksum offload

Quoted 16 lines · Brian King

This patch adds the appropriate ethtool hooks to allow for enabling/disabling of hypervisor assisted checksum offload for TCP.

---

 linux-2.6-bjking1/drivers/net/ibmveth.c |  118 +++++++++++++++++++++++++++++++-
 linux-2.6-bjking1/drivers/net/ibmveth.h |    1 
 2 files changed, 117 insertions(+), 2 deletions(-)

diff -puN drivers/net/ibmveth.c~ibmveth_csum_offload_ethtool drivers/net/ibmveth.c

drivers/net/ibmveth.c renamed from drivers/net/ibmveth.c~ibmveth_csum_offload_ethtool +0 -0
@@ -641,12 +641,125 @@ static u32 netdev_get_link(struct net_de
 	return 1;
 }

drivers/net/ibmveth.c static u32 netdev_get_link(struct net_de · line 652

Quoted 10 lines
drivers/net/ibmveth.c renamed from drivers/net/ibmveth.c~ibmveth_csum_offload_ethtool +9 -0
 
+static void ibmveth_set_rx_csum_flags(struct net_device *dev, u32 data)
+{
+	struct ibmveth_adapter *adapter = dev->priv;
+
+	if (data)
+		adapter->rx_csum = 1;
+	else {
+		adapter->rx_csum = 0;
+		dev->features &= ~NETIF_F_IP_CSUM;

why does this RX-related code clear a TX-related flag?

otherwise looks OK

author
#3 lore raw

Re: [PATCH 2/6] ibmveth: Implement ethtool hooks to enable/disable checksum offload

Quoted 18 lines · Jeff Garzik
Quoted 17 lines · Brian King

This patch adds the appropriate ethtool hooks to allow for enabling/disabling of hypervisor assisted checksum offload for TCP.

---

 linux-2.6-bjking1/drivers/net/ibmveth.c |  118 +++++++++++++++++++++++++++++++-
 linux-2.6-bjking1/drivers/net/ibmveth.h |    1 
 2 files changed, 117 insertions(+), 2 deletions(-)

diff -puN drivers/net/ibmveth.c~ibmveth_csum_offload_ethtool drivers/net/ibmveth.c

drivers/net/ibmveth.c renamed from drivers/net/ibmveth.c~ibmveth_csum_offload_ethtool +0 -0
@@ -641,12 +641,125 @@ static u32 netdev_get_link(struct net_de
 	return 1;
 }
 
Quoted 11 lines

drivers/net/ibmveth.c static u32 netdev_get_link(struct net_de · line 652

Quoted 9 lines
drivers/net/ibmveth.c renamed from drivers/net/ibmveth.c~ibmveth_csum_offload_ethtool +9 -0
+static void ibmveth_set_rx_csum_flags(struct net_device *dev, u32 data)
+{
+	struct ibmveth_adapter *adapter = dev->priv;
+
+	if (data)
+		adapter->rx_csum = 1;
+	else {
+		adapter->rx_csum = 0;
+		dev->features &= ~NETIF_F_IP_CSUM;

why does this RX-related code clear a TX-related flag?

Its related to how the pSeries firmware works. The firmware provides an interface to enable "checksum offload", which means both tx and rx checksum offload from the firmware's point of view. The firmware does not support enabling checksum offload for only rx. If I disable it for rx I have to disable it for tx as well, otherwise the firmware will reject all future tx buffers I throw at it that are not checksummed.

-Brian

Signature · 3 lines
Brian King
Linux on Power Virtualization
IBM Linux Technology Center
#4 lore raw

Re: [PATCH 2/6] ibmveth: Implement ethtool hooks to enable/disable checksum offload

Quoted 30 lines · Brian King
Quoted 18 lines · Jeff Garzik
Quoted 17 lines · Brian King

This patch adds the appropriate ethtool hooks to allow for enabling/disabling of hypervisor assisted checksum offload for TCP.

---

 linux-2.6-bjking1/drivers/net/ibmveth.c |  118 +++++++++++++++++++++++++++++++-
 linux-2.6-bjking1/drivers/net/ibmveth.h |    1 
 2 files changed, 117 insertions(+), 2 deletions(-)

diff -puN drivers/net/ibmveth.c~ibmveth_csum_offload_ethtool drivers/net/ibmveth.c

drivers/net/ibmveth.c renamed from drivers/net/ibmveth.c~ibmveth_csum_offload_ethtool +0 -0
@@ -641,12 +641,125 @@ static u32 netdev_get_link(struct net_de
 	return 1;
 }
 
Quoted 10 lines

drivers/net/ibmveth.c static u32 netdev_get_link(struct net_de · line 652

Quoted 9 lines
drivers/net/ibmveth.c renamed from drivers/net/ibmveth.c~ibmveth_csum_offload_ethtool +9 -0
+static void ibmveth_set_rx_csum_flags(struct net_device *dev, u32 data)
+{
+	struct ibmveth_adapter *adapter = dev->priv;
+
+	if (data)
+		adapter->rx_csum = 1;
+	else {
+		adapter->rx_csum = 0;
+		dev->features &= ~NETIF_F_IP_CSUM;

why does this RX-related code clear a TX-related flag?

Quoted 6 lines

Its related to how the pSeries firmware works. The firmware provides an interface to enable "checksum offload", which means both tx and rx checksum offload from the firmware's point of view. The firmware does not support enabling checksum offload for only rx. If I disable it for rx I have to disable it for tx as well, otherwise the firmware will reject all future tx buffers I throw at it that are not checksummed.

ACK once you add a comment describing this :)

author
#5 lore raw

PATCH 3/6 ibmveth: Add ethtool TSO handlers

In reply to Brian King

Add handlers for get_tso and get_ufo to prevent errors being printed by ethtool.

---

 drivers/net/ibmveth.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
drivers/net/ibmveth.c renamed from drivers/net/ibmveth.c~ibmveth_ethtool_get_tso +3 -1
diff -puN drivers/net/ibmveth.c~ibmveth_ethtool_get_tso drivers/net/ibmveth.c
@@ -759,7 +759,9 @@ static const struct ethtool_ops netdev_e
 	.get_tx_csum		= ethtool_op_get_tx_csum,
 	.set_tx_csum		= ibmveth_set_tx_csum,
 	.get_rx_csum		= ibmveth_get_rx_csum,
-	.set_rx_csum		= ibmveth_set_rx_csum
+	.set_rx_csum		= ibmveth_set_rx_csum,
+	.get_tso			= ethtool_op_get_tso,
+	.get_ufo			= ethtool_op_get_ufo
 };
 
 static int ibmveth_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)

_

#6 lore raw

Re: [PATCH 3/6] ibmveth: Add ethtool TSO handlers

Quoted 13 lines · Brian King

Add handlers for get_tso and get_ufo to prevent errors being printed by ethtool.

---

 drivers/net/ibmveth.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff -puN drivers/net/ibmveth.c~ibmveth_ethtool_get_tso drivers/net/ibmveth.c

drivers/net/ibmveth.c static const struct ethtool_ops netdev_e · line 764

Quoted 7 lines
drivers/net/ibmveth.c renamed from drivers/net/ibmveth.c~ibmveth_ethtool_get_tso +3 -1
@@ -759,7 +759,9 @@ static const struct ethtool_ops netdev_e
 	.get_tx_csum		= ethtool_op_get_tx_csum,
 	.set_tx_csum		= ibmveth_set_tx_csum,
 	.get_rx_csum		= ibmveth_get_rx_csum,
-	.set_rx_csum		= ibmveth_set_rx_csum
+	.set_rx_csum		= ibmveth_set_rx_csum,
+	.get_tso			= ethtool_op_get_tso,
+	.get_ufo			= ethtool_op_get_ufo

ACK, once you add a comma to the end of the final initializer

As you see from this patch, the practice of -not- having commas at the end of a list of struct initializers is not patch-friendly, since you must touch an unrelated line each time you patch the end of the struct.
For named initializers particularly, the lack of a comma is even more
useless.

So, it might tweak some C perfectionists, but adding that seemingly-useless comma at the end of the last entry reduces maintenance headache and makes patch reviews slightly more clear.

Jeff

author
#7 lore raw

PATCH 4/6 ibmveth: Add ethtool driver stats hooks

In reply to Brian King

Add ethtool hooks to ibmveth to retrieve driver statistics.

---

 drivers/net/ibmveth.c |   53 +++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 52 insertions(+), 1 deletion(-)
drivers/net/ibmveth.c renamed from drivers/net/ibmveth.c~ibmveth_ethtool_driver_stats +52 -1
diff -puN drivers/net/ibmveth.c~ibmveth_ethtool_driver_stats drivers/net/ibmveth.c
@@ -112,6 +112,28 @@ MODULE_DESCRIPTION("IBM i/pSeries Virtua
 MODULE_LICENSE("GPL");
 MODULE_VERSION(ibmveth_driver_version);
 
+struct ibmveth_stat {
+	char name[ETH_GSTRING_LEN];
+	int offset;
+};
+
+#define IBMVETH_STAT_OFF(stat) offsetof(struct ibmveth_adapter, stat)
+#define IBMVETH_GET_STAT(a, off) *((u64 *)(((unsigned long)(a)) + off))
+
+struct ibmveth_stat ibmveth_stats[] = {
+	{ "replenish_task_cycles", IBMVETH_STAT_OFF(replenish_task_cycles) },
+	{ "replenish_no_mem", IBMVETH_STAT_OFF(replenish_no_mem) },
+	{ "replenish_add_buff_failure", IBMVETH_STAT_OFF(replenish_add_buff_failure) },
+	{ "replenish_add_buff_success", IBMVETH_STAT_OFF(replenish_add_buff_success) },
+	{ "rx_invalid_buffer", IBMVETH_STAT_OFF(rx_invalid_buffer) },
+	{ "rx_no_buffer", IBMVETH_STAT_OFF(rx_no_buffer) },
+	{ "tx_multidesc_send", IBMVETH_STAT_OFF(tx_multidesc_send) },
+	{ "tx_linearized", IBMVETH_STAT_OFF(tx_linearized) },
+	{ "tx_linearize_failed", IBMVETH_STAT_OFF(tx_linearize_failed) },
+	{ "tx_map_failed", IBMVETH_STAT_OFF(tx_map_failed) },
+	{ "tx_send_failed", IBMVETH_STAT_OFF(tx_send_failed) }
+};
+
 /* simple methods of getting data from the current rxq entry */
 static inline int ibmveth_rxq_pending_buffer(struct ibmveth_adapter *adapter)
 {
@@ -751,6 +773,32 @@ static u32 ibmveth_get_rx_csum(struct ne
 	return adapter->rx_csum;
 }
 
+static void ibmveth_get_strings(struct net_device *dev, u32 stringset, u8 *data)
+{
+	int i;
+
+	if (stringset != ETH_SS_STATS)
+		return;
+
+	for (i = 0; i < ARRAY_SIZE(ibmveth_stats); i++, data += ETH_GSTRING_LEN)
+		memcpy(data, ibmveth_stats[i].name, ETH_GSTRING_LEN);
+}
+
+static int ibmveth_get_stats_count(struct net_device *dev)
+{
+	return ARRAY_SIZE(ibmveth_stats);
+}
+
+static void ibmveth_get_ethtool_stats(struct net_device *dev,
+				      struct ethtool_stats *stats, u64 *data)
+{
+	int i;
+	struct ibmveth_adapter *adapter = dev->priv;
+
+	for (i = 0; i < ARRAY_SIZE(ibmveth_stats); i++)
+		data[i] = IBMVETH_GET_STAT(adapter, ibmveth_stats[i].offset);
+}
+
 static const struct ethtool_ops netdev_ethtool_ops = {
 	.get_drvinfo		= netdev_get_drvinfo,
 	.get_settings		= netdev_get_settings,
@@ -761,7 +809,10 @@ static const struct ethtool_ops netdev_e
 	.get_rx_csum		= ibmveth_get_rx_csum,
 	.set_rx_csum		= ibmveth_set_rx_csum,
 	.get_tso			= ethtool_op_get_tso,
-	.get_ufo			= ethtool_op_get_ufo
+	.get_ufo			= ethtool_op_get_ufo,
+	.get_strings		= ibmveth_get_strings,
+	.get_stats_count		= ibmveth_get_stats_count,
+	.get_ethtool_stats	= ibmveth_get_ethtool_stats
 };
 
 static int ibmveth_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)

diff -puN drivers/net/ibmveth.h~ibmveth_ethtool_driver_stats drivers/net/ibmveth.h _

#8 lore raw

Re: [PATCH 4/6] ibmveth: Add ethtool driver stats hooks

Quoted 74 lines · Brian King

Add ethtool hooks to ibmveth to retrieve driver statistics.

---

 drivers/net/ibmveth.c |   53 +++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 52 insertions(+), 1 deletion(-)

diff -puN drivers/net/ibmveth.c~ibmveth_ethtool_driver_stats drivers/net/ibmveth.c

drivers/net/ibmveth.c renamed from drivers/net/ibmveth.c~ibmveth_ethtool_driver_stats +48 -0
@@ -112,6 +112,28 @@ MODULE_DESCRIPTION("IBM i/pSeries Virtua
 MODULE_LICENSE("GPL");
 MODULE_VERSION(ibmveth_driver_version);
 
+struct ibmveth_stat {
+	char name[ETH_GSTRING_LEN];
+	int offset;
+};
+
+#define IBMVETH_STAT_OFF(stat) offsetof(struct ibmveth_adapter, stat)
+#define IBMVETH_GET_STAT(a, off) *((u64 *)(((unsigned long)(a)) + off))
+
+struct ibmveth_stat ibmveth_stats[] = {
+	{ "replenish_task_cycles", IBMVETH_STAT_OFF(replenish_task_cycles) },
+	{ "replenish_no_mem", IBMVETH_STAT_OFF(replenish_no_mem) },
+	{ "replenish_add_buff_failure", IBMVETH_STAT_OFF(replenish_add_buff_failure) },
+	{ "replenish_add_buff_success", IBMVETH_STAT_OFF(replenish_add_buff_success) },
+	{ "rx_invalid_buffer", IBMVETH_STAT_OFF(rx_invalid_buffer) },
+	{ "rx_no_buffer", IBMVETH_STAT_OFF(rx_no_buffer) },
+	{ "tx_multidesc_send", IBMVETH_STAT_OFF(tx_multidesc_send) },
+	{ "tx_linearized", IBMVETH_STAT_OFF(tx_linearized) },
+	{ "tx_linearize_failed", IBMVETH_STAT_OFF(tx_linearize_failed) },
+	{ "tx_map_failed", IBMVETH_STAT_OFF(tx_map_failed) },
+	{ "tx_send_failed", IBMVETH_STAT_OFF(tx_send_failed) }
+};
+
 /* simple methods of getting data from the current rxq entry */
 static inline int ibmveth_rxq_pending_buffer(struct ibmveth_adapter *adapter)
 {
@@ -751,6 +773,32 @@ static u32 ibmveth_get_rx_csum(struct ne
 	return adapter->rx_csum;
 }
 
+static void ibmveth_get_strings(struct net_device *dev, u32 stringset, u8 *data)
+{
+	int i;
+
+	if (stringset != ETH_SS_STATS)
+		return;
+
+	for (i = 0; i < ARRAY_SIZE(ibmveth_stats); i++, data += ETH_GSTRING_LEN)
+		memcpy(data, ibmveth_stats[i].name, ETH_GSTRING_LEN);
+}
+
+static int ibmveth_get_stats_count(struct net_device *dev)
+{
+	return ARRAY_SIZE(ibmveth_stats);
+}
+
+static void ibmveth_get_ethtool_stats(struct net_device *dev,
+				      struct ethtool_stats *stats, u64 *data)
+{
+	int i;
+	struct ibmveth_adapter *adapter = dev->priv;
+
+	for (i = 0; i < ARRAY_SIZE(ibmveth_stats); i++)
+		data[i] = IBMVETH_GET_STAT(adapter, ibmveth_stats[i].offset);
+}
+
 static const struct ethtool_ops netdev_ethtool_ops = {
 	.get_drvinfo		= netdev_get_drvinfo,
 	.get_settings		= netdev_get_settings,

drivers/net/ibmveth.c static const struct ethtool_ops netdev_e · line 815

Quoted 8 lines
drivers/net/ibmveth.c renamed from drivers/net/ibmveth.c~ibmveth_ethtool_driver_stats +4 -1
@@ -761,7 +809,10 @@ static const struct ethtool_ops netdev_e
 	.get_rx_csum		= ibmveth_get_rx_csum,
 	.set_rx_csum		= ibmveth_set_rx_csum,
 	.get_tso			= ethtool_op_get_tso,
-	.get_ufo			= ethtool_op_get_ufo
+	.get_ufo			= ethtool_op_get_ufo,
+	.get_strings		= ibmveth_get_strings,
+	.get_stats_count		= ibmveth_get_stats_count,
+	.get_ethtool_stats	= ibmveth_get_ethtool_stats

ACK, modulo comma-at-end-of-initializer per previous email

author
#9 lore raw

PATCH 5/6 ibmveth: Remove dead frag processing code

In reply to Brian King

Removes dead frag processing code from ibmveth. Since NETIF_F_SG was not set, this code was never executed. Also, since the ibmveth interface can only handle 6 fragments, core networking code would need to be modified in order to efficiently enable this support.

---

 linux-2.6-bjking1/drivers/net/ibmveth.c |  100 +++++---------------------------
 linux-2.6-bjking1/drivers/net/ibmveth.h |    5 -
 2 files changed, 17 insertions(+), 88 deletions(-)
drivers/net/ibmveth.c renamed from drivers/net/ibmveth.c~ibmveth_remove_frag +17 -83
diff -puN drivers/net/ibmveth.c~ibmveth_remove_frag drivers/net/ibmveth.c
@@ -28,7 +28,6 @@
 /**************************************************************************/
 /*
   TODO:
-  - remove frag processing code - no longer needed
   - add support for sysfs
   - possibly remove procfs support
 */
@@ -127,9 +126,6 @@ struct ibmveth_stat ibmveth_stats[] = {
 	{ "replenish_add_buff_success", IBMVETH_STAT_OFF(replenish_add_buff_success) },
 	{ "rx_invalid_buffer", IBMVETH_STAT_OFF(rx_invalid_buffer) },
 	{ "rx_no_buffer", IBMVETH_STAT_OFF(rx_no_buffer) },
-	{ "tx_multidesc_send", IBMVETH_STAT_OFF(tx_multidesc_send) },
-	{ "tx_linearized", IBMVETH_STAT_OFF(tx_linearized) },
-	{ "tx_linearize_failed", IBMVETH_STAT_OFF(tx_linearize_failed) },
 	{ "tx_map_failed", IBMVETH_STAT_OFF(tx_map_failed) },
 	{ "tx_send_failed", IBMVETH_STAT_OFF(tx_send_failed) }
 };
@@ -825,9 +821,8 @@ static int ibmveth_ioctl(struct net_devi
 static int ibmveth_start_xmit(struct sk_buff *skb, struct net_device *netdev)
 {
 	struct ibmveth_adapter *adapter = netdev->priv;
-	union ibmveth_buf_desc desc[IbmVethMaxSendFrags];
+	union ibmveth_buf_desc desc;
 	unsigned long lpar_rc;
-	int nfrags = 0, curfrag;
 	unsigned long correlator;
 	unsigned long flags;
 	unsigned int retry_count;
@@ -837,25 +832,11 @@ static int ibmveth_start_xmit(struct sk_
 	unsigned int tx_send_failed = 0;
 	unsigned int tx_map_failed = 0;
 
-
-	if ((skb_shinfo(skb)->nr_frags + 1) > IbmVethMaxSendFrags) {
-		tx_dropped++;
-		goto out;
-	}
-
-	memset(&desc, 0, sizeof(desc));
-
-	/* nfrags = number of frags after the initial fragment */
-	nfrags = skb_shinfo(skb)->nr_frags;
-
-	if(nfrags)
-		adapter->tx_multidesc_send++;
-
-	/* map the initial fragment */
-	desc[0].fields.length  = nfrags ? skb->len - skb->data_len : skb->len;
-	desc[0].fields.address = dma_map_single(&adapter->vdev->dev, skb->data,
-					desc[0].fields.length, DMA_TO_DEVICE);
-	desc[0].fields.valid   = 1;
+	desc.desc = 0;
+	desc.fields.length  = skb->len;
+	desc.fields.address = dma_map_single(&adapter->vdev->dev, skb->data,
+					     desc.fields.length, DMA_TO_DEVICE);
+	desc.fields.valid   = 1;
 
 	if (skb->ip_summed == CHECKSUM_PARTIAL &&
 	    ip_hdr(skb)->protocol != IPPROTO_TCP && skb_checksum_help(skb)) {
@@ -867,75 +848,34 @@ static int ibmveth_start_xmit(struct sk_
 	if (skb->ip_summed == CHECKSUM_PARTIAL) {
 		unsigned char *buf = skb_transport_header(skb) + skb->csum_offset;
 
-		desc[0].fields.no_csum = 1;
-		desc[0].fields.csum_good = 1;
+		desc.fields.no_csum = 1;
+		desc.fields.csum_good = 1;
 
 		/* Need to zero out the checksum */
 		buf[0] = 0;
 		buf[1] = 0;
 	}
 
-	if(dma_mapping_error(desc[0].fields.address)) {
-		ibmveth_error_printk("tx: unable to map initial fragment\n");
+	if (dma_mapping_error(desc.fields.address)) {
+		ibmveth_error_printk("tx: unable to map xmit buffer\n");
 		tx_map_failed++;
 		tx_dropped++;
 		goto out;
 	}
 
-	curfrag = nfrags;
-
-	/* map fragments past the initial portion if there are any */
-	while(curfrag--) {
-		skb_frag_t *frag = &skb_shinfo(skb)->frags[curfrag];
-		desc[curfrag+1].fields.address
-			= dma_map_single(&adapter->vdev->dev,
-				page_address(frag->page) + frag->page_offset,
-				frag->size, DMA_TO_DEVICE);
-		desc[curfrag+1].fields.length = frag->size;
-		desc[curfrag+1].fields.valid  = 1;
-		if (skb->ip_summed == CHECKSUM_PARTIAL) {
-			desc[curfrag+1].fields.no_csum = 1;
-			desc[curfrag+1].fields.csum_good = 1;
-		}
-
-		if(dma_mapping_error(desc[curfrag+1].fields.address)) {
-			ibmveth_error_printk("tx: unable to map fragment %d\n", curfrag);
-			tx_map_failed++;
-			tx_dropped++;
-			/* Free all the mappings we just created */
-			while(curfrag < nfrags) {
-				dma_unmap_single(&adapter->vdev->dev,
-						 desc[curfrag+1].fields.address,
-						 desc[curfrag+1].fields.length,
-						 DMA_TO_DEVICE);
-				curfrag++;
-			}
-			goto out;
-		}
-	}
-
 	/* send the frame. Arbitrarily set retrycount to 1024 */
 	correlator = 0;
 	retry_count = 1024;
 	do {
 		lpar_rc = h_send_logical_lan(adapter->vdev->unit_address,
-					     desc[0].desc,
-					     desc[1].desc,
-					     desc[2].desc,
-					     desc[3].desc,
-					     desc[4].desc,
-					     desc[5].desc,
-					     correlator,
-					     &correlator);
+					     desc.desc, 0, 0, 0, 0, 0,
+					     correlator, &correlator);
 	} while ((lpar_rc == H_BUSY) && (retry_count--));
 
 	if(lpar_rc != H_SUCCESS && lpar_rc != H_DROPPED) {
-		int i;
 		ibmveth_error_printk("tx: h_send_logical_lan failed with rc=%ld\n", lpar_rc);
-		for(i = 0; i < 6; i++) {
-			ibmveth_error_printk("tx: desc[%i] valid=%d, len=%d, address=0x%d\n", i,
-					     desc[i].fields.valid, desc[i].fields.length, desc[i].fields.address);
-		}
+		ibmveth_error_printk("tx: valid=%d, len=%d, address=0x%08x\n",
+				     desc.fields.valid, desc.fields.length, desc.fields.address);
 		tx_send_failed++;
 		tx_dropped++;
 	} else {
@@ -944,11 +884,8 @@ static int ibmveth_start_xmit(struct sk_
 		netdev->trans_start = jiffies;
 	}
 
-	do {
-		dma_unmap_single(&adapter->vdev->dev,
-				desc[nfrags].fields.address,
-				desc[nfrags].fields.length, DMA_TO_DEVICE);
-	} while(--nfrags >= 0);
+	dma_unmap_single(&adapter->vdev->dev, desc.fields.address,
+			 desc.fields.length, DMA_TO_DEVICE);
 
 out:	spin_lock_irqsave(&adapter->stats_lock, flags);
 	adapter->stats.tx_dropped += tx_dropped;
@@ -1360,10 +1297,7 @@ static int ibmveth_seq_show(struct seq_f
 		   firmware_mac[3], firmware_mac[4], firmware_mac[5]);
 
 	seq_printf(seq, "\nAdapter Statistics:\n");
-	seq_printf(seq, "  TX:  skbuffs linearized:          %ld\n", adapter->tx_linearized);
-	seq_printf(seq, "       multi-descriptor sends:      %ld\n", adapter->tx_multidesc_send);
-	seq_printf(seq, "       skb_linearize failures:      %ld\n", adapter->tx_linearize_failed);
-	seq_printf(seq, "       vio_map_single failres:      %ld\n", adapter->tx_map_failed);
+	seq_printf(seq, "  TX:  vio_map_single failres:      %ld\n", adapter->tx_map_failed);
 	seq_printf(seq, "       send failures:               %ld\n", adapter->tx_send_failed);
 	seq_printf(seq, "  RX:  replenish task cycles:       %ld\n", adapter->replenish_task_cycles);
 	seq_printf(seq, "       alloc_skb_failures:          %ld\n", adapter->replenish_no_mem);
drivers/net/ibmveth.h renamed from drivers/net/ibmveth.h~ibmveth_remove_frag +0 -5
diff -puN drivers/net/ibmveth.h~ibmveth_remove_frag drivers/net/ibmveth.h
@@ -25,8 +25,6 @@
 #ifndef _IBMVETH_H
 #define _IBMVETH_H
 
-#define IbmVethMaxSendFrags 6
-
 /* constants for H_MULTICAST_CTRL */
 #define IbmVethMcastReceptionModifyBit     0x80000UL
 #define IbmVethMcastReceptionEnableBit     0x20000UL
@@ -146,9 +144,6 @@ struct ibmveth_adapter {
     u64 replenish_add_buff_success;
     u64 rx_invalid_buffer;
     u64 rx_no_buffer;
-    u64 tx_multidesc_send;
-    u64 tx_linearized;
-    u64 tx_linearize_failed;
     u64 tx_map_failed;
     u64 tx_send_failed;
     spinlock_t stats_lock;

_

author
#10 lore raw

PATCH 6/6 ibmveth: Remove use of bitfields

In reply to Brian King

Removes the use of bitfields from the ibmveth driver. This results in slightly smaller object code.

---

 linux-2.6-bjking1/drivers/net/ibmveth.c |   90 ++++++++++++++++----------------
 linux-2.6-bjking1/drivers/net/ibmveth.h |   56 ++++++++-----------
 2 files changed, 68 insertions(+), 78 deletions(-)
drivers/net/ibmveth.h renamed from drivers/net/ibmveth.h~ibmveth_nobitfields +23 -33
diff -puN drivers/net/ibmveth.h~ibmveth_nobitfields drivers/net/ibmveth.h
@@ -39,6 +39,12 @@
 #define IbmVethMcastRemoveFilter     0x2UL
 #define IbmVethMcastClearFilterTable 0x3UL
 
+#define IBMVETH_ILLAN_PADDED_PKT_CSUM	0x0000000000002000ULL
+#define IBMVETH_ILLAN_TRUNK_PRI_MASK	0x0000000000000F00ULL
+#define IBMVETH_ILLAN_IPV6_TCP_CSUM		0x0000000000000004ULL
+#define IBMVETH_ILLAN_IPV4_TCP_CSUM		0x0000000000000002ULL
+#define IBMVETH_ILLAN_ACTIVE_TRUNK		0x0000000000000001ULL
+
 /* hcall macros */
 #define h_register_logical_lan(ua, buflst, rxq, fltlst, mac) \
   plpar_hcall_norets(H_REGISTER_LOGICAL_LAN, ua, buflst, rxq, fltlst, mac)
@@ -150,13 +156,13 @@ struct ibmveth_adapter {
 };
 
 struct ibmveth_buf_desc_fields {
-    u32 valid : 1;
-    u32 toggle : 1;
-    u32 reserved : 4;
-    u32 no_csum : 1;
-    u32 csum_good : 1;
-    u32 length : 24;
-    u32 address;
+	u32 flags_len;
+#define IBMVETH_BUF_VALID	0x80000000
+#define IBMVETH_BUF_TOGGLE	0x40000000
+#define IBMVETH_BUF_NO_CSUM	0x02000000
+#define IBMVETH_BUF_CSUM_GOOD	0x01000000
+#define IBMVETH_BUF_LEN_MASK	0x00FFFFFF
+	u32 address;
 };
 
 union ibmveth_buf_desc {
@@ -164,33 +170,17 @@ union ibmveth_buf_desc {
     struct ibmveth_buf_desc_fields fields;
 };
 
-struct ibmveth_illan_attributes_fields {
-	u32 reserved;
-	u32 reserved2 : 18;
-	u32 csum_offload_padded_pkt_support : 1;
-	u32 reserved3 : 1;
-	u32 trunk_priority : 4;
-	u32 reserved4 : 5;
-	u32 tcp_csum_offload_ipv6 : 1;
-	u32 tcp_csum_offload_ipv4 : 1;
-	u32 active_trunk : 1;
-};
-
-union ibmveth_illan_attributes {
-	u64 desc;
-	struct ibmveth_illan_attributes_fields fields;
-};
-
 struct ibmveth_rx_q_entry {
-    u16 toggle : 1;
-    u16 valid : 1;
-    u16 reserved : 4;
-    u16 no_csum : 1;
-    u16 csum_good : 1;
-    u16 reserved2 : 8;
-    u16 offset;
-    u32 length;
-    u64 correlator;
+	u32 flags_off;
+#define IBMVETH_RXQ_TOGGLE		0x80000000
+#define IBMVETH_RXQ_TOGGLE_SHIFT	31
+#define IBMVETH_RXQ_VALID		0x40000000
+#define IBMVETH_RXQ_NO_CSUM		0x02000000
+#define IBMVETH_RXQ_CSUM_GOOD		0x01000000
+#define IBMVETH_RXQ_OFF_MASK		0x0000FFFF
+
+	u32 length;
+	u64 correlator;
 };
 
 #endif /* _IBMVETH_H */
drivers/net/ibmveth.c renamed from drivers/net/ibmveth.c~ibmveth_nobitfields +45 -45

202 lines not on this page · raw message

_

#11 lore raw

Re: [PATCH 6/6] ibmveth: Remove use of bitfields

Quoted 9 lines · Brian King

Removes the use of bitfields from the ibmveth driver. This results in slightly smaller object code.

---

 linux-2.6-bjking1/drivers/net/ibmveth.c |   90 ++++++++++++++++----------------
 linux-2.6-bjking1/drivers/net/ibmveth.h |   56 ++++++++-----------
 2 files changed, 68 insertions(+), 78 deletions(-)

strong ACK :)

Though I also encourage you to avoid #defines for named constants, in favor of

enum {
	IBMVETH_BUF_VALID	= (1U << 31),
	IBMVETH_BUF_TOGGLE	= (1U << 30),
	IBMVETH_BUF_NO_CSUM	= (1U << 25),
	IBMVETH_BUF_CSUM_GOOD	= (1U << 24),
	IBMVETH_BUF_LEN_MASK	= 0x00FFFFFF,
};

This illustrates:

1) The "1 << n" notation is FAR easier to read and compare with data sheets. You're just adding to the trouble by requiring the reviewer's brain to convert hex numbers to bits, even if most engineers can do this in their sleep.

2) The named constants are available to the C compiler, which is more friendly to debuggers. It also supplies type information to the C compiler.

3) Similar to #2, wading through C pre-processor output is much easier when the symbols don't disappear.

These are recommendations, not requirements, but I've found these techniques superior to cpp in many other drivers.

Jeff

#12 lore raw
In reply to Brian King
Quoted 9 lines · Brian King

This patchset enables TCP checksum offload support for IPV4 on ibmveth. This completely eliminates the generation and checking of the checksum for packets that are completely virtual and never touch a physical network. A simple TCP_STREAM netperf run on a virtual network with maximum mtu set yielded a ~30% increase in throughput. This feature is enabled by default on systems that support it, but can be disabled with a module option.

ACK, but does not apply to current netdev-2.6.git#upstream-fixes. Request resend after the ibmveth fixes hit mainline (24 hours or so after push, I suppose)