On Thu, Dec 10, 2015 at 11:00:11PM -0500, Christos Zoulas wrote: > On Dec 11, 11:37am, k-nakah...@iij.ad.jp (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.
(moving this discussion to tech-kern) how about instead we fix the kmem_alloc() implementation to match the man page? that seems much more practical to me. adding failure checks and recovery code to the thousands of *alloc() calls in the kernel would be a vast amount of work for very little benefit. an attempt to allocate an amount of memory large enough that it can never succeed sounds like a bug to me, and it seems better to fix any such bugs rather than add a vast amount of mostly useless error handling code in hopes of papering over them. -Chuck