Messages in this thread | | | Date | Fri, 7 Feb 2014 15:20:05 -0800 | From | Greg Kroah-Hartman <> | Subject | Re: [PATCHv4] driver core/platform: don't leak memory allocated for dma_mask |
| |
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. > > 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 <u.kleine-koenig@pengutronix.de> > Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Signed-off-by: Yann Droneaud <ydroneaud@opteya.com> > --- > 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 -- 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/
| |