Re: CVS commit: src/sys/net

[christos%zoulas.com@localhost (Christos Zoulas)]

On Dec 11, 11:37am, k-nakahara%iij.ad.jp@localhost (Kengo NAKAHARA) wrote:
-- Subject: Re: CVS commit: src/sys/net

| Hi,
| 
| > In article <20151210081103.E0FBBFB83%cvs.NetBSD.org@localhost>,
| > Kengo NAKAHARA <source-changes-d%NetBSD.org@localhost> wrote:
| >>-=-=-=-=-=-
| >>
| >>Module Name:	src
| >>Committed By:	knakahara
| >>Date:		Thu Dec 10 08:11:03 UTC 2015
| >>
| >>Modified Files:
| >>	src/sys/net: if_gif.c
| >>
| >>Log Message:
| >>kmem_zalloc(, KM_SLEEP) must not return NULL.
| > 
| > I would like to solicit opinions about this change and form a general
| > policy.
| > 
| > 1. I would like to reduce the use of KASSERT in the kernel, specially
| > in situations like thee above where the test can be centralized (inside
| > kmem_alloc) and avoided without being fatal.
| 
| OK, this kmem_zalloc() is not fatal. I should avoid KASSERT here.
| 
| 
| > 2. Static analyzer models understand allocators, but they are not
| > smart enough to determine under which situations they can fail. I
| > believe even kmem_alloc with KM_SLEEP can fail when the size is
| > large enough.
| 
| I have a question. The man of kmem(9) says:
| ====================
|      kmflags  Either of the following:
| 
|               KM_SLEEP    If the allocation cannot be satisfied immediately,
|                           sleep until enough memory is available.

		....
                int ret = uvm_km_kmem_alloc(kmem_va_arena,
                    (vsize_t)round_page(size),
                    ((kmflags & KM_SLEEP) ? VM_SLEEP : VM_NOSLEEP) 
                     | VM_INSTANTFIT, (vmem_addr_t *)&p);
                if (ret) {
                        return NULL; 
                } 
		....

| ====================
| Is this manual incorrect?
| I'm confused... Could you tell me easily comprehensible manual?

It is documented that it does not fail if you sleep because this
is the usual scenario. Follow the path to the rebbit hole and you
go the uvm_km_kmem_alloc -> vmem_alloc -> bt_refill etc. and you
see that under certain conditions it will fail even when you sleep.
Note that bt_refill return values are not even always checked, so
this can fail in other mysterious ways.

| > So I propose to always check the return value of allocators with
| > an 'if' and not a KASSERT.
| 
| There are some codes like "foo = kmem_alloc(size, KM_SLEEP);
| KASSERT(foo != NULL)".
| Should the codes be unified to use not KASSERT' but if'?

Yes (when it is possible), and the man page for kmem_alloc should be
changed to reflect that.

christos