From owner-freebsd-hackers@FreeBSD.ORG Fri May 20 19:50:53 2005 Return-Path: Delivered-To: freebsd-hackers@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 76DE716A4CE; Fri, 20 May 2005 19:50:53 +0000 (GMT) Received: from swip.net (mailfe07.swip.net [212.247.154.193]) by mx1.FreeBSD.org (Postfix) with ESMTP id 9677C43D39; Fri, 20 May 2005 19:50:50 +0000 (GMT) (envelope-from hselasky@c2i.net) X-T2-Posting-ID: Y1QAsIk9O44SO+J/q9KNyQ== Received: from mp-217-198-20.daxnet.no ([193.217.198.20] verified) by mailfe07.swip.net (CommuniGate Pro SMTP 4.3c5) with ESMTP id 174860987; Fri, 20 May 2005 21:50:48 +0200 From: Hans Petter Selasky To: Brian Fundakowski Feldman Date: Fri, 20 May 2005 21:51:34 +0200 User-Agent: KMail/1.7 References: <200505201340.36176.hselasky@c2i.net> <428E12B0.9020208@samsco.org> <20050520173236.GG1201@green.homeunix.org> In-Reply-To: <20050520173236.GG1201@green.homeunix.org> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Content-Disposition: inline Message-Id: <200505202151.35831.hselasky@c2i.net> cc: Scott Long cc: hackers@freebsd.org Subject: Re: problems with new the "contigmalloc" routine X-BeenThere: freebsd-hackers@freebsd.org X-Mailman-Version: 2.1.1 Precedence: list Reply-To: hselasky@c2i.net List-Id: Technical Discussions relating to FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 20 May 2005 19:50:53 -0000 On Friday 20 May 2005 19:32, Brian Fundakowski Feldman wrote: > On Fri, May 20, 2005 at 10:39:12AM -0600, Scott Long wrote: > > Brian Fundakowski Feldman wrote: > > >On Fri, May 20, 2005 at 01:40:35PM +0200, Hans Petter Selasky wrote: > > >>Hi, > > >> > > >>I just hit some problems with the new "contigmalloc()" routine in > > >>FreeBSD-6-current, which is used by "bus_dmamem_alloc()". > > >> > > >>Firstly it locks Giant, which cause locking order reversals when > > >>allocating memory from certain contexts. Secondly it sleeps even if > > >> flag M_NOWAIT is passed. Thirdly it does not support flag M_ZERO. > > > > > >Read the documentation. It supports M_ZERO just fine, and it does _not_ > > >support M_NOWAIT. OK, the M_ZERO support has been there for a while. Maybe it was before M_ZERO support was added, but I remember I used "contigmalloc()" to allocate memory and got dirty pages. Mostly the pages were zeroed, but suddently not, and I had to use bzero. How much has the zeroing mechanism been tested? > > > > > >>Can someone explain why these changes have been made? > > > > > >Changes? Maybe a WITNESS_WARN() was added that wasn't there before. At least it didn't complain earlier. > > > > > >>Why doesn't "contigmalloc()" give a warning when an invalid flag is > > >>passed? > > > > > >The kernel would be significantly larger and almost certainly slower > > >if it were to double-check that everywhere any bit fields are used > > >that flags that are not defined to have any behavior are unset. > > > It might be a pitfall when porting software. Suddenly a flag does not work like expected, undermining the logic of the software. Or what if someone passes the wrong enum like "BUS_DMA_ZERO" instead of "M_ZERO". Isn't there any KASSERT_WARN(!(mflags & (~(M_XXX)))) ? > > >>Are these bugs in "contigmalloc()"? > > > > > >No. > > > > > >>Or does the code using "contigmalloc()" have to be changed? > > > > > >Yes. The i386 bus_dmamem_alloc(), for example, calls it with M_NOWAIT > > >which is not a valid flag. The contigmalloc(9) page is not entirely > > >truthful about the fact that it doesn't sleep at all -- it calls those > > >routines which can. They can both be documented to require no locks > > >to be held when being called, except for M_NOWAIT specifically in the > > >one-page-or-less allocation case. > > > > The amount of colateral damage that has been inflicted on > > bus_dmamem_alloc by your changes to contigmalloc is truly impressive. > > Since I've spent the better part of 8 months cleaning it up, please > > let me know what else needs to be done so that there are no more > > surprises. And no, this is not my happy face. > > 1. > http://www.freebsd.org/cgi/cvsweb.cgi/src/sys/vm/vm_contig.c.diff?r1=1.34&r >2=1.41&f=h 2. Notice the lack of semantical changes. > 3. Notice that M_ZERO does the same thing as ever. > 4. Notice that the same goes for M_WAITOK/M_NOWAIT: ignored for anything > that needs more than one page. > 5. Negative points for Hans for posting corrupt backtraces and making > me go through a lot more work than should be necessary. > 6. Negative points to Hans for posting backtraces to an obviously- > molested tree (src/sys/dev/usb2/_*?!?). Sorry about that, but I've got my own USB driver located in /sys/dev/usb2, but the top of the backtrace should be right. > 7. Negative points to Scott for attempting to imply that for some > reason it's okay to call contigmalloc(9) in contexts that it has > never been okay to call it in. > 8. > http://unix.derkeiler.com/Mailing-Lists/FreeBSD/current/2003-11/1488.html > 9. Negative points to Scott for failure to spend a couple minutes of > research and find out these are known bugs. > 10. > http://lists.freebsd.org/pipermail/freebsd-current/2005-January/045105.html > 11. Negative points to Scott for failure to document system semantics when > it is obvious they should have been clarified according to Scott's own > notes on the matter. > 12. Mega positive points to Hans for actually asking the right questions. > 13. Go fix or document the brokenness in busdma, or implement the features > in contigmalloc(9) that were imagined to exist. > Well, the problem with blocking behaviour is that processes sleeping needs to be waited for: some_read(): ============ lock(); in_read = 1; unlock(); data = contigmalloc(...); lock(); if(detaching) { detaching = 0; wakeup(&detaching); goto done; } hw_queue(data); msleep(data); if(detaching) { detaching = 0; wakeup(&detaching); goto done; } unlock(); uiomove(data); lock(); if(detaching) { detaching = 0; wakeup(&detaching); goto done; } done: in_read = 0; unlock(); some_detach(): ============== lock(); if(in_read) { wakeup(data); detaching = 1; msleep(&detaching); } unlock(); It would be very nice if all memory allocation functions and "uiomove()" could support non-blocking mode, for example by passing a flag. If you look at device drivers like (/sys/dev/usb/ugen.c) you will see that the ones that wrote it didn't expect "uiomove" to sleep. And "uiomove" does not warn because it is called from a Giant locked context. I think you will find the same situation with "bus_dmamem_alloc()", where "BUS_DMA_NOWAIT" is passed as an argument because the code does not handle it when the call sleeps. Non-blocking mode has got to be supported. Else you get a nightmare rewriting the code to support blocking mode. Can anyone explain why "uiomove()" has to sleep, and why there is no non-blocking "uiomove()"? Will M_NOWAIT support will be added to "contigmalloc()"? --HPS