Date: Sat, 13 Mar 2010 23:05:11 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Pyun YongHyeon <yongari@freebsd.org> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r205090 - head/sys/dev/bge Message-ID: <20100313222131.K22847@delplex.bde.org> In-Reply-To: <201003121818.o2CII4ri076014@svn.freebsd.org> References: <201003121818.o2CII4ri076014@svn.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Fri, 12 Mar 2010, Pyun YongHyeon wrote: > Log: > Reorder interrupt handler a bit such that producer/consumer > index of status block is read first before acknowledging the > interrupts. Otherwise bge(4) may get stale status block as > acknowledging an interrupt may yield another status block update. > > Reviewed by: marius Er, doesn't this give a race instead? It undoes a critical part of rev.1.169 but not the comment part which still says that the ack is done first, and why (to ensure getting another interrupt if the status block changes after we have looked at it). % /* % * Do the mandatory PCI flush as well as get the link status. % */ % statusword = CSR_READ_4(sc, BGE_MAC_STS) & BGE_MACSTAT_LINK_CHANGED; % % /* Make sure the descriptor ring indexes are coherent. */ % bus_dmamap_sync(sc->bge_cdata.bge_status_tag, % sc->bge_cdata.bge_status_map, % BUS_DMASYNC_POSTREAD | BUS_DMASYNC_POSTWRITE); % rx_prod = sc->bge_ldata.bge_status_block->bge_idx[0].bge_rx_prod_idx; % tx_cons = sc->bge_ldata.bge_status_block->bge_idx[0].bge_tx_cons_idx; % sc->bge_ldata.bge_status_block->bge_status = 0; % bus_dmamap_sync(sc->bge_cdata.bge_status_tag, % sc->bge_cdata.bge_status_map, % BUS_DMASYNC_PREREAD | BUS_DMASYNC_PREWRITE); The above presumably gives sufficiently coherent accesses to the status block, but what happens if a status update occurs now (before the ack). Doesn't the ack prevent an interrupt for this status update? I think tx_prod and tx cons (read above) don't become stale since they are only advanced by software, and we may processes tx and rx descriptors beyond the ones reported by status updates before or after the ack, but statusword (read above) does become stale. % % /* % * Ack the interrupt by writing something to BGE_MBX_IRQ0_LO. Don't % * disable interrupts by writing nonzero like we used to, since with % * our current organization this just gives complications and % * pessimizations for re-enabling interrupts. We used to have races % * instead of the necessary complications. Disabling interrupts I don't remember seeing races with the current order, but I seem to remember seeing them when the ack was the last hardware thing in the function. As described in detail below, the latter gives quite a large race window so it is easy to miss an interrupt. % * would just reduce the chance of a status update while we are % * running (by switching to the interrupt-mode coalescence % * parameters), but this chance is already very low so it is more % * efficient to get another interrupt than prevent it. This describes why it doesn't matter if we get an extra interrupt due to the status block being updated after the ack, even in rev.1.168 when the race window was much larger (it was the entire runtime of bge_intr(), which can be several hundred uS; now it is several hundred nS). % * % * We do the ack first to ensure another interrupt if there is a % * status update after the ack. We don't check for the status But we don't do the ack first any more. % * changing later because it is more efficient to get another % * interrupt than prevent it, not quite as above (not checking is % * a smaller optimization than not toggling the interrupt enable, % * since checking doesn't involve PCI accesses and toggling require % * the status check). So toggling would probably be a pessimization % * even with MSI. It would only be needed for using a task queue. % */ % bge_writembx(sc, BGE_MBX_IRQ0_LO, 0); Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20100313222131.K22847>