From owner-svn-src-head@FreeBSD.ORG Fri May 30 16:45:17 2014 Return-Path: Delivered-To: svn-src-head@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 154E7BF3; Fri, 30 May 2014 16:45:17 +0000 (UTC) Received: from bigwig.baldwin.cx (bigwig.baldwin.cx [IPv6:2001:470:1f11:75::1]) (using TLSv1 with cipher DHE-RSA-CAMELLIA256-SHA (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id DD58C2920; Fri, 30 May 2014 16:45:16 +0000 (UTC) Received: from jhbbsd.localnet (unknown [209.249.190.124]) by bigwig.baldwin.cx (Postfix) with ESMTPSA id 8AACAB924; Fri, 30 May 2014 12:45:14 -0400 (EDT) From: John Baldwin To: attilio@freebsd.org Subject: Re: svn commit: r266775 - head/sys/x86/x86 Date: Fri, 30 May 2014 12:44:07 -0400 User-Agent: KMail/1.13.5 (FreeBSD/8.4-CBSD-20140415; KDE/4.5.5; amd64; ; ) References: <201405272131.s4RLVBEU035321@svn.freebsd.org> <201405301147.46872.jhb@freebsd.org> In-Reply-To: MIME-Version: 1.0 Content-Type: Text/Plain; charset="utf-8" Content-Transfer-Encoding: 7bit Message-Id: <201405301244.07316.jhb@freebsd.org> X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.2.7 (bigwig.baldwin.cx); Fri, 30 May 2014 12:45:14 -0400 (EDT) Cc: "src-committers@freebsd.org" , Benno Rice , "svn-src-all@freebsd.org" , Scott Long , "svn-src-head@freebsd.org" , "Peel, Casey" X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.18 Precedence: list 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: Fri, 30 May 2014 16:45:17 -0000 On Friday, May 30, 2014 11:51:38 am Attilio Rao wrote: > On Fri, May 30, 2014 at 5:47 PM, John Baldwin wrote: > > On Friday, May 30, 2014 11:39:24 am Attilio Rao wrote: > >> On Fri, May 30, 2014 at 5:03 PM, John Baldwin wrote: > >> > On Friday, May 30, 2014 10:54:06 am Attilio Rao wrote: > >> >> On Tue, May 27, 2014 at 11:31 PM, Scott Long wrote: > >> >> > Author: scottl > >> >> > Date: Tue May 27 21:31:11 2014 > >> >> > New Revision: 266775 > >> >> > URL: http://svnweb.freebsd.org/changeset/base/266775 > >> >> > > >> >> > Log: > >> >> > Eliminate the fake contig_dmamap and replace it with a new flag, > >> >> > BUS_DMA_KMEM_ALLOC. They serve the same purpose, but using the flag > >> >> > means that the map can be NULL again, which in turn enables significant > >> >> > optimizations for the common case of no bouncing. > >> >> > >> >> While I think this is in general a good idea, unfortunately our > >> >> drivers do so many dumb things when freeing DMA allocated buffers that > >> >> having a NULL map is going to cause some "turbolence" and make such > >> >> bugs more visible. > >> >> An example is with ATA, where I think this fix is needed: > >> >> http://www.freebsd.org/~attilio/dmamem_free-ata.patch > >> >> > >> >> Otherwise, what can happen with bounce buffers, is that the allocated > >> >> memory via contig malloc was not going to be freed anytime. > >> >> > >> >> I tried to look around and I found questionable (read broken) code in > >> >> basically every driver which allocates DMA buffers, so I really don't > >> >> feel I want to fix the majority of our drivers. I just think such > >> >> paths are not excercised enough to be seen in practice often or the > >> >> bugs just get unnoticed. > >> > > >> > Eh, many maps for static allocations were already NULL and have been for a > >> > long time. This is nothign new. Plus, the diff you posted has a bug > >> > regardless of explicitly destroying a map created by bus_dmamem_alloc(). > >> > >> Did you notice that I *removed* the destroy not *added*? > > > > Yes, my point was that that bug in the original code you are fixing was there > > regardless of Scott's change. > > And when I did say something different? > I don't understand what's the point of your messages, besides showing > that you didn't read correctly my patch. I read yours correctly but worded mine poorly. My point is that Scott's change does not introduce anything new. We've had NULL maps for static allocations for many, many years. It's only been recently that we've had more maps not be NULL for this. However, even if you discounted the whole NULL vs non-NULL maps thing, the driver in question that you are fixing was broken regardless. That is, due to the extra bus_dmamap_destroy() the driver was broken regardless of whether the map was NULL or non-NULL. TL;DR: - Scott's change did not introduce any new behavior so we don't need to worry about a spate of new bugs uncovered in drivers because of it. - Your fix is correct, and it was broken with or without Scott's change. -- John Baldwin