[PATCH][E1000E] some cleanups

[PATCH][E1000E] some cleanups

[jamal]

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

Auke,

heres part of something i promised. 
I couldnt do any packet testing on because 82571EB is disabled in the
driver. I uncommented the code out in the table, but the best i could
get was the module loading, some probing and some sysfs renaming
failures (probably a debianism); the machine access is intermittent, so
thats as far as i could go. In any case, you probably have a good reason
for disabling that chip. So, heres the patch, the burden of testing now
falls on you ;->
Once you have 82571EB on and kicking, my next steps are to kill LLTX
then add batching on top.
BTW, since this driver is just for PCIE, would you take a similar patch
for non-PCIE e1000?

comment:
There used to be an "mmiowb()" call right after the dma wake which is
gone now; is this unneeded with pcie? I have restored it, look for the
"XXX".

cheers,
jamal

[-- Attachment #2: e1000e-p1 --]
[-- Type: text/plain, Size: 10445 bytes --]

[E1000E] some cleanups
This patch makes the xmit path code a lot more readable and reusable.
preps for removing LLTX.

Signed-off-by: Jamal Hadi Salim <hadi@cyberus.ca>

---
commit ad63c288ce980907f68d94d5faac08625c0b1782
tree 296a6da371b98c6488c544a8910941ea6d8c18a8
parent 7f5d0afdff875b2c4957031f8934741aefe257cc
author Jamal Hadi Salim <hadi@cyberus.ca> Sun, 30 Sep 2007 13:26:00 -0400
committer Jamal Hadi Salim <hadi@cyberus.ca> Sun, 30 Sep 2007 13:26:00 -0400

 drivers/net/e1000e/netdev.c |  187 +++++++++++++++++++++++++++----------------
 1 files changed, 119 insertions(+), 68 deletions(-)

diff --git a/drivers/net/e1000e/netdev.c b/drivers/net/e1000e/netdev.c
index 4a21d7d..5043504 100644
--- a/drivers/net/e1000e/netdev.c
+++ b/drivers/net/e1000e/netdev.c
@@ -3076,6 +3076,18 @@ link_up:
 #define E1000_TX_FLAGS_VLAN_MASK	0xffff0000
 #define E1000_TX_FLAGS_VLAN_SHIFT	16
 
+struct e1000_tx_cbdata {
+	int count;
+	unsigned int max_per_txd;
+	unsigned int nr_frags;
+	unsigned int mss;
+	unsigned int tx_flags;
+};
+
+#define E1000_SKB_CB(__skb) ((struct e1000_tx_cbdata *)&((__skb)->cb[8]))
+#define NETDEV_TX_DROPPED -5
+#define TXD_USE_COUNT(S, X) (((S) >> (X)) + 1)
+
 static int e1000_tso(struct e1000_adapter *adapter,
 		     struct sk_buff *skb)
 {
@@ -3194,8 +3206,7 @@ static bool e1000_tx_csum(struct e1000_adapter *adapter, struct sk_buff *skb)
 
 static int e1000_tx_map(struct e1000_adapter *adapter,
 			struct sk_buff *skb, unsigned int first,
-			unsigned int max_per_txd, unsigned int nr_frags,
-			unsigned int mss)
+			struct e1000_tx_cbdata *cb)
 {
 	struct e1000_ring *tx_ring = adapter->tx_ring;
 	struct e1000_buffer *buffer_info;
@@ -3207,11 +3218,11 @@ static int e1000_tx_map(struct e1000_adapter *adapter,
 
 	while (len) {
 		buffer_info = &tx_ring->buffer_info[i];
-		size = min(len, max_per_txd);
+		size = min(len, cb->max_per_txd);
 
 		/* Workaround for premature desc write-backs
 		 * in TSO mode.  Append 4-byte sentinel desc */
-		if (mss && !nr_frags && size == len && size > 8)
+		if (cb->mss && !cb->nr_frags && size == len && size > 8)
 			size -= 4;
 
 		buffer_info->length = size;
@@ -3237,7 +3248,7 @@ static int e1000_tx_map(struct e1000_adapter *adapter,
 			i = 0;
 	}
 
-	for (f = 0; f < nr_frags; f++) {
+	for (f = 0; f < cb->nr_frags; f++) {
 		struct skb_frag_struct *frag;
 
 		frag = &skb_shinfo(skb)->frags[f];
@@ -3246,10 +3257,11 @@ static int e1000_tx_map(struct e1000_adapter *adapter,
 
 		while (len) {
 			buffer_info = &tx_ring->buffer_info[i];
-			size = min(len, max_per_txd);
+			size = min(len, cb->max_per_txd);
 			/* Workaround for premature desc write-backs
 			 * in TSO mode.  Append 4-byte sentinel desc */
-			if (mss && f == (nr_frags-1) && size == len && size > 8)
+			if (cb->mss && f == (cb->nr_frags-1) &&
+			    size == len && size > 8)
 				size -= 4;
 
 			buffer_info->length = size;
@@ -3334,18 +3346,7 @@ static void e1000_tx_queue(struct e1000_adapter *adapter,
 	}
 
 	tx_desc->lower.data |= cpu_to_le32(adapter->txd_cmd);
-
-	/* Force memory writes to complete before letting h/w
-	 * know there are new descriptors to fetch.  (Only
-	 * applicable for weak-ordered memory model archs,
-	 * such as IA-64). */
-	wmb();
-
 	tx_ring->next_to_use = i;
-	writel(i, adapter->hw.hw_addr + tx_ring->tail);
-	/* we need this if more than one processor can write to our tail
-	 * at a time, it synchronizes IO on IA64/Altix systems */
-	mmiowb();
 }
 
 #define MINIMUM_DHCP_PACKET_SIZE 282
@@ -3417,45 +3418,54 @@ static int e1000_maybe_stop_tx(struct net_device *netdev, int size)
 	return __e1000_maybe_stop_tx(netdev, size);
 }
 
-#define TXD_USE_COUNT(S, X) (((S) >> (X)) + 1 )
-static int e1000_xmit_frame(struct sk_buff *skb, struct net_device *netdev)
+static void
+e1000_complete_tx(struct net_device *netdev)
 {
 	struct e1000_adapter *adapter = netdev_priv(netdev);
 	struct e1000_ring *tx_ring = adapter->tx_ring;
-	unsigned int first;
-	unsigned int max_per_txd = E1000_MAX_PER_TXD;
+	/* Force memory writes to complete before letting h/w
+	 * know there are new descriptors to fetch.  (Only
+	 * applicable for weak-ordered memory model archs,
+	 * such as IA-64). */
+	wmb();
+	writel(tx_ring->next_to_use, adapter->hw.hw_addr + tx_ring->tail);
+	/* XXX: we need this if more than one processor can write to
+	 * our tail at a time, it syncronizes IO on IA64/Altix systems */
+	mmiowb();
+	netdev->trans_start = jiffies;
+}
+
+static int e1000_prep_frame(struct sk_buff *skb, struct net_device *netdev)
+{
+	struct e1000_adapter *adapter = netdev_priv(netdev);
 	unsigned int max_txd_pwr = E1000_MAX_TXD_PWR;
-	unsigned int tx_flags = 0;
 	unsigned int len = skb->len;
-	unsigned long irq_flags;
-	unsigned int nr_frags = 0;
-	unsigned int mss = 0;
-	int count = 0;
-	int tso;
+	struct e1000_tx_cbdata *cb = E1000_SKB_CB(skb);
 	unsigned int f;
-	len -= skb->data_len;
 
-	if (test_bit(__E1000_DOWN, &adapter->state)) {
-		dev_kfree_skb_any(skb);
-		return NETDEV_TX_OK;
-	}
+	cb->nr_frags = 0;
+	cb->mss = 0;
+	cb->count = 0;
+	cb->max_per_txd = E1000_MAX_PER_TXD;
+
+	len -= skb->data_len;
 
 	if (skb->len <= 0) {
 		dev_kfree_skb_any(skb);
-		return NETDEV_TX_OK;
+		return NETDEV_TX_DROPPED;
 	}
 
-	mss = skb_shinfo(skb)->gso_size;
+	cb->mss = skb_shinfo(skb)->gso_size;
 	/* The controller does a simple calculation to
 	 * make sure there is enough room in the FIFO before
 	 * initiating the DMA for each buffer.  The calc is:
 	 * 4 = ceil(buffer len/mss).  To make sure we don't
 	 * overrun the FIFO, adjust the max buffer len if mss
 	 * drops. */
-	if (mss) {
+	if (cb->mss) {
 		u8 hdr_len;
-		max_per_txd = min(mss << 2, max_per_txd);
-		max_txd_pwr = fls(max_per_txd) - 1;
+		cb->max_per_txd = min(cb->mss << 2, cb->max_per_txd);
+		max_txd_pwr = fls(cb->max_per_txd) - 1;
 
 		/* TSO Workaround for 82571/2/3 Controllers -- if skb->data
 		* points to just header, pull a few bytes of payload from
@@ -3469,80 +3479,121 @@ static int e1000_xmit_frame(struct sk_buff *skb, struct net_device *netdev)
 				ndev_err(netdev,
 					 "__pskb_pull_tail failed.\n");
 				dev_kfree_skb_any(skb);
-				return NETDEV_TX_OK;
+				return NETDEV_TX_DROPPED;
 			}
 			len = skb->len - skb->data_len;
 		}
 	}
 
 	/* reserve a descriptor for the offload context */
-	if ((mss) || (skb->ip_summed == CHECKSUM_PARTIAL))
-		count++;
-	count++;
+	if ((cb->mss) || (skb->ip_summed == CHECKSUM_PARTIAL))
+		cb->count++;
+	cb->count++;
 
-	count += TXD_USE_COUNT(len, max_txd_pwr);
+	cb->count += TXD_USE_COUNT(len, max_txd_pwr);
 
-	nr_frags = skb_shinfo(skb)->nr_frags;
-	for (f = 0; f < nr_frags; f++)
-		count += TXD_USE_COUNT(skb_shinfo(skb)->frags[f].size,
+	cb->nr_frags = skb_shinfo(skb)->nr_frags;
+	for (f = 0; f < cb->nr_frags; f++)
+		cb->count += TXD_USE_COUNT(skb_shinfo(skb)->frags[f].size,
 				       max_txd_pwr);
 
 	if (adapter->hw.mac.tx_pkt_filtering)
 		e1000_transfer_dhcp_info(adapter, skb);
 
-	if (!spin_trylock_irqsave(&adapter->tx_queue_lock, irq_flags))
-		/* Collision - tell upper layer to requeue */
-		return NETDEV_TX_LOCKED;
+	if (adapter->vlgrp && vlan_tx_tag_present(skb)) {
+		cb->tx_flags |= E1000_TX_FLAGS_VLAN;
+		cb->tx_flags |= (vlan_tx_tag_get(skb) <<
+				E1000_TX_FLAGS_VLAN_SHIFT);
+	}
+
+	return NETDEV_TX_OK;
+}
+
+static int e1000_queue_frame(struct sk_buff *skb, struct net_device *netdev)
+{
+	struct e1000_adapter *adapter = netdev_priv(netdev);
+	struct e1000_ring *tx_ring = adapter->tx_ring;
+	unsigned int first;
+	int tso;
+	struct e1000_tx_cbdata *cb = E1000_SKB_CB(skb);
+
 
 	/* need: count + 2 desc gap to keep tail from touching
 	 * head, otherwise try next time */
-	if (e1000_maybe_stop_tx(netdev, count + 2)) {
-		spin_unlock_irqrestore(&adapter->tx_queue_lock, irq_flags);
+	if (e1000_maybe_stop_tx(netdev, cb->count + 2))
 		return NETDEV_TX_BUSY;
-	}
-
-	if (adapter->vlgrp && vlan_tx_tag_present(skb)) {
-		tx_flags |= E1000_TX_FLAGS_VLAN;
-		tx_flags |= (vlan_tx_tag_get(skb) << E1000_TX_FLAGS_VLAN_SHIFT);
-	}
 
 	first = tx_ring->next_to_use;
 
 	tso = e1000_tso(adapter, skb);
 	if (tso < 0) {
 		dev_kfree_skb_any(skb);
-		spin_unlock_irqrestore(&adapter->tx_queue_lock, irq_flags);
-		return NETDEV_TX_OK;
+		return NETDEV_TX_DROPPED;
 	}
 
 	if (tso)
-		tx_flags |= E1000_TX_FLAGS_TSO;
+		cb->tx_flags |= E1000_TX_FLAGS_TSO;
 	else if (e1000_tx_csum(adapter, skb))
-		tx_flags |= E1000_TX_FLAGS_CSUM;
+		cb->tx_flags |= E1000_TX_FLAGS_CSUM;
 
 	/* Old method was to assume IPv4 packet by default if TSO was enabled.
 	 * 82571 hardware supports TSO capabilities for IPv6 as well...
 	 * no longer assume, we must. */
 	if (skb->protocol == htons(ETH_P_IP))
-		tx_flags |= E1000_TX_FLAGS_IPV4;
+		cb->tx_flags |= E1000_TX_FLAGS_IPV4;
 
-	count = e1000_tx_map(adapter, skb, first, max_per_txd, nr_frags, mss);
-	if (count < 0) {
+	cb->count = e1000_tx_map(adapter, skb, first, cb);
+	if (cb->count < 0) {
 		/* handle pci_map_single() error in e1000_tx_map */
 		dev_kfree_skb_any(skb);
-		spin_unlock_irqrestore(&adapter->tx_queue_lock, irq_flags);
+		return NETDEV_TX_DROPPED;
+	}
+
+	e1000_tx_queue(adapter, cb->tx_flags, cb->count);
+
+	return NETDEV_TX_OK;
+}
+
+static int e1000_xmit_frame(struct sk_buff *skb, struct net_device *netdev)
+{
+	int ret = NETDEV_TX_OK;
+	struct e1000_adapter *adapter = netdev_priv(netdev);
+	unsigned long irq_flags;
+	struct e1000_tx_cbdata *cb = E1000_SKB_CB(skb);
+
+	ret = e1000_prep_frame(skb, netdev);
+	if (unlikely(ret != NETDEV_TX_OK))
+		return NETDEV_TX_OK;
+
+	if (test_bit(__E1000_DOWN, &adapter->state)) {
+		dev_kfree_skb_any(skb);
 		return NETDEV_TX_OK;
 	}
 
-	e1000_tx_queue(adapter, tx_flags, count);
+	if (!spin_trylock_irqsave(&adapter->tx_queue_lock, irq_flags))
+		/* Collision - tell upper layer to requeue */
+		return NETDEV_TX_LOCKED;
 
-	netdev->trans_start = jiffies;
+	/* need: count + 2 desc gap to keep tail from touching
+	 * head, otherwise try next time */
+	if (e1000_maybe_stop_tx(netdev, cb->count + 2)) {
+		spin_unlock_irqrestore(&adapter->tx_queue_lock, irq_flags);
+		return NETDEV_TX_BUSY;
+	}
+
+	ret = e1000_queue_frame(skb, netdev);
+
+	if (ret == NETDEV_TX_OK)
+		e1000_complete_tx(netdev);
+
+	if (ret == NETDEV_TX_DROPPED)
+		ret = NETDEV_TX_OK;
 
 	/* Make sure there is space in the ring for the next send. */
 	e1000_maybe_stop_tx(netdev, MAX_SKB_FRAGS + 2);
 
 	spin_unlock_irqrestore(&adapter->tx_queue_lock, irq_flags);
-	return NETDEV_TX_OK;
+	return ret;
 }
 
 /**

Re: [PATCH][E1000E] some cleanups

[Kok, Auke]

jamal wrote:
> Auke,
> 
> heres part of something i promised. 
> I couldnt do any packet testing on because 82571EB is disabled in the
> driver. I uncommented the code out in the table, but the best i could
> get was the module loading, some probing and some sysfs renaming
> failures (probably a debianism); the machine access is intermittent, so
> thats as far as i could go. In any case, you probably have a good reason
> for disabling that chip. So, heres the patch, the burden of testing now
> falls on you ;->

no, all the hardware that is commented should work just fine. I tested this driver
on 82571, 82573 and ich8/ich9 - extensively.

the reason that we disable them is that we're going to migrate devices over in
batches. At introduction we'll support ich9, afterwards we'll drop in the IDs of
the other groups of silicon.

> Once you have 82571EB on and kicking, my next steps are to kill LLTX
> then add batching on top.
> BTW, since this driver is just for PCIE, would you take a similar patch
> for non-PCIE e1000?

if it's a fix, yes.

> comment:
> There used to be an "mmiowb()" call right after the dma wake which is
> gone now; is this unneeded with pcie? I have restored it, look for the
> "XXX".


thanks, I'll go and look at this in depth in the coming weeks.

Auke

Re: [PATCH][E1000E] some cleanups

[jamal]

On Sun, 2007-30-09 at 11:16 -0700, Kok, Auke wrote:

> no, all the hardware that is commented should work just fine. I tested this driver
> on 82571, 82573 and ich8/ich9 - extensively.

Something else is wrong then. Can you just uncomment the 82571EB bits in
Dave's net-2.6.24 and just send a ping? If it works, let me know what
you did so i can test next time i get a chance.

> the reason that we disable them is that we're going to migrate devices over in
> batches. At introduction we'll support ich9, afterwards we'll drop in the IDs of
> the other groups of silicon.

Turn them on if you want people to start using that driver.

> > Once you have 82571EB on and kicking, my next steps are to kill LLTX
> > then add batching on top.
> > BTW, since this driver is just for PCIE, would you take a similar patch
> > for non-PCIE e1000?
> 
> if it's a fix, yes.

It just makes it easier to kill LLTX. If you consider killing LLTX
risky, then i will focus on e1000e.

cheers,
jamal

Re: [PATCH][E1000E] some cleanups

[Jeff Garzik]

jamal wrote:
> On Sun, 2007-30-09 at 11:16 -0700, Kok, Auke wrote:
> 
>> no, all the hardware that is commented should work just fine. I tested this driver
>> on 82571, 82573 and ich8/ich9 - extensively.
> 
> Something else is wrong then. Can you just uncomment the 82571EB bits in
> Dave's net-2.6.24 and just send a ping? If it works, let me know what
> you did so i can test next time i get a chance.
> 
>> the reason that we disable them is that we're going to migrate devices over in
>> batches. At introduction we'll support ich9, afterwards we'll drop in the IDs of
>> the other groups of silicon.
> 
> Turn them on if you want people to start using that driver.

Gotta wait a bit, otherwise we have confusion and a bit of breakage from 
two drivers with the same PCI IDs.

	Jeff

Re: [PATCH][E1000E] some cleanups

[jamal]

On Sun, 2007-30-09 at 15:23 -0400, Jeff Garzik wrote:

> Gotta wait a bit, otherwise we have confusion and a bit of breakage from 
> two drivers with the same PCI IDs.

ah, ok ;-> 
When i was testing i compiled out e1000. I am willing to totaly migrate
to e1000e, since major machine i have access to has PCIE. Auke, just let
me know what you need to do other than uncommenting the table entries
and i will leave you alone ;->

cheers,
jamal

Re: [PATCH][E1000E] some cleanups

[Kok, Auke]

jamal wrote:
> On Sun, 2007-30-09 at 15:23 -0400, Jeff Garzik wrote:
> 
>> Gotta wait a bit, otherwise we have confusion and a bit of breakage from 
>> two drivers with the same PCI IDs.
> 
> ah, ok ;-> 
> When i was testing i compiled out e1000. I am willing to totaly migrate
> to e1000e, since major machine i have access to has PCIE. Auke, just let
> me know what you need to do other than uncommenting the table entries
> and i will leave you alone ;->

the IDs are the only thing needed to enable all pci-e e1000 hardware.

by all means we need to have guys like you and Jeff test the commented IDs! I've
been doing this myself and the e1000e driver goes to our labs for a period of
testing from next week. Unfortunately they don't know how to break it that good as
some of you guys ;)

I'll personally try to get an 82571EB tested on monday.

Auke

Re: [PATCH][E1000E] some cleanups

[jamal]

On Sun, 2007-30-09 at 18:59 -0700, Kok, Auke wrote:

> the IDs are the only thing needed to enable all pci-e e1000 hardware.

I'll give it a whirl in the next few days. It failed as a module (with
e1000 compiled out), i will try to compile it in. I have access to the
hardware in quiet times - so it may be the weekend.

> by all means we need to have guys like you and Jeff test the commented IDs! I've
> been doing this myself and the e1000e driver goes to our labs for a period of
> testing from next week. Unfortunately they don't know how to break it that good as
> some of you guys ;)
> 
> I'll personally try to get an 82571EB tested on monday.

How did that go?

cheers,
jamal

Re: [PATCH][E1000E] some cleanups

[Kok, Auke]

jamal wrote:
> On Sun, 2007-30-09 at 18:59 -0700, Kok, Auke wrote:
> 
>> the IDs are the only thing needed to enable all pci-e e1000 hardware.
> 
> I'll give it a whirl in the next few days. It failed as a module (with
> e1000 compiled out), i will try to compile it in. I have access to the
> hardware in quiet times - so it may be the weekend.
> 
>> by all means we need to have guys like you and Jeff test the commented IDs! I've
>> been doing this myself and the e1000e driver goes to our labs for a period of
>> testing from next week. Unfortunately they don't know how to break it that good as
>> some of you guys ;)
>>
>> I'll personally try to get an 82571EB tested on monday.
> 
> How did that go?

Emil just ran overnight testing on a 82571, a 81572, a ich9 and an esb2 onboard
LOM. All passed traffic OK. There was one issue/unconfirmed bug with jumbo frames
that I'm currently looking at, but nothing at normal MTU's.

So, you should be just fine using 82571's with e1000e for now.

Auke

Re: [PATCH][E1000E] some cleanups

[Kok, Auke]

jamal wrote:
> Auke,
> 
> heres part of something i promised. 
> I couldnt do any packet testing on because 82571EB is disabled in the
> driver. I uncommented the code out in the table, but the best i could
> get was the module loading, some probing and some sysfs renaming
> failures (probably a debianism); the machine access is intermittent, so
> thats as far as i could go. In any case, you probably have a good reason
> for disabling that chip. So, heres the patch, the burden of testing now
> falls on you ;->


the description of this patch is rather misleading, and the title certainly too.
Can you resend this with a bit more elaborate explanation as to why the cb code is
relevant to use here? Not only do I need to understand this, but others might want
to as well later on ;)

Cheers,

Auke

Re: [PATCH][E1000E] some cleanups

[jamal]

On Tue, 2007-02-10 at 10:43 -0700, Kok, Auke wrote:

> the description of this patch is rather misleading, and the title certainly too.

That was fast - you said weeks, not days;->

> Can you resend this with a bit more elaborate explanation as to why the cb code is
> relevant to use here? Not only do I need to understand this, but others might want
> to as well later on ;)

I am probably repeating something youve seen/know already.
The cleanup is to break up the code so it is functionally more readable
from a perspective of the 4 distinct parts in ->hard_start_xmit():

a) packet formatting (example: vlan, mss, descriptor counting, etc.)
b) chip-specific formatting
c) enqueueing the packet on a DMA ring
d) IO operations to complete packet transmit, tell DMA engine to chew
on, tx completion interrupts, set last tx time, etc.

Each of those steps sitting in different functions accumulates state
that is used in the next steps. cb stores this state because it a
scratchpad the driver owns. You could create some other structure and
pass it around the iteration, but why waste more bytes.

I could stop there with the explanation, but let me go on .. ;->

>From a secondary angle, remember i am pulling these patches out of my
batching work. Thats how we started this discussion ;-> I would like,
once converted the driver to remove LLTX, to do #a without holding the
tx lock. This stands on its own even without batching. Then of course,
once all this is in such good shape it makes it easier to add the
batching code because i could reuse the now functionalized steps.
I hope that provides reasonable and good explanation ;->

cheers,
jamal

[PATCH][E1000E] some cleanups

[jamal]

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


Ok, here you go; the explanation is below. This is from net-2.6.24 of
early this AM. I saw a patch you posted that is derived from Krishna;
although it hasnt showed up in the tree - i have considered those
changes and this patch adds a little more optimization in case of
errors.

I will send you a patch to kill LLTX the sooner this shows up somewhere.

cheers,
jamal

On Wed, 2007-03-10 at 09:18 -0400, jamal wrote:

> The cleanup is to break up the code so it is functionally more readable
> from a perspective of the 4 distinct parts in ->hard_start_xmit():
> 
> a) packet formatting (example: vlan, mss, descriptor counting, etc.)
> b) chip-specific formatting
> c) enqueueing the packet on a DMA ring
> d) IO operations to complete packet transmit, tell DMA engine to chew
> on, tx completion interrupts, set last tx time, etc.
> 
> Each of those steps sitting in different functions accumulates state
> that is used in the next steps. cb stores this state because it a
> scratchpad the driver owns. You could create some other structure and
> pass it around the iteration, but why waste more bytes.


[-- Attachment #2: e1000e-p1 --]
[-- Type: text/x-patch, Size: 6356 bytes --]

diff --git a/drivers/net/e1000e/netdev.c b/drivers/net/e1000e/netdev.c
index 4a21d7d..26d35bb 100644
--- a/drivers/net/e1000e/netdev.c
+++ b/drivers/net/e1000e/netdev.c
@@ -3417,51 +3417,60 @@ static int e1000_maybe_stop_tx(struct net_device *netdev, int size)
 	return __e1000_maybe_stop_tx(netdev, size);
 }
 
+/* This structure cannot be larger than 40B given it uses skb->cb[8] */
+struct e1000_tx_cbdata {
+	int count;
+	unsigned int max_per_txd;
+	unsigned int nr_frags;
+	unsigned int mss;
+	unsigned int tx_flags;
+};
+
+/* we avoid cb[0] because it is used by VLAN cookie
+ * struct vlan_skb_tx_cookie
+*/
+#define E1000_SKB_CB(__skb)       ((struct e1000_tx_cbdata *)&((__skb)->cb[8]))
+#define NETDEV_TX_DROPPED       -5
 #define TXD_USE_COUNT(S, X) (((S) >> (X)) + 1 )
-static int e1000_xmit_frame(struct sk_buff *skb, struct net_device *netdev)
+
+static int e1000_prep_frame(struct sk_buff *skb, struct net_device *netdev)
 {
 	struct e1000_adapter *adapter = netdev_priv(netdev);
-	struct e1000_ring *tx_ring = adapter->tx_ring;
-	unsigned int first;
-	unsigned int max_per_txd = E1000_MAX_PER_TXD;
-	unsigned int max_txd_pwr = E1000_MAX_TXD_PWR;
-	unsigned int tx_flags = 0;
 	unsigned int len = skb->len;
-	unsigned long irq_flags;
-	unsigned int nr_frags = 0;
-	unsigned int mss = 0;
-	int count = 0;
-	int tso;
+	unsigned int max_txd_pwr; 
+	struct e1000_tx_cbdata *cb;
 	unsigned int f;
-	len -= skb->data_len;
-
-	if (test_bit(__E1000_DOWN, &adapter->state)) {
-		dev_kfree_skb_any(skb);
-		return NETDEV_TX_OK;
-	}
 
 	if (skb->len <= 0) {
 		dev_kfree_skb_any(skb);
-		return NETDEV_TX_OK;
+		return NETDEV_TX_DROPPED;
 	}
 
-	mss = skb_shinfo(skb)->gso_size;
+	max_txd_pwr = E1000_MAX_TXD_PWR;
+	cb = E1000_SKB_CB(skb);
+	cb->nr_frags = 0;
+	cb->mss = 0;
+	cb->count = 0;
+	cb->max_per_txd = E1000_MAX_PER_TXD;
+
+	len -= skb->data_len;
+	cb->mss = skb_shinfo(skb)->gso_size;
 	/* The controller does a simple calculation to
 	 * make sure there is enough room in the FIFO before
 	 * initiating the DMA for each buffer.  The calc is:
 	 * 4 = ceil(buffer len/mss).  To make sure we don't
 	 * overrun the FIFO, adjust the max buffer len if mss
 	 * drops. */
-	if (mss) {
+	if (cb->mss) {
 		u8 hdr_len;
-		max_per_txd = min(mss << 2, max_per_txd);
-		max_txd_pwr = fls(max_per_txd) - 1;
+		cb->max_per_txd = min(cb->mss << 2, cb->max_per_txd);
+		max_txd_pwr = fls(cb->max_per_txd) - 1;
 
 		/* TSO Workaround for 82571/2/3 Controllers -- if skb->data
 		* points to just header, pull a few bytes of payload from
 		* frags into skb->data */
 		hdr_len = skb_transport_offset(skb) + tcp_hdrlen(skb);
-		if (skb->data_len && (hdr_len == (skb->len - skb->data_len))) {
+		if (skb->data_len && (hdr_len == len )) {
 			unsigned int pull_size;
 
 			pull_size = min((unsigned int)4, skb->data_len);
@@ -3469,43 +3478,66 @@ static int e1000_xmit_frame(struct sk_buff *skb, struct net_device *netdev)
 				ndev_err(netdev,
 					 "__pskb_pull_tail failed.\n");
 				dev_kfree_skb_any(skb);
-				return NETDEV_TX_OK;
+				return NETDEV_TX_DROPPED;
 			}
 			len = skb->len - skb->data_len;
 		}
 	}
 
 	/* reserve a descriptor for the offload context */
-	if ((mss) || (skb->ip_summed == CHECKSUM_PARTIAL))
-		count++;
-	count++;
+	if ((cb->mss) || (skb->ip_summed == CHECKSUM_PARTIAL))
+		cb->count++;
+	cb->count++;
 
-	count += TXD_USE_COUNT(len, max_txd_pwr);
+	cb->count += TXD_USE_COUNT(len, max_txd_pwr);
 
-	nr_frags = skb_shinfo(skb)->nr_frags;
-	for (f = 0; f < nr_frags; f++)
-		count += TXD_USE_COUNT(skb_shinfo(skb)->frags[f].size,
+	cb->nr_frags = skb_shinfo(skb)->nr_frags;
+	for (f = 0; f < cb->nr_frags; f++)
+		cb->count += TXD_USE_COUNT(skb_shinfo(skb)->frags[f].size,
 				       max_txd_pwr);
 
 	if (adapter->hw.mac.tx_pkt_filtering)
 		e1000_transfer_dhcp_info(adapter, skb);
 
+	if (adapter->vlgrp && vlan_tx_tag_present(skb)) {
+		cb->tx_flags |= E1000_TX_FLAGS_VLAN;
+		cb->tx_flags |= (vlan_tx_tag_get(skb) << E1000_TX_FLAGS_VLAN_SHIFT);
+	}
+
+	return NETDEV_TX_OK;
+}
+
+static int e1000_xmit_frame(struct sk_buff *skb, struct net_device *netdev)
+{
+	int ret = NETDEV_TX_OK;
+	struct e1000_adapter *adapter = netdev_priv(netdev);
+	struct e1000_ring *tx_ring = adapter->tx_ring;
+	unsigned int first;
+	unsigned long irq_flags;
+	int tso;
+	struct e1000_tx_cbdata *cb;
+
+	ret = e1000_prep_frame(skb, netdev);
+	if (unlikely(ret != NETDEV_TX_OK))
+		return NETDEV_TX_OK;
+
+	if (test_bit(__E1000_DOWN, &adapter->state)) {
+		dev_kfree_skb_any(skb);
+		return NETDEV_TX_OK;
+	}
+
 	if (!spin_trylock_irqsave(&adapter->tx_queue_lock, irq_flags))
 		/* Collision - tell upper layer to requeue */
 		return NETDEV_TX_LOCKED;
 
+	cb = E1000_SKB_CB(skb);
 	/* need: count + 2 desc gap to keep tail from touching
 	 * head, otherwise try next time */
-	if (e1000_maybe_stop_tx(netdev, count + 2)) {
+	if (e1000_maybe_stop_tx(netdev, cb->count + 2)) {
 		spin_unlock_irqrestore(&adapter->tx_queue_lock, irq_flags);
 		return NETDEV_TX_BUSY;
 	}
 
-	if (adapter->vlgrp && vlan_tx_tag_present(skb)) {
-		tx_flags |= E1000_TX_FLAGS_VLAN;
-		tx_flags |= (vlan_tx_tag_get(skb) << E1000_TX_FLAGS_VLAN_SHIFT);
-	}
-
 	first = tx_ring->next_to_use;
 
 	tso = e1000_tso(adapter, skb);
@@ -3516,25 +3548,25 @@ static int e1000_xmit_frame(struct sk_buff *skb, struct net_device *netdev)
 	}
 
 	if (tso)
-		tx_flags |= E1000_TX_FLAGS_TSO;
+		cb->tx_flags |= E1000_TX_FLAGS_TSO;
 	else if (e1000_tx_csum(adapter, skb))
-		tx_flags |= E1000_TX_FLAGS_CSUM;
+		cb->tx_flags |= E1000_TX_FLAGS_CSUM;
 
 	/* Old method was to assume IPv4 packet by default if TSO was enabled.
 	 * 82571 hardware supports TSO capabilities for IPv6 as well...
 	 * no longer assume, we must. */
 	if (skb->protocol == htons(ETH_P_IP))
-		tx_flags |= E1000_TX_FLAGS_IPV4;
+		cb->tx_flags |= E1000_TX_FLAGS_IPV4;
 
-	count = e1000_tx_map(adapter, skb, first, max_per_txd, nr_frags, mss);
-	if (count < 0) {
+	cb->count = e1000_tx_map(adapter, skb, first, cb->max_per_txd, cb->nr_frags, cb->mss);
+	if (cb->count < 0) {
 		/* handle pci_map_single() error in e1000_tx_map */
 		dev_kfree_skb_any(skb);
 		spin_unlock_irqrestore(&adapter->tx_queue_lock, irq_flags);
-		return NETDEV_TX_OK;
+		return NETDEV_TX_BUSY;
 	}
 
-	e1000_tx_queue(adapter, tx_flags, count);
+	e1000_tx_queue(adapter, cb->tx_flags, cb->count);
 
 	netdev->trans_start = jiffies;
 

Re: [PATCH][E1000E] some cleanups

[Kok, Auke]

jamal wrote:
> Ok, here you go; the explanation is below. This is from net-2.6.24 of
> early this AM. I saw a patch you posted that is derived from Krishna;
> although it hasnt showed up in the tree - i have considered those
> changes and this patch adds a little more optimization in case of
> errors.
> 
> I will send you a patch to kill LLTX the sooner this shows up somewhere.


thanks.

My biggest problem with the patch as you sent it that it's a tonload of changes
and no implicit benefit immediately as I can see. I would really have to see the
LLTX change as well to make a wellfounded decision whether it is the right thing
to go and overhaul this part of the driver or not :)

I'm not particularly against the changes per se though.

so please, if needed offlist send me those LLTX changes as well.

Auke

> 
> On Wed, 2007-03-10 at 09:18 -0400, jamal wrote:
> 
>> The cleanup is to break up the code so it is functionally more readable
>> from a perspective of the 4 distinct parts in ->hard_start_xmit():
>>
>> a) packet formatting (example: vlan, mss, descriptor counting, etc.)
>> b) chip-specific formatting
>> c) enqueueing the packet on a DMA ring
>> d) IO operations to complete packet transmit, tell DMA engine to chew
>> on, tx completion interrupts, set last tx time, etc.
>>
>> Each of those steps sitting in different functions accumulates state
>> that is used in the next steps. cb stores this state because it a
>> scratchpad the driver owns. You could create some other structure and
>> pass it around the iteration, but why waste more bytes.
> 

Re: [PATCH][E1000E] some cleanups

[jamal]

On Mon, 2007-08-10 at 15:40 -0700, Kok, Auke wrote:

> 
> My biggest problem with the patch as you sent it that it's a tonload of changes
> and no implicit benefit immediately as I can see. 

The patch looks scary but is pretty tame when you apply it and stare at
it.

> I would really have to see the
> LLTX change as well to make a wellfounded decision whether it is the right thing
> to go and overhaul this part of the driver or not :)
> 
> I'm not particularly against the changes per se though.
> 
> so please, if needed offlist send me those LLTX changes as well.

Dont worry about it - i didnt get any love for a similar patch i did for
tg3 either ;-> I will hold onto it for now. I could do the un-LLTX
outside of this - would you like that for both e1000/e1000e ?

cheers,
jamal

Re: [PATCH][E1000E] some cleanups

[Kok, Auke]

jamal wrote:
> On Mon, 2007-08-10 at 15:40 -0700, Kok, Auke wrote:
> 
>> My biggest problem with the patch as you sent it that it's a tonload of changes
>> and no implicit benefit immediately as I can see. 
> 
> The patch looks scary but is pretty tame when you apply it and stare at
> it.
> 
>> I would really have to see the
>> LLTX change as well to make a wellfounded decision whether it is the right thing
>> to go and overhaul this part of the driver or not :)
>>
>> I'm not particularly against the changes per se though.
>>
>> so please, if needed offlist send me those LLTX changes as well.
> 
> Dont worry about it - i didnt get any love for a similar patch i did for
> tg3 either ;-> I will hold onto it for now. I could do the un-LLTX
> outside of this - would you like that for both e1000/e1000e ?

if we're going to remove LLTX from e1000 I prefer to do that at a much later time.
Let's focus on e1000e instead - while it is still moving ;)

Auke

Re: [PATCH][E1000E] some cleanups

[jamal]

On Tue, 2007-09-10 at 09:02 -0700, Kok, Auke wrote:
> 
> if we're going to remove LLTX from e1000 I prefer to do that at a much later time.
> Let's focus on e1000e instead - while it is still moving ;)

I think you may be in luck ;-> I just saw a patch posted by jgarzik
which touched both e1000/e LLTX. I am on travel mode starting tommorow,
I will try to extract it on my laptop while on the road but cant test
it.

cheers,
jamal