problems with new the "contigmalloc" routine

[Brian Fundakowski Feldman]

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.
> >
> >
> >>Can someone explain why these changes have been made?
> >
> >
> >Changes?
> >
> >
> >>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.
> >
> >
> >>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&r2=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/_*?!?).
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.

Out.

-- 
Brian Fundakowski Feldman                           \'[ FreeBSD ]''''''''''\
  <> green at FreeBSD.org                               \  The Power to Serve! \
 Opinions expressed are my own.                       \,,,,,,,,,,,,,,,,,,,,,,\