Re: CVS commit: src/sys/net

[Kengo NAKAHARA <k-nakahara%iij.ad.jp@localhost>]

Hi,

On 2015/12/11 13:00, Christos Zoulas wrote:
> 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.

I see. Thank you for your explanation.

> Note that bt_refill return values are not even always checked, so
> this can fail in other mysterious ways.

Oh...

> | > 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.

I revert KASSERT in if_gif.c, and I use 'if' instead of KASSERT from now.

> Log Message:
> Spell out that KM_SLEEP allocations can fail.
> 
> 
> To generate a diff of this commit:
> cvs rdiff -u -r1.17 -r1.18 src/share/man/man9/kmem.9

Thank you for your quickly update!


Thanks,

-- 
//////////////////////////////////////////////////////////////////////
Internet Initiative Japan Inc.

Device Engineering Section,
Core Product Development Department,
Product Division,
Technology Unit

Kengo NAKAHARA <k-nakahara%iij.ad.jp@localhost>