Date: Fri, 30 May 2014 12:44:07 -0400 From: John Baldwin <jhb@freebsd.org> To: attilio@freebsd.org Cc: "src-committers@freebsd.org" <src-committers@freebsd.org>, Benno Rice <benno@freebsd.org>, "svn-src-all@freebsd.org" <svn-src-all@freebsd.org>, Scott Long <scottl@freebsd.org>, "svn-src-head@freebsd.org" <svn-src-head@freebsd.org>, "Peel, Casey" <casey.peel@isilon.com> Subject: Re: svn commit: r266775 - head/sys/x86/x86 Message-ID: <201405301244.07316.jhb@freebsd.org> In-Reply-To: <CAJ-FndA_Z_52t2xD2=LftCcwuipsVUFNfGNM-1RZ4zu2NyGsJA@mail.gmail.com> References: <201405272131.s4RLVBEU035321@svn.freebsd.org> <201405301147.46872.jhb@freebsd.org> <CAJ-FndA_Z_52t2xD2=LftCcwuipsVUFNfGNM-1RZ4zu2NyGsJA@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Friday, May 30, 2014 11:51:38 am Attilio Rao wrote: > On Fri, May 30, 2014 at 5:47 PM, John Baldwin <jhb@freebsd.org> wrote: > > On Friday, May 30, 2014 11:39:24 am Attilio Rao wrote: > >> On Fri, May 30, 2014 at 5:03 PM, John Baldwin <jhb@freebsd.org> wrote: > >> > On Friday, May 30, 2014 10:54:06 am Attilio Rao wrote: > >> >> On Tue, May 27, 2014 at 11:31 PM, Scott Long <scottl@freebsd.org> 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201405301244.07316.jhb>