problems with new the "contigmalloc" routine

[Peter Jeremy]

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