Date: Thu, 13 Mar 2014 09:38:51 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Konstantin Belousov <kib@freebsd.org> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r263080 - in head/sys: dev/cpuctl dev/hwpmc kern Message-ID: <20140313081336.C5739@besplex.bde.org> In-Reply-To: <201403121025.s2CAPQRl017239@svn.freebsd.org> References: <201403121025.s2CAPQRl017239@svn.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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 <cemeyer@uw.edu> [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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20140313081336.C5739>