lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Tue, 22 May 2007 11:51:23 -0500 From: Milton Miller <miltonm@....com> To: "Kok, Auke" <auke-jan.h.kok@...el.com> Cc: Jeff Garzik <jgarzik@...ox.com>, Scott Feldman <sfeldma@...ox.com>, e1000-devel@...ts.sourceforge.net, Jeff Kirsher <jeffrey.t.kirsher@...el.com>, John Ronciak <john.ronciak@...el.com>, Jesse Brandeburg <jesse.brandeburg@...el.com>, netdev@...r.kernel.org, David Acker <dacker@...net.com> Subject: Re: [PATCH] fix e100 rx path on ARM (was [PATCH] e100 rx: or s and el bits) On May 21, 2007, at 12:45 PM, Kok, Auke wrote: > Milton Miller wrote: >> On May 18, 2007, at 12:11 PM, David Acker wrote: >>> Kok, Auke wrote: >>>> First impression just came in: It seems RX performance is dropped >>>> to 10mbit. TX is unaffected and runs at 94mbit/tcp, but RX the new >>>> code seems to misbehave and fluctuate, dropping below 10mbit after >>>> a few netperf runs and staying there... >>>> ideas? >>> I found the problem. Another casualty of working with two different >>> kernels at once...arg. >>> The blank rfd needs to have its el-bit clear now. Here is the new >>> and improved patch. ... >> Proceeding with the review: >> Coding style: >> (1) if body on seperate line. >> (2) space after if before ( >> (3) The other enums in this driver are not ALL_CAPS >> (4) This driver doesn't do CONSTANT != value but value != enum >> (see nic->mac for examples) > > I sent Milton my copy of this patch which has these style issues > corrected and > applies cleanly to a recent git tree. If anyone else specifically > wants a copy > let me know. > > Auke It addressed 1 and 2, and applies, but did not address 3 and 4. But the bigger point is it didn't address the holes I identified. I think we need to change the logic to reclaim the size from 0 only if we are restarting, and make rx_indicate look ahead to rx->next if it encounters a !EL size 0 buffer. Without this we are doing a "prohibited" rx_start to a running machine. The device can still see this size 0 !EL state. Also we will get stuck when the device finds the window between the two writes. We can remove some register pressure by finding old_before_last_rfd when we are ready to use it, just comparing old_before_last_rx to new. Also, as I pointed out, the rx_to_start change to start_reciever is compicated and unnecessary, as rx_to_clean can always be used and it was the starting point before the changes. As far as the RU_SUSPENDED, I don't think we need it, instead we should poll the device. Here is my proposal: rx_indicate can stop when it hits the packet with EL. If it hits a packet with size 0, look ahead to rx->next to see if it is complete, if so complete this one otherwise leave it as next to clean. After the rx_indicate loop, try to allocate more skbs. If we are successful, then fixup the before-next as we do now. Then check the device status for RNR, and if its stopped then set rx_to_clean rfd size back to ETH_LEN and restart the reciever. This does have a small hole: if we only add one packet at a time we will end up with all size 0 descriptors in the lopp. We can detect that and not remove EL from the old before-next unless we are restarting. That would imply moving the status poll before we allocate the list. milton - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists