Re: [PATCHv4] driver core/platform: don't leak memory allocated for dma_mask

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

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