On 10/18/2012 07:31 AM, Stefan Hajnoczi wrote: > On Thu, Oct 18, 2012 at 10:34 AM, Dmitry Fleytman <dmi...@daynix.com> wrote: >> The real purpose of check_rxov it a bit confusing indeed, mainly >> because of unclear name (rename?), >> however it works as following: >> >> There are 2 possible when RDT == RDH for RX ring: >> 1. Device used all the buffers from ring, no empty buffers available >> 2. Driver fully refilled the ring and all buffers are empty and ready to >> use
The 2nd case is not true. We should only have RDT == RDH when the ring is empty. If RDT == RDH and the ring is full then we have a bug in the driver. The driver should only ever allow RDT to be one less than head, or ring size - 1 if head is 0. >> check_rxov is used to distinguish these 2 cases: >> 1. It must be 1 initially (init, reset, etc.) >> 2. It must be set to one when device uses buffer >> 3. It must be set to 0 when driver adds buffer to the ring >> check_rxov == 1 - ring is empty >> check_rxov == 0 - ring is full >> >> Indeed, RX init sequence doesn't look logical, however this is the way >> all Intel driver behave from e1000 and up to ixgbe. >> Also see some explanation here: >> http://permalink.gmane.org/gmane.linux.kernel/1375917 >> >> If we drop check_rxov and always treat RDH == RDT as empty ring we'll >> probably get correct behavior for current Linux driver's code (needs >> testing of course), >> however we have no idea how Windows drivers work. The windows driver should work the same way. If RDH == RDT the hardware will treat that as a empty ring and will hang. If there is a driver that is setting RDH == RDT to indicate the ring is full please let us know as that is likely a buggy driver. > Thanks, for the great explanation, Dmitry. > > Alexander: I CCed you because I hope you might be able to explain what > the 82540EM card does when a driver sets RDT to the value of RDH. The > QEMU NIC emulation code treats this as a full ring (i.e. the > descriptors are valid and will be filled in by the hardware). Does > the real hardware act like this or will it treat this condition as > ring empty (i.e. if the driver sets RDT to the value of RDH then the > hardware stops receive because there are no descriptors available)? > > I can't find a statement in the Intel datasheet about what happens > when the driver sets RDT = RDH. The QEMU check_rxov variable is > trying to distinguish between ring empty (RDH has moved to RDT) and > ring full (driver has set RDH = RDT because the full descriptor ring > is available). If RDT == RDH then we should stop receiving traffic. As far as I know all of our e1000 hardware treat RDT == RDH as an empty ring state. All of the drivers should have code in place to stop it. For example the E1000_DESC_UNUSED macro should be returning ring size - 1 in the case of RDT == RDH which will result in the head being 0 and the tail being ring size - 2. > Dmitry: At this point we'd need to test what happens on real hardware > when RDH = RDT in order to be able to remove check_rxov. As you > mentioned, with the Linux e1000 driver we don't see ring full RDH = > RDT: > > /* call E1000_DESC_UNUSED which always leaves > * at least 1 descriptor unused to make sure > * next_to_use != next_to_clean */ > for (i = 0; i < adapter->num_rx_queues; i++) { > struct e1000_rx_ring *ring = &adapter->rx_ring[i]; > adapter->alloc_rx_buf(adapter, ring, > E1000_DESC_UNUSED(ring)); > } > > Here some sample output from a QEMU printf, notice how RDH is never > the same as RDT once rx begins: > > set_rdt rdh=0 rdt_old=0 rdt_new=0 > set_rdt rdh=0 rdt_old=0 rdt_new=254 > set_rdt rdh=1 rdt_old=254 rdt_new=255 > set_rdt rdh=2 rdt_old=255 rdt_new=0 > set_rdt rdh=3 rdt_old=0 rdt_new=1 > set_rdt rdh=4 rdt_old=1 rdt_new=2 > set_rdt rdh=5 rdt_old=2 rdt_new=3 > set_rdt rdh=6 rdt_old=3 rdt_new=4 > set_rdt rdh=7 rdt_old=4 rdt_new=5 > set_rdt rdh=9 rdt_old=5 rdt_new=7 > set_rdt rdh=10 rdt_old=7 rdt_new=8 > set_rdt rdh=11 rdt_old=8 rdt_new=9 > set_rdt rdh=12 rdt_old=9 rdt_new=10 > set_rdt rdh=13 rdt_old=10 rdt_new=11 > set_rdt rdh=14 rdt_old=11 rdt_new=12 > > The iPXE 'intel' driver (supports e1000 cards) also does not set RDH = > RDT for full rx ring, instead it only uses 4 out of 8 descriptors at a > time. > > The reason I'm digging into the need for check_rxov is because it's a > dangerous piece of code to have. If check_rxov logic is ever out of > sync we risk memory corruption. I'd really like to drop it > completely. > > Stefan There should be no need for check_rxov. As far as I know none of our drivers will ever set RDT == RDH if there are descriptors available on the ring. Thanks, Alex
