Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752046AbaBHPKF (ORCPT ); Sat, 8 Feb 2014 10:10:05 -0500 Received: from metis.ext.pengutronix.de ([92.198.50.35]:54877 "EHLO metis.ext.pengutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751377AbaBHPKC (ORCPT ); Sat, 8 Feb 2014 10:10:02 -0500 Date: Sat, 8 Feb 2014 16:09:58 +0100 From: Uwe =?iso-8859-1?Q?Kleine-K=F6nig?= To: Greg Kroah-Hartman Cc: Yann Droneaud , linux-kernel@vger.kernel.org, Dmitry Torokhov Subject: Re: [PATCHv4] driver core/platform: don't leak memory allocated for dma_mask Message-ID: <20140208150958.GK17045@pengutronix.de> References: <1389649085-7365-1-git-send-email-ydroneaud@opteya.com> <20140113225606.GA4132@kroah.com> <1389683909-17495-1-git-send-email-ydroneaud@opteya.com> <20140114081944.GO29475@pengutronix.de> <1389693453.1585.26.camel@localhost.localdomain> <20140114103648.GT29475@pengutronix.de> <20140114180204.GD27791@kroah.com> <1390771138-28348-1-git-send-email-ydroneaud@opteya.com> <1390817152-30898-1-git-send-email-ydroneaud@opteya.com> <20140207232005.GA23477@kroah.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20140207232005.GA23477@kroah.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-SA-Exim-Connect-IP: 2001:6f8:1178:2:21e:67ff:fe11:9c5c X-SA-Exim-Mail-From: ukl@pengutronix.de X-SA-Exim-Scanned: No (on metis.ext.pengutronix.de); SAEximRunCond expanded to false X-PTX-Original-Recipient: linux-kernel@vger.kernel.org Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Feb 07, 2014 at 03:20:05PM -0800, Greg Kroah-Hartman wrote: > On Mon, Jan 27, 2014 at 11:05:52AM +0100, Yann Droneaud wrote: > > Since commit 01dcc60a7cb8, platform_device_register_full() is > > available to allocate and register a platform device. > > > > If a dma_mask is provided as part of platform_device_info, > > platform_device_register_full() allocate memory for a u64 using > > kmalloc(). > > > > A comment in the code state that "[t]his memory isn't freed when the > > device is put". > > > > It's never a good thing to leak memory, but there's only very few > > users of platform_device_info's dma_mask, and those are mostly > > "static" devices that are not going to be plugged/unplugged often. > > > > So memory leak is not really an issue, but allocating 8 bytes through > > kmalloc() seems overkill. > > > > And it's a pity to have to allocate 8 bytes for the dma_mask while up > > to 7 bytes are wasted at the end of struct platform_object in the form > > of padding after name field: unfortunately this padding is not used > > when allocating the memory to hold the name. > > > > To address theses issues, this patch adds dma_mask to platform_object > > struct so that it is always allocated for all struct platform_device > > allocated through platform_device_alloc() or platform_device_register_full(). > > And since it's within struct platform_object, dma_mask won't be leaked > > anymore when struct platform_device got released. Storage for dma_mask > > is added almost for free by removing the padding at the end of struct > > platform_object. How does the padding of name change? The only thing that I see changing for .name is that it's a char[] now instead of a char[1]. As it was used as a flexible array already before the padding (which only applies to a stand alone struct platform_object) doesn't matter. I guess that is a tool problem that still some padding changes are reported? Other than that the patch looks good. Uwe > > Padding at the end of the structure is removed by making name a C99 > > flexible array member (eg. a zero sized array). To handle this change, > > memory allocation is updated to take care of allocating an additional > > byte for the NUL terminating character. > > > > No more memory leak, no small allocation, no byte wasted and > > a slight reduction in code size. > > > > Built on Fedora 20, using GCC 4.8, for ARM, i386, x86_64 and PPC64 > > architectures, the size of the object file and the data structure layout > > are updated as follow, produced with commands: > > > > $ size drivers/base/platform.o > > $ pahole drivers/base/platform.o \ > > --recursive \ > > --class_name device,pdev_archdata,platform_device,platform_object > > > > --- size+pahole.next-20140124 > > +++ size+pahole.next-20140124+ > > @@ -1,5 +1,5 @@ > > text data bss dec hex filename > > - 5616 472 32 6120 17e8 obj-arm/drivers/base/platform.o > > + 5572 472 32 6076 17bc obj-arm/drivers/base/platform.o > > struct device { > > struct device * parent; /* 0 4 */ > > struct device_private * p; /* 4 4 */ > > @@ -77,15 +77,15 @@ struct platform_object { > > /* XXX last struct has 4 bytes of padding */ > > > > /* --- cacheline 6 boundary (384 bytes) --- */ > > - char name[1]; /* 384 1 */ > > + u64 dma_mask; /* 384 8 */ > > + char name[0]; /* 392 0 */ > > > > - /* size: 392, cachelines: 7, members: 2 */ > > - /* padding: 7 */ > > + /* size: 392, cachelines: 7, members: 3 */ > > /* paddings: 1, sum paddings: 4 */ > > /* last cacheline: 8 bytes */ > > }; > > > > text data bss dec hex filename > > - 6007 532 32 6571 19ab obj-i386/drivers/base/platform.o > > + 5943 532 32 6507 196b obj-i386/drivers/base/platform.o > > struct device { > > struct device * parent; /* 0 4 */ > > struct device_private * p; /* 4 4 */ > > @@ -161,14 +161,14 @@ struct platform_device { > > struct platform_object { > > struct platform_device pdev; /* 0 392 */ > > /* --- cacheline 6 boundary (384 bytes) was 8 bytes ago --- */ > > - char name[1]; /* 392 1 */ > > + u64 dma_mask; /* 392 8 */ > > + char name[0]; /* 400 0 */ > > > > - /* size: 396, cachelines: 7, members: 2 */ > > - /* padding: 3 */ > > - /* last cacheline: 12 bytes */ > > + /* size: 400, cachelines: 7, members: 3 */ > > + /* last cacheline: 16 bytes */ > > }; > > > > text data bss dec hex filename > > - 8851 2112 48 11011 2b03 obj-ppc64/drivers/base/platform.o > > + 8787 2104 48 10939 2abb obj-ppc64/drivers/base/platform.o > > struct device { > > struct device * parent; /* 0 8 */ > > struct device_private * p; /* 8 8 */ > > @@ -256,14 +256,14 @@ struct platform_device { > > struct platform_object { > > struct platform_device pdev; /* 0 728 */ > > /* --- cacheline 11 boundary (704 bytes) was 24 bytes ago --- */ > > - char name[1]; /* 728 1 */ > > + u64 dma_mask; /* 728 8 */ > > + char name[0]; /* 736 0 */ > > > > - /* size: 736, cachelines: 12, members: 2 */ > > - /* padding: 7 */ > > + /* size: 736, cachelines: 12, members: 3 */ > > /* last cacheline: 32 bytes */ > > }; > > > > text data bss dec hex filename > > - 7100 960 48 8108 1fac obj-x86_64/drivers/base/platform.o > > + 7020 960 48 8028 1f5c obj-x86_64/drivers/base/platform.o > > struct device { > > struct device * parent; /* 0 8 */ > > struct device_private * p; /* 8 8 */ > > @@ -350,9 +350,9 @@ struct platform_device { > > struct platform_object { > > struct platform_device pdev; /* 0 712 */ > > /* --- cacheline 11 boundary (704 bytes) was 8 bytes ago --- */ > > - char name[1]; /* 712 1 */ > > + u64 dma_mask; /* 712 8 */ > > + char name[0]; /* 720 0 */ > > > > - /* size: 720, cachelines: 12, members: 2 */ > > - /* padding: 7 */ > > + /* size: 720, cachelines: 12, members: 3 */ > > /* last cacheline: 16 bytes */ > > }; > > > > Changes from v3 [1]: > > - fixed commit message so that git am doesn't fail. > > > > Changes from v2 [2]: > > - move 'dma_mask' to platform_object so that it's always > > allocated and won't leak on release; remove all previously > > added support functions. > > - use C99 flexible array member for 'name' to remove padding > > at the end of platform_object. > > > > Changes from v1 [3]: > > - remove unneeded kfree() from error path > > - add reference to author/commit adding allocation of dmamask > > > > Changes from v0 [4]: > > - small rewrite to squeeze the patch to a bare minimal > > > > [1] http://lkml.kernel.org/r/1390771138-28348-1-git-send-email-ydroneaud@opteya.com > > https://patchwork.kernel.org/patch/3540081/ > > > > [2] http://lkml.kernel.org/r/1389683909-17495-1-git-send-email-ydroneaud@opteya.com > > https://patchwork.kernel.org/patch/3484411/ > > > > [3] http://lkml.kernel.org/r/1389649085-7365-1-git-send-email-ydroneaud@opteya.com > > https://patchwork.kernel.org/patch/3480961/ > > > > [4] http://lkml.kernel.org/r/1386886207-2735-1-git-send-email-ydroneaud@opteya.com > > > > Cc: Uwe Kleine-K?nig > > Cc: Dmitry Torokhov > > Cc: Greg Kroah-Hartman > > Signed-off-by: Yann Droneaud > > --- > > Hi, > > > > I've fixed the commit message to move the diff on pahole > > outside of the scope of git am. > > Uwe, can I get your review of this? > > thanks, > > greg k-h > -- Pengutronix e.K. | Uwe Kleine-K?nig | Industrial Linux Solutions | http://www.pengutronix.de/ | -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/