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: > 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 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: > 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
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
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
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
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 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
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 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 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: > 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
