Date: Sat, 21 May 2005 07:34:27 +1000 From: Peter Jeremy <PeterJeremy@optushome.com.au> To: Brian Fundakowski Feldman <green@freebsd.org> Cc: Hans Petter Selasky <hselasky@c2i.net> Subject: Re: problems with new the "contigmalloc" routine Message-ID: <20050520213427.GH2129@cirb503493.alcatel.com.au> In-Reply-To: <20050520151620.GF1201@green.homeunix.org> References: <200505201340.36176.hselasky@c2i.net> <20050520151620.GF1201@green.homeunix.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Fri, 2005-May-20 11:16:20 -0400, Brian Fundakowski Feldman wrote: >On Fri, May 20, 2005 at 01:40:35PM +0200, Hans Petter Selasky wrote: >> I just hit some problems with the new "contigmalloc()" routine in >> FreeBSD-6-current, which is used by "bus_dmamem_alloc()". I found the problem in 4.x at the beginning of the year and worked out that it was still present in 6-current at that time: http://lists.freebsd.org/pipermail/freebsd-stable/2005-January/010979.html (which has a followup in -current which you noted later). >> 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. I agree that M_NOWAIT is not documented but which part of the following extract fron contigmalloc(9) allows contigmalloc() to sleep? The contigmalloc() function does not sleep waiting for memory resources to be freed up, but instead scans available physical memory a small num- ber of times for a suitably sized free address range before giving up. >> 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. We have lots of other checks (eg WITNESS) which are very space and time intensive. I think it would make a lot of sense to (optionally) more extensively validate function arguments and return values in assert()-style constructs. These assert()s should be written based on the documentation, not the code. (I can't remember what this programming style is called). Validating semantics (lock X should or should not be held and sleeping is/is not allowed) is more difficult but we have code that does some of this. >> Are these bugs in "contigmalloc()"? > >No. I would dispute this. contigmalloc() does not comply with its documentation and kernel developers should be able to use kernel internal APIs as they are documented. They should not need to audit the functions they are calling to verify that they behave as documented. In this particular case, it appears that contigmalloc() (and its predecessor, vm_page_alloc_contig()) were always able to sleep so this would appear to be a bug in the man page, rather than the code. >> 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. Agreed. > The contigmalloc(9) page is not entirely >truthful about the fact that it doesn't sleep at all -- it calls those >routines which can. If I call a function which states "this function never sleeps", I expect that my execution thread will return from the call without sleeping. I do not think it is reasonable for this statement to be interpreted as "this function doesn't sleep but can call other functions that do". I agree that you did not break contigmalloc() but since you are are working on contigmalloc() and are aware that the documentation is wrong, you might at least fix the documentation. -- Peter Jeremy
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20050520213427.GH2129>