Re: [PATCH][E1000E] some cleanups
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 - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH][E1000E] some cleanups
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 - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH][E1000E] some cleanups
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 - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH][E1000E] some cleanups
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. - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH][E1000E] some cleanups
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.
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)
Re: [PATCH][E1000E] some cleanups
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 - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH][E1000E] some cleanups
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 - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH][E1000E] some cleanups
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 - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH][E1000E] some cleanups
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 - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH][E1000E] some cleanups
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
[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 [EMAIL PROTECTED]
---
commit ad63c288ce980907f68d94d5faac08625c0b1782
tree 296a6da371b98c6488c544a8910941ea6d8c18a8
parent 7f5d0afdff875b2c4957031f8934741aefe257cc
author Jamal Hadi Salim [EMAIL PROTECTED] Sun, 30 Sep 2007 13:26:00 -0400
committer Jamal Hadi Salim [EMAIL PROTECTED] 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 0x
#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)
Re: [PATCH][E1000E] some cleanups
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 - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH][E1000E] some cleanups
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 - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH][E1000E] some cleanups
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 - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH][E1000E] some cleanups
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 - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH][E1000E] some cleanups
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 - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
