From owner-svn-src-all@FreeBSD.ORG Wed Mar 12 22:39:00 2014 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 4823CF2C; Wed, 12 Mar 2014 22:39:00 +0000 (UTC) Received: from mail105.syd.optusnet.com.au (mail105.syd.optusnet.com.au [211.29.132.249]) by mx1.freebsd.org (Postfix) with ESMTP id E6B74EC0; Wed, 12 Mar 2014 22:38:59 +0000 (UTC) Received: from c122-106-147-133.carlnfd1.nsw.optusnet.com.au (c122-106-147-133.carlnfd1.nsw.optusnet.com.au [122.106.147.133]) by mail105.syd.optusnet.com.au (Postfix) with ESMTPS id 0FA8710436DF; Thu, 13 Mar 2014 09:38:52 +1100 (EST) Date: Thu, 13 Mar 2014 09:38:51 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Konstantin Belousov Subject: Re: svn commit: r263080 - in head/sys: dev/cpuctl dev/hwpmc kern In-Reply-To: <201403121025.s2CAPQRl017239@svn.freebsd.org> Message-ID: <20140313081336.C5739@besplex.bde.org> References: <201403121025.s2CAPQRl017239@svn.freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.1 cv=fbeUlSgF c=1 sm=1 tr=0 a=7NqvjVvQucbO2RlWB8PEog==:117 a=PO7r1zJSAAAA:8 a=nTxyh5FW7Y8A:10 a=kj9zAlcOel0A:10 a=JzwRw_2MAAAA:8 a=D_k5nLaqhlZ3kOncPtIA:9 a=CjuIK1q_8ugA:10 Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.17 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 12 Mar 2014 22:39:00 -0000 On Wed, 12 Mar 2014, Konstantin Belousov wrote: > Log: > Use correct types for sizeof() in the calculations for the malloc(9) sizes [1]. > While there, remove unneeded checks for failed allocations with M_WAITOK flag. > > Submitted by: Conrad Meyer [1] > MFC after: 1 week Nice cleanups. > Modified: head/sys/kern/kern_linker.c > ============================================================================== > --- head/sys/kern/kern_linker.c Wed Mar 12 10:23:51 2014 (r263079) > +++ head/sys/kern/kern_linker.c Wed Mar 12 10:25:26 2014 (r263080) > @@ -725,14 +725,11 @@ linker_file_add_dependency(linker_file_t > linker_file_t *newdeps; > > sx_assert(&kld_sx, SA_XLOCKED); > - newdeps = malloc((file->ndeps + 1) * sizeof(linker_file_t *), > - M_LINKER, M_WAITOK | M_ZERO); > - if (newdeps == NULL) > - return (ENOMEM); > + newdeps = malloc((file->ndeps + 1) * sizeof(*newdeps), M_LINKER, > + M_WAITOK | M_ZERO); > Still has a style bug: extra blank line. KNF disallows all optional blank lines (indent -sob), even ones to separate unrelated statements, and this one separates related statements. > if (file->deps) { > - bcopy(file->deps, newdeps, > - file->ndeps * sizeof(linker_file_t *)); > + bcopy(file->deps, newdeps, file->ndeps * sizeof(*newdeps)); > free(file->deps, M_LINKER); > } > file->deps = newdeps; I don't agree with having realloc() or reallocf() in the kernel, but since realloc() is there it is a style bug to not use it in the above. The code using realloc() seems to be: file->deps = realloc(file->deps, (file->ndeps + 1) * sizeof(*newdeps), M_LINKER, M_WAITOK | M_ZERO); This is 5 more lines shorter. Perhaps 8 lines shorter after removing the temporary variable. reallocf() is especially not needed in the kernel, since most allocations are M_WAITOK so they can't fail. It is used a whole 5 files in the whole kernel, and most of these uses are wrong. - in drm_realloc() in dev/drm/drmP.h. This function is badly named. It does reallocf(), not realloc(). This function is so bogus that it is never used, at least in dev/drm. The nearby drm_alloc(), which does malloc(), is used a lot. Instead, 4 places use plain realloc(). Plain realloc() suffices for these cases, since all uses are with M_WAITOK. But all these uses are followed by garbage error handling that is even more laborious than in the garbage removed in this commit, despite having the usual bug from not using reallocf() (leak the old allocation on failure). - 3 times in dev/pci/pci.c. All these uses are bogus, since the allocations are M_WAITOK so they can't fail, and if they could fail the error handling given by reallocf() would be wrong. - 2 times in in uhso_alloc_tty(). All these uses are bogus, as in pci.c, except they are followed by bogus checks that the M_WAITOK reallocf() doesn't fail, so perhaps the error handling for the impossible case where it fails is correct. - in drm_realloc() in dev/drm2/drmP.h. drm is cloned for some reason, so its bugs are cloned too. All the other bugs described above are cloned too, starting with not actually using this function. - twice in dev/drm2/drm_edid2.c. All these uses are bogus, as in pci.c, plus they have the internal layering violation of not using drm2's own realloc interface. These bugs are not due to the cloning -- the original never uses reallocf(), and the dead code for the error handling is not present in the new code. - in atm_getvccs(). This use is actually correct. Summary: after removing the garbage, reallocf() is used a whole once in the kernel. Not counting the above, or boot or utilities, realloc() is used in a whole 22 files in the kernel, with an average of not much more than 1 use per file. Many of the uses are in low quality code that does things like casting the result of realloc() or trying to implement the usual bug from not using reallocf() but not managing to do so since by using M_WAITOK (these tend to be followed by verbose dead code for handling allocation failure but not the leak; hopefully callers of malloc() are not so stupid about M_WAITOK). This leaves about 10 correct useful uses of realloc(). It saves a whole statement to use realloc() instead of malloc() + bcopy() for a M_WAITOK malloc(). I prefer the separate statement, and don't really like the M_ZERO flag instead of a separate bzero() either. Kernel realloc() is non-smart and never extends the original allocation, but just does malloc() + bcopy() internally. Since it is so rarely needed in the kernel, it is OK for callers to know this and not use it just to benefit from it ecoming smarter. reallocf() gives much larger simplifications in callers for the single non-M_WAITOK caller. Further cleanups might give as many a 2 correct non-M_WAITOK callers. Bruce