From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Wed, 16 Jan 2019 17:13:49 +0200 From: Mike Rapoport Subject: Re: [PATCH 19/21] treewide: add checks for the return value of memblock_alloc*() References: <1547646261-32535-1-git-send-email-rppt@linux.ibm.com> <1547646261-32535-20-git-send-email-rppt@linux.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Message-Id: <20190116151348.GD6643@rapoport-lnx> To: Geert Uytterhoeven Cc: Linux MM , Andrew Morton , Catalin Marinas , Christoph Hellwig , "David S. Miller" , Dennis Zhou , Greentime Hu , Greg Kroah-Hartman , Guan Xuetao , Guo Ren , Heiko Carstens , Mark Salter , Matt Turner , Max Filippov , Michael Ellerman , Michal Simek , Paul Burton , Petr Mladek , Rich Felker , Richard Weinberger , Rob Herring , Russell King , Stafford Horne , Tony Luck , Vineet Gupta , Yoshinori Sato , "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" , kasan-dev@googlegroups.com, alpha , Linux ARM , linux-c6x-dev@linux-c6x.org, "linux-ia64@vger.kernel.org" , Linux Kernel Mailing List , linux-m68k , linux-mips@vger.kernel.org, linux-s390 , Linux-sh list , arcml , linux-um@lists.infradead.org, USB list , linux-xtensa@linux-xtensa.org, linuxppc-dev , Openrisc , sparclinux , "moderated list:H8/300 ARCHITECTURE" , the arch/x86 maintainers , xen-devel@lists.xenproject.org List-ID: On Wed, Jan 16, 2019 at 03:27:35PM +0100, Geert Uytterhoeven wrote: > Hi Mike, > > On Wed, Jan 16, 2019 at 2:46 PM Mike Rapoport wrote: > > Add check for the return value of memblock_alloc*() functions and call > > panic() in case of error. > > The panic message repeats the one used by panicing memblock allocators with > > adjustment of parameters to include only relevant ones. > > > > The replacement was mostly automated with semantic patches like the one > > below with manual massaging of format strings. > > > > @@ > > expression ptr, size, align; > > @@ > > ptr = memblock_alloc(size, align); > > + if (!ptr) > > + panic("%s: Failed to allocate %lu bytes align=0x%lx\n", __func__, > > In general, you want to use %zu for size_t > > > size, align); > > > > Signed-off-by: Mike Rapoport > > Thanks for your patch! > > > 74 files changed, 415 insertions(+), 29 deletions(-) > > I'm wondering if this is really an improvement? >From memblock perspective it's definitely an improvement :) git diff --stat mmotm/master include/linux/memblock.h mm/memblock.c include/linux/memblock.h | 59 ++--------- mm/memblock.c | 249 ++++++++++++++++------------------------------- 2 files changed, 90 insertions(+), 218 deletions(-) > For the normal memory allocator, the trend is to remove printing of errors > from all callers, as the core takes care of that. It's more about allocation errors handling than printing of the errors. Indeed, there is not much that can be done if an early allocation fails, but I believe having an explicit pattern ptr = alloc(); if (!ptr) do_something_about_it(); is clearer than relying on the allocator to panic(). Besides, the diversity of panic and nopanic variants creates a confusion and I've caught several places that call nopanic variant and do not check its return value. > > --- a/arch/alpha/kernel/core_marvel.c > > +++ b/arch/alpha/kernel/core_marvel.c > > @@ -83,6 +83,9 @@ mk_resource_name(int pe, int port, char *str) > > > > sprintf(tmp, "PCI %s PE %d PORT %d", str, pe, port); > > name = memblock_alloc(strlen(tmp) + 1, SMP_CACHE_BYTES); > > + if (!name) > > + panic("%s: Failed to allocate %lu bytes\n", __func__, > > %zu, as strlen() returns size_t. Thanks for spotting it, will fix. > > + strlen(tmp) + 1); > > strcpy(name, tmp); > > > > return name; > > @@ -118,6 +121,9 @@ alloc_io7(unsigned int pe) > > } > > > > io7 = memblock_alloc(sizeof(*io7), SMP_CACHE_BYTES); > > + if (!io7) > > + panic("%s: Failed to allocate %lu bytes\n", __func__, > > %zu, as sizeof() returns size_t. > Probably there are more. Yes, it's hard to get them right in all callers. Yeah :) > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds > -- Sincerely yours, Mike.