From owner-svn-src-head@FreeBSD.ORG Mon Mar 15 17:39:16 2010 Return-Path: Delivered-To: svn-src-head@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 842E2106566C; Mon, 15 Mar 2010 17:39:16 +0000 (UTC) (envelope-from pyunyh@gmail.com) Received: from mail-fx0-f215.google.com (mail-fx0-f215.google.com [209.85.220.215]) by mx1.freebsd.org (Postfix) with ESMTP id 855D28FC17; Mon, 15 Mar 2010 17:39:15 +0000 (UTC) Received: by fxm7 with SMTP id 7so457614fxm.3 for ; Mon, 15 Mar 2010 10:39:14 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:received:received:received:from:date:to:cc :subject:message-id:reply-to:references:mime-version:content-type :content-disposition:in-reply-to:user-agent; bh=beeC7vyKsyqAtDMoB83RXZTtLEOsgZ0Egv3E3PznHeQ=; b=YYpREpK5taQ092fOOuvJzQvXmcTOZDvePlzMhADg2s3xqbqc7kqf2gr85nVozL34EF nB+gN9JZVqB+/0lTZVddH9W6i+p2JqjsDHqU5/Mn4BPw7jMYddClxbv0p7yeBG0EawcK 39Ue+xq6+VVWZmYM4VAh7ShxDeDhqeEIpuLqU= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=from:date:to:cc:subject:message-id:reply-to:references:mime-version :content-type:content-disposition:in-reply-to:user-agent; b=HjS5X7b54fiDh4slFPxlMcXCNWafIoUnEIB/uNKyCNtuxh1CH4rxda/bZIHJeAO46h 1CoLCDofVQ33VUXbgP1wtUkdT0PcBOj4Kz7vY60cQeZYBFfTBUdeym++szvfH/8zLkS1 EetYY7HqQ5rBuOjkEZiCNC38Or0uDmkDv/FCc= Received: by 10.87.76.30 with SMTP id d30mr8196999fgl.52.1268674754359; Mon, 15 Mar 2010 10:39:14 -0700 (PDT) Received: from pyunyh@gmail.com ([174.35.1.224]) by mx.google.com with ESMTPS id e20sm4733749fga.15.2010.03.15.10.39.10 (version=TLSv1/SSLv3 cipher=RC4-MD5); Mon, 15 Mar 2010 10:39:13 -0700 (PDT) Received: by pyunyh@gmail.com (sSMTP sendmail emulation); Mon, 15 Mar 2010 10:38:40 -0700 From: Pyun YongHyeon Date: Mon, 15 Mar 2010 10:38:40 -0700 To: Bruce Evans Message-ID: <20100315173840.GC1178@michelle.cdnetworks.com> References: <201003121818.o2CII4ri076014@svn.freebsd.org> <20100313222131.K22847@delplex.bde.org> <20100314225409.GA1245@michelle.cdnetworks.com> <20100316031232.H1539@besplex.bde.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20100316031232.H1539@besplex.bde.org> User-Agent: Mutt/1.4.2.3i Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org, Pyun YongHyeon Subject: Re: svn commit: r205090 - head/sys/dev/bge X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list Reply-To: pyunyh@gmail.com List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 15 Mar 2010 17:39:16 -0000 On Tue, Mar 16, 2010 at 03:47:29AM +1100, Bruce Evans wrote: > On Sun, 14 Mar 2010, Pyun YongHyeon wrote: > > >On Sat, Mar 13, 2010 at 11:05:11PM +1100, Bruce Evans wrote: > >>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 > > > >You're probably right it may increase race window for interrupts. > > Rev.1.169 was supposed to fix all races involving the interrupt ack. > An increase from 0 to epsilon is large :-). > It may fix the race but ignored status block coherency. > >But it ensures coherent accesses to the status block which is more > >important than losing interrupts. > > Interrupts weren't lost after 1.169 AFAIK. Rather the reverse. We > take a few extra interrupts in order not to miss any. I don't see how > your change improves coherency but don't really understand the coherency. > But^2 I now remember discussing with scottl that there is no locking > for the status block and it is not clear whether there should be or > how things mostly work when there isn't (mutex-type locking seems to > be useless). > Old code (before r196370) had packet drop issues due to the status block coherency problems. > >I think old code fought not to > >lose interrupts by acknowledging interrupts first and tried to get > >next interrupts if status block was updated in interrupt handler. > >Old code also had similar race window because it blindly > >acknowledged the interrupt, status block could be updated after the > >acknowledgment of the interrupt. > > Why is this fighting? Acking the interrupt should have no affect on > the contents of the status block, and any status block update after > acking should cause a new interrupt. Of course the hardware might If you're talking about interrupt only, yes you're right. Acking the interrupt tell the controller can update the status block at any time which in turn can cause packet drops. Tagged status mode will tell acked sequence number to controller so controller will interrupt again if there is sequence number difference between driver and status block. I think this is more reliable way to catch interrupts as well as fixing status block coherency. > not be as nice as that, but something like this seems to be needed > for a status block to work efficiently. > I still think the bge(4) hardwares except BCM5700 were correctly designed. bge(4) just didn't take advantage of it. > Anyway, the old race window (if it is a race that matters) isn't similar, > but is the opposite race. > > > > >>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). > > > >Theoretically this may happen if interrupt is shared with other > >devices. Since bge(4) does not check whether the interrupt is ours > >it may blindly process the interrupt. > > Well, my version checks, but you said before that some hardware cannot > be trusted to get this right (it works with my 5701 UP no-MSI). > Yes, for non-MSI case, interrupt could be delivered first before updating a status block. This is the reason why bge(4) still has CSR_READ_4(sc, BGE_MAC_STS) to ensure status block update. This is different issue and I think it can't be easily avoided. > I see that you are saying that your change doesn't help if the interrupt > isn't ours. Then the ack is still done first (long ago, on the last > bge_intr()) so the above may run partly before the interrupt is ours > and partly after, giving the same problem (the one that I don't As long as status block coherency is guaranteed there is no negative effect except doing some unnecessary work. This is the same behavior as before. > understand :-) caused by running the above after an ack. Is the status > block supposed to be frozen once an interrupt really for us occurs > until we ack the interrupt? > I think so. This means acking interrupt can yield status block update in the middle of interrupt handler.