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.
So memory leak is not really an issue, but allocating 8 bytes
through kmalloc() seems overkill, so this patch moves dma_mask
after the platform_device struct, dynamically allocated along
the name buffer.
With dma_mask part of the memory allocated for the platform_device
struct, like name buffer, it will be released with it:
no memory leak, no small allocation.
The drawback is the additional code needed to handle
dma_mask allocation:
Before (on next-20140113 with gcc-4.8):
text data bss dec hex filename
5600 472 32 6104 17d8 obj-arm/drivers/base/platform.o
5927 532 32 6491 195b obj-i386/drivers/base/platform.o
7036 960 48 8044 1f6c obj-x86_64/drivers/base/platform.o
After:
text data bss dec hex filename
5672 472 32 6176 1820 obj-arm/drivers/base/platform.o
6007 532 32 6571 19ab obj-i386/drivers/base/platform.o
7132 960 48 8140 1fcc obj-x86_64/drivers/base/platform.o
Changes from v0 [1]:
- small rewrite to squeeze the patch to a bare minimal
[1] http://lkml.kernel.org/r/[email protected]
Signed-off-by: Yann Droneaud <[email protected]>
---
drivers/base/platform.c | 82 +++++++++++++++++++++++++++++++++++++------------
1 file changed, 62 insertions(+), 20 deletions(-)
diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 3a94b799f166..0741be056885 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -157,7 +157,7 @@ EXPORT_SYMBOL_GPL(platform_add_devices);
struct platform_object {
struct platform_device pdev;
- char name[1];
+ char payload[0];
};
/**
@@ -186,6 +186,25 @@ static void platform_device_release(struct device *dev)
kfree(pa);
}
+static struct platform_object *platform_object_alloc(size_t payload)
+{
+ struct platform_object *pa;
+
+ pa = kzalloc(sizeof(*pa) + payload, GFP_KERNEL);
+
+ return pa;
+}
+
+static void platform_object_init(struct platform_object *pa,
+ const char *name, int id)
+{
+ pa->pdev.name = name;
+ pa->pdev.id = id;
+ device_initialize(&pa->pdev.dev);
+ pa->pdev.dev.release = platform_device_release;
+ arch_setup_pdev_archdata(&pa->pdev);
+}
+
/**
* platform_device_alloc - create a platform device
* @name: base name of the device we're adding
@@ -198,14 +217,10 @@ struct platform_device *platform_device_alloc(const char *name, int id)
{
struct platform_object *pa;
- pa = kzalloc(sizeof(struct platform_object) + strlen(name), GFP_KERNEL);
+ pa = platform_object_alloc(strlen(name) + 1);
if (pa) {
- strcpy(pa->name, name);
- pa->pdev.name = pa->name;
- pa->pdev.id = id;
- device_initialize(&pa->pdev.dev);
- pa->pdev.dev.release = platform_device_release;
- arch_setup_pdev_archdata(&pa->pdev);
+ strcpy(pa->payload, name);
+ platform_object_init(pa, pa->payload, id);
}
return pa ? &pa->pdev : NULL;
@@ -213,6 +228,39 @@ struct platform_device *platform_device_alloc(const char *name, int id)
EXPORT_SYMBOL_GPL(platform_device_alloc);
/**
+ * platform_device_dmamask_alloc - create a platform device suitable to hold a dmamask
+ * @name: base name of the device we're adding
+ * @id: instance id
+ *
+ * Create a platform device object which can have other objects attached
+ * to it, and which will have attached objects freed when it is released.
+ */
+static struct platform_device *platform_device_dmamask_alloc(const char *name,
+ int id)
+{
+ struct platform_object *pa;
+ const size_t padding = (((offsetof(struct platform_object, payload) +
+ (__alignof__(u64) - 1)) &
+ ~(__alignof__(u64) - 1)) -
+ offsetof(struct platform_object, payload));
+
+ pa = platform_object_alloc(padding + sizeof(u64) + strlen(name) + 1);
+ if (pa) {
+ char *payload = pa->payload + padding;
+ /*
+ * Conceptually dma_mask in struct device should not be a pointer.
+ * See http://thread.gmane.org/gmane.linux.kernel.pci/9081
+ */
+ pa->pdev.dev.dma_mask = (void *)payload;
+ payload += sizeof(u64);
+ strcpy(payload, name);
+ platform_object_init(pa, payload, id);
+ }
+
+ return pa ? &pa->pdev : NULL;
+}
+
+/**
* platform_device_add_resources - add resources to a platform device
* @pdev: platform device allocated by platform_device_alloc to add resources to
* @res: set of resources that needs to be allocated for the device
@@ -427,7 +475,12 @@ struct platform_device *platform_device_register_full(
int ret = -ENOMEM;
struct platform_device *pdev;
- pdev = platform_device_alloc(pdevinfo->name, pdevinfo->id);
+ if (!pdevinfo->dma_mask)
+ pdev = platform_device_alloc(pdevinfo->name, pdevinfo->id);
+ else
+ pdev = platform_device_dmamask_alloc(pdevinfo->name,
+ pdevinfo->id);
+
if (!pdev)
goto err_alloc;
@@ -435,17 +488,6 @@ struct platform_device *platform_device_register_full(
ACPI_COMPANION_SET(&pdev->dev, pdevinfo->acpi_node.companion);
if (pdevinfo->dma_mask) {
- /*
- * This memory isn't freed when the device is put,
- * I don't have a nice idea for that though. Conceptually
- * dma_mask in struct device should not be a pointer.
- * See http://thread.gmane.org/gmane.linux.kernel.pci/9081
- */
- pdev->dev.dma_mask =
- kmalloc(sizeof(*pdev->dev.dma_mask), GFP_KERNEL);
- if (!pdev->dev.dma_mask)
- goto err;
-
*pdev->dev.dma_mask = pdevinfo->dma_mask;
pdev->dev.coherent_dma_mask = pdevinfo->dma_mask;
}
--
1.8.4.2
On Mon, Jan 13, 2014 at 10:38:05PM +0100, Yann Droneaud wrote:
> 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.
Why haven't you cc:ed the author of that comment? He would be best to
evaluate if this patch is good enough or not.
And is leaking that memory really an issue? As you point out, these
aren't devices that are going to go away (I'd argue that no platform
device should ever be a removable device, but that's a longer
argument...)
Please resend and cc: all of the needed developers.
thanks,
greg k-h
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.
So memory leak is not really an issue, but allocating 8 bytes
through kmalloc() seems overkill, so this patch moves dma_mask
after the platform_device struct, dynamically allocated along
the name buffer.
With dma_mask part of the memory allocated for the platform_device
struct, like name buffer, it will be released with it:
no memory leak, no small allocation.
The drawback is the additional code needed to handle
dma_mask allocation:
Before (on next-20140113 with gcc-4.8):
text data bss dec hex filename
5600 472 32 6104 17d8 obj-arm/drivers/base/platform.o
5927 532 32 6491 195b obj-i386/drivers/base/platform.o
7036 960 48 8044 1f6c obj-x86_64/drivers/base/platform.o
After:
text data bss dec hex filename
5668 472 32 6172 181c obj-arm/drivers/base/platform.o
6007 532 32 6571 19ab obj-i386/drivers/base/platform.o
7132 960 48 8140 1fcc obj-x86_64/drivers/base/platform.o
Changes from v1 [1]:
- remove unneeded kfree() from error path
- add reference to author/commit adding allocation of dmamask
Changes from v0 [2]:
- small rewrite to squeeze the patch to a bare minimal
[1] http://lkml.kernel.org/r/[email protected]
https://patchwork.kernel.org/patch/3480961/
[2] http://lkml.kernel.org/r/[email protected]
Cc: Uwe Kleine-König <[email protected]>
Cc: Dmitry Torokhov <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Signed-off-by: Yann Droneaud <[email protected]>
---
Hi Greg,
> Why haven't you cc:ed the author of that comment? He would be best to
> evaluate if this patch is good enough or not.
>
I must admit I was a bit lazy: I've tried ./script/get_maintainer.pl --git / --git-blame
but the results scare me, so I've send the patch to the maintainer only. (And somehow
I've thought you wrote that comment).
> And is leaking that memory really an issue? As you point out, these
> aren't devices that are going to go away (I'd argue that no platform
> device should ever be a removable device, but that's a longer
> argument...)
>
I've seen some removable platform driver ... and, in fact, wrote some:
when writing/testing it, being able to remove the devices/driver is a must.
> Please resend and cc: all of the needed developers.
>
Thanks for the advice.
Regards.
drivers/base/platform.c | 83 ++++++++++++++++++++++++++++++++++++-------------
1 file changed, 62 insertions(+), 21 deletions(-)
diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 3a94b799f166..6e3e639fb886 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -157,7 +157,7 @@ EXPORT_SYMBOL_GPL(platform_add_devices);
struct platform_object {
struct platform_device pdev;
- char name[1];
+ char payload[0];
};
/**
@@ -186,6 +186,25 @@ static void platform_device_release(struct device *dev)
kfree(pa);
}
+static struct platform_object *platform_object_alloc(size_t payload)
+{
+ struct platform_object *pa;
+
+ pa = kzalloc(sizeof(*pa) + payload, GFP_KERNEL);
+
+ return pa;
+}
+
+static void platform_object_init(struct platform_object *pa,
+ const char *name, int id)
+{
+ pa->pdev.name = name;
+ pa->pdev.id = id;
+ device_initialize(&pa->pdev.dev);
+ pa->pdev.dev.release = platform_device_release;
+ arch_setup_pdev_archdata(&pa->pdev);
+}
+
/**
* platform_device_alloc - create a platform device
* @name: base name of the device we're adding
@@ -198,14 +217,10 @@ struct platform_device *platform_device_alloc(const char *name, int id)
{
struct platform_object *pa;
- pa = kzalloc(sizeof(struct platform_object) + strlen(name), GFP_KERNEL);
+ pa = platform_object_alloc(strlen(name) + 1);
if (pa) {
- strcpy(pa->name, name);
- pa->pdev.name = pa->name;
- pa->pdev.id = id;
- device_initialize(&pa->pdev.dev);
- pa->pdev.dev.release = platform_device_release;
- arch_setup_pdev_archdata(&pa->pdev);
+ strcpy(pa->payload, name);
+ platform_object_init(pa, pa->payload, id);
}
return pa ? &pa->pdev : NULL;
@@ -213,6 +228,39 @@ struct platform_device *platform_device_alloc(const char *name, int id)
EXPORT_SYMBOL_GPL(platform_device_alloc);
/**
+ * platform_device_dmamask_alloc - create a platform device suitable to hold a dmamask
+ * @name: base name of the device we're adding
+ * @id: instance id
+ *
+ * Create a platform device object which can have other objects attached
+ * to it, and which will have attached objects freed when it is released.
+ */
+static struct platform_device *platform_device_dmamask_alloc(const char *name,
+ int id)
+{
+ struct platform_object *pa;
+ const size_t padding = (((offsetof(struct platform_object, payload) +
+ (__alignof__(u64) - 1)) &
+ ~(__alignof__(u64) - 1)) -
+ offsetof(struct platform_object, payload));
+
+ pa = platform_object_alloc(padding + sizeof(u64) + strlen(name) + 1);
+ if (pa) {
+ char *payload = pa->payload + padding;
+ /*
+ * Conceptually dma_mask in struct device should not be a pointer.
+ * See http://thread.gmane.org/gmane.linux.kernel.pci/9081
+ */
+ pa->pdev.dev.dma_mask = (void *)payload;
+ payload += sizeof(u64);
+ strcpy(payload, name);
+ platform_object_init(pa, payload, id);
+ }
+
+ return pa ? &pa->pdev : NULL;
+}
+
+/**
* platform_device_add_resources - add resources to a platform device
* @pdev: platform device allocated by platform_device_alloc to add resources to
* @res: set of resources that needs to be allocated for the device
@@ -427,7 +475,12 @@ struct platform_device *platform_device_register_full(
int ret = -ENOMEM;
struct platform_device *pdev;
- pdev = platform_device_alloc(pdevinfo->name, pdevinfo->id);
+ if (!pdevinfo->dma_mask)
+ pdev = platform_device_alloc(pdevinfo->name, pdevinfo->id);
+ else
+ pdev = platform_device_dmamask_alloc(pdevinfo->name,
+ pdevinfo->id);
+
if (!pdev)
goto err_alloc;
@@ -435,17 +488,6 @@ struct platform_device *platform_device_register_full(
ACPI_COMPANION_SET(&pdev->dev, pdevinfo->acpi_node.companion);
if (pdevinfo->dma_mask) {
- /*
- * This memory isn't freed when the device is put,
- * I don't have a nice idea for that though. Conceptually
- * dma_mask in struct device should not be a pointer.
- * See http://thread.gmane.org/gmane.linux.kernel.pci/9081
- */
- pdev->dev.dma_mask =
- kmalloc(sizeof(*pdev->dev.dma_mask), GFP_KERNEL);
- if (!pdev->dev.dma_mask)
- goto err;
-
*pdev->dev.dma_mask = pdevinfo->dma_mask;
pdev->dev.coherent_dma_mask = pdevinfo->dma_mask;
}
@@ -464,7 +506,6 @@ struct platform_device *platform_device_register_full(
if (ret) {
err:
ACPI_COMPANION_SET(&pdev->dev, NULL);
- kfree(pdev->dev.dma_mask);
err_alloc:
platform_device_put(pdev);
--
1.8.4.2
Hello Yann,
On Tue, Jan 14, 2014 at 08:18:29AM +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.
>
> So memory leak is not really an issue, but allocating 8 bytes
> through kmalloc() seems overkill, so this patch moves dma_mask
> after the platform_device struct, dynamically allocated along
> the name buffer.
>
> With dma_mask part of the memory allocated for the platform_device
> struct, like name buffer, it will be released with it:
> no memory leak, no small allocation.
>
> The drawback is the additional code needed to handle
> dma_mask allocation:
>
> Before (on next-20140113 with gcc-4.8):
> text data bss dec hex filename
> 5600 472 32 6104 17d8 obj-arm/drivers/base/platform.o
> 5927 532 32 6491 195b obj-i386/drivers/base/platform.o
> 7036 960 48 8044 1f6c obj-x86_64/drivers/base/platform.o
>
> After:
> text data bss dec hex filename
> 5668 472 32 6172 181c obj-arm/drivers/base/platform.o
> 6007 532 32 6571 19ab obj-i386/drivers/base/platform.o
> 7132 960 48 8140 1fcc obj-x86_64/drivers/base/platform.o
>
> Changes from v1 [1]:
> - remove unneeded kfree() from error path
> - add reference to author/commit adding allocation of dmamask
>
> Changes from v0 [2]:
> - small rewrite to squeeze the patch to a bare minimal
>
> [1] http://lkml.kernel.org/r/[email protected]
> https://patchwork.kernel.org/patch/3480961/
>
> [2] http://lkml.kernel.org/r/[email protected]
>
> Cc: Uwe Kleine-K?nig <[email protected]>
> Cc: Dmitry Torokhov <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>
> Signed-off-by: Yann Droneaud <[email protected]>
> ---
>
> Hi Greg,
>
> > Why haven't you cc:ed the author of that comment? He would be best to
> > evaluate if this patch is good enough or not.
> >
>
> I must admit I was a bit lazy: I've tried ./script/get_maintainer.pl --git / --git-blame
> but the results scare me, so I've send the patch to the maintainer only. (And somehow
> I've thought you wrote that comment).
>
> > And is leaking that memory really an issue? As you point out, these
> > aren't devices that are going to go away (I'd argue that no platform
> > device should ever be a removable device, but that's a longer
> > argument...)
> >
>
> I've seen some removable platform driver ... and, in fact, wrote some:
> when writing/testing it, being able to remove the devices/driver is a must.
>
> > Please resend and cc: all of the needed developers.
> >
>
> Thanks for the advice.
>
> Regards.
>
> drivers/base/platform.c | 83 ++++++++++++++++++++++++++++++++++++-------------
> 1 file changed, 62 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index 3a94b799f166..6e3e639fb886 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -157,7 +157,7 @@ EXPORT_SYMBOL_GPL(platform_add_devices);
>
> struct platform_object {
> struct platform_device pdev;
> - char name[1];
> + char payload[0];
I don't know the recent minimal versions needed to compile the kernel
and since when gcc supports c99 flexible array members, but I would
expect that they just work. Having said that I'd prefer using that one,
i.e. use
char payload[];
> };
>
> /**
> @@ -186,6 +186,25 @@ static void platform_device_release(struct device *dev)
> kfree(pa);
> }
>
> +static struct platform_object *platform_object_alloc(size_t payload)
> +{
> + struct platform_object *pa;
> +
> + pa = kzalloc(sizeof(*pa) + payload, GFP_KERNEL);
> +
> + return pa;
> +}
> +
> +static void platform_object_init(struct platform_object *pa,
> + const char *name, int id)
> +{
> + pa->pdev.name = name;
> + pa->pdev.id = id;
> + device_initialize(&pa->pdev.dev);
> + pa->pdev.dev.release = platform_device_release;
> + arch_setup_pdev_archdata(&pa->pdev);
> +}
> +
> /**
> * platform_device_alloc - create a platform device
> * @name: base name of the device we're adding
> @@ -198,14 +217,10 @@ struct platform_device *platform_device_alloc(const char *name, int id)
> {
> struct platform_object *pa;
>
> - pa = kzalloc(sizeof(struct platform_object) + strlen(name), GFP_KERNEL);
> + pa = platform_object_alloc(strlen(name) + 1);
> if (pa) {
> - strcpy(pa->name, name);
> - pa->pdev.name = pa->name;
> - pa->pdev.id = id;
> - device_initialize(&pa->pdev.dev);
> - pa->pdev.dev.release = platform_device_release;
> - arch_setup_pdev_archdata(&pa->pdev);
> + strcpy(pa->payload, name);
> + platform_object_init(pa, pa->payload, id);
> }
>
> return pa ? &pa->pdev : NULL;
> @@ -213,6 +228,39 @@ struct platform_device *platform_device_alloc(const char *name, int id)
> EXPORT_SYMBOL_GPL(platform_device_alloc);
>
> /**
> + * platform_device_dmamask_alloc - create a platform device suitable to hold a dmamask
> + * @name: base name of the device we're adding
> + * @id: instance id
> + *
> + * Create a platform device object which can have other objects attached
> + * to it, and which will have attached objects freed when it is released.
> + */
> +static struct platform_device *platform_device_dmamask_alloc(const char *name,
> + int id)
> +{
> + struct platform_object *pa;
> + const size_t padding = (((offsetof(struct platform_object, payload) +
> + (__alignof__(u64) - 1)) &
> + ~(__alignof__(u64) - 1)) -
> + offsetof(struct platform_object, payload));
> +
> + pa = platform_object_alloc(padding + sizeof(u64) + strlen(name) + 1);
> + if (pa) {
> + char *payload = pa->payload + padding;
> + /*
> + * Conceptually dma_mask in struct device should not be a pointer.
> + * See http://thread.gmane.org/gmane.linux.kernel.pci/9081
> + */
> + pa->pdev.dev.dma_mask = (void *)payload;
> + payload += sizeof(u64);
> + strcpy(payload, name);
> + platform_object_init(pa, payload, id);
> + }
> +
> + return pa ? &pa->pdev : NULL;
> +}
This looks all complicated. Did you think about spending the extra
memory and add a dma_mask to platform_object? That should simplify the
code quite a bit which probably is worth the extra memory being used.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Hi Uwe,
Le mardi 14 janvier 2014 à 09:19 +0100, Uwe Kleine-König a écrit :
> On Tue, Jan 14, 2014 at 08:18:29AM +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".
> >
[...]
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> > index 3a94b799f166..6e3e639fb886 100644
> > --- a/drivers/base/platform.c
> > +++ b/drivers/base/platform.c
> > @@ -157,7 +157,7 @@ EXPORT_SYMBOL_GPL(platform_add_devices);
> >
> > struct platform_object {
> > struct platform_device pdev;
> > - char name[1];
> > + char payload[0];
> I don't know the recent minimal versions needed to compile the kernel
> and since when gcc supports c99 flexible array members, but I would
> expect that they just work. Having said that I'd prefer using that one,
> i.e. use
> char payload[];
> > };
I'm not confident with flexible array when using sizeof(), offsetof(),
etc. I will try to use the c99 feature.
> > +static struct platform_device *platform_device_dmamask_alloc(const char *name,
> > + int id)
> > +{
> > + struct platform_object *pa;
> > + const size_t padding = (((offsetof(struct platform_object, payload) +
> > + (__alignof__(u64) - 1)) &
> > + ~(__alignof__(u64) - 1)) -
> > + offsetof(struct platform_object, payload));
> > +
> > + pa = platform_object_alloc(padding + sizeof(u64) + strlen(name) + 1);
> > + if (pa) {
> > + char *payload = pa->payload + padding;
> > + /*
> > + * Conceptually dma_mask in struct device should not be a pointer.
> > + * See http://thread.gmane.org/gmane.linux.kernel.pci/9081
> > + */
> > + pa->pdev.dev.dma_mask = (void *)payload;
> > + payload += sizeof(u64);
> > + strcpy(payload, name);
> > + platform_object_init(pa, payload, id);
> > + }
> > +
> > + return pa ? &pa->pdev : NULL;
> > +}
> This looks all complicated. Did you think about spending the extra
> memory and add a dma_mask to platform_object? That should simplify the
> code quite a bit which probably is worth the extra memory being used.
>
You could have did it in the first place. But you choose to allocate a
chunk of memory for the u64. I believe there's a reason ;)
I will try to get some figure on the number of platform_device
registered with a dmamask versus without a dmamask: adding the u64 to
all platform_object might cost more memory than the extra code (1 branch
and a function).
Regards.
--
Yann Droneaud
OPTEYA
Hello Yann,
On Tue, Jan 14, 2014 at 10:57:33AM +0100, Yann Droneaud wrote:
> Le mardi 14 janvier 2014 ? 09:19 +0100, Uwe Kleine-K?nig a ?crit :
> > On Tue, Jan 14, 2014 at 08:18:29AM +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".
> > >
>
> [...]
>
> > diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> > > index 3a94b799f166..6e3e639fb886 100644
> > > --- a/drivers/base/platform.c
> > > +++ b/drivers/base/platform.c
> > > @@ -157,7 +157,7 @@ EXPORT_SYMBOL_GPL(platform_add_devices);
> > >
> > > struct platform_object {
> > > struct platform_device pdev;
> > > - char name[1];
> > > + char payload[0];
> > I don't know the recent minimal versions needed to compile the kernel
> > and since when gcc supports c99 flexible array members, but I would
> > expect that they just work. Having said that I'd prefer using that one,
> > i.e. use
> > char payload[];
> > > };
>
> I'm not confident with flexible array when using sizeof(), offsetof(),
> etc. I will try to use the c99 feature.
sizeof etc. will work. I'm not sure about c99 in general, but gcc gets
it right.
> > > +static struct platform_device *platform_device_dmamask_alloc(const char *name,
> > > + int id)
> > > +{
> > > + struct platform_object *pa;
> > > + const size_t padding = (((offsetof(struct platform_object, payload) +
> > > + (__alignof__(u64) - 1)) &
> > > + ~(__alignof__(u64) - 1)) -
> > > + offsetof(struct platform_object, payload));
> > > +
> > > + pa = platform_object_alloc(padding + sizeof(u64) + strlen(name) + 1);
> > > + if (pa) {
> > > + char *payload = pa->payload + padding;
> > > + /*
> > > + * Conceptually dma_mask in struct device should not be a pointer.
> > > + * See http://thread.gmane.org/gmane.linux.kernel.pci/9081
> > > + */
> > > + pa->pdev.dev.dma_mask = (void *)payload;
> > > + payload += sizeof(u64);
> > > + strcpy(payload, name);
> > > + platform_object_init(pa, payload, id);
> > > + }
> > > +
> > > + return pa ? &pa->pdev : NULL;
> > > +}
> > This looks all complicated. Did you think about spending the extra
> > memory and add a dma_mask to platform_object? That should simplify the
> > code quite a bit which probably is worth the extra memory being used.
> >
>
> You could have did it in the first place. But you choose to allocate a
> chunk of memory for the u64. I believe there's a reason ;)
Yeah, I think the reason is that back then I was younger and didn't
value maintainability higher than saving a few bytes. :-)
> I will try to get some figure on the number of platform_device
> registered with a dmamask versus without a dmamask: adding the u64 to
> all platform_object might cost more memory than the extra code (1 branch
> and a function).
Also take into account
sizeof(struct platform_object) + strlen(average device)
Before and after the change. ISTR that the first summand is ~300 (on
ARM). With putting dma_mask unconditionally into platform_object this
goes up by 8, IMHO that is OK.
In return you save that alignment hassle and both your patch and the
resulting code become simpler.
YMMV, best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
On Tue, Jan 14, 2014 at 11:36:48AM +0100, Uwe Kleine-K?nig wrote:
> > I will try to get some figure on the number of platform_device
> > registered with a dmamask versus without a dmamask: adding the u64 to
> > all platform_object might cost more memory than the extra code (1 branch
> > and a function).
> Also take into account
>
> sizeof(struct platform_object) + strlen(average device)
>
> Before and after the change. ISTR that the first summand is ~300 (on
> ARM). With putting dma_mask unconditionally into platform_object this
> goes up by 8, IMHO that is OK.
>
> In return you save that alignment hassle and both your patch and the
> resulting code become simpler.
Yes, please do it this way, it makes it more obvious as to exactly what
is going on, which is more valuable than trying to be "tricky" and save
a few bytes of ram or rom.
thanks,
greg k-h
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 v2 [1]:
- 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 [2]:
- remove unneeded kfree() from error path
- add reference to author/commit adding allocation of dmamask
Changes from v0 [3]:
- small rewrite to squeeze the patch to a bare minimal
[1] http://lkml.kernel.org/r/[email protected]
https://patchwork.kernel.org/patch/3484411/
[2] http://lkml.kernel.org/r/[email protected]
https://patchwork.kernel.org/patch/3480961/
[3] http://lkml.kernel.org/r/[email protected]
Cc: Uwe Kleine-König <[email protected]>
Cc: Dmitry Torokhov <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Signed-off-by: Yann Droneaud <[email protected]>
---
Hi Greg and Uwe,
I've tried to add dma_mask to platform_object,
and found it was good. Here's the patch.
Then I tried to "fix" existing drivers to remove
the no more needed allocation for dma_mask. I've used
a spatch/coccinelle semantic patch such as:
@catchall1@
identifier func;
struct platform_device * pdev;
expression E;
@@
func(...) {
<...
pdev = platform_device_alloc(...);
...
/*
TODO: use dma_mask allocated by platform_device_alloc()
pdev->dev.dma_mask = E;
*/
...>
}
I've found that some drivers use a pointer to the inner
dev.coherent_mask.
Is this could be another option to not have to allocate
a placeholder for the dma_mask ?
Regards.
--
Yann Droneaud
OPTEYA
drivers/base/platform.c | 17 +++++++----------
1 file changed, 7 insertions(+), 10 deletions(-)
diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index bc78848dd59a..1f6d1e832e03 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -157,7 +157,8 @@ EXPORT_SYMBOL_GPL(platform_add_devices);
struct platform_object {
struct platform_device pdev;
- char name[1];
+ u64 dma_mask;
+ char name[];
};
/**
@@ -193,16 +194,20 @@ static void platform_device_release(struct device *dev)
*
* Create a platform device object which can have other objects attached
* to it, and which will have attached objects freed when it is released.
+ *
+ * The associated struct device will be set up so that its dma_mask field
+ * is a valid pointer to an u64. This pointer must not be free'd manually.
*/
struct platform_device *platform_device_alloc(const char *name, int id)
{
struct platform_object *pa;
- pa = kzalloc(sizeof(struct platform_object) + strlen(name), GFP_KERNEL);
+ pa = kzalloc(sizeof(*pa) + strlen(name) + 1, GFP_KERNEL);
if (pa) {
strcpy(pa->name, name);
pa->pdev.name = pa->name;
pa->pdev.id = id;
+ pa->pdev.dev.dma_mask = &pa->dma_mask;
device_initialize(&pa->pdev.dev);
pa->pdev.dev.release = platform_device_release;
arch_setup_pdev_archdata(&pa->pdev);
@@ -436,16 +441,9 @@ struct platform_device *platform_device_register_full(
if (pdevinfo->dma_mask) {
/*
- * This memory isn't freed when the device is put,
- * I don't have a nice idea for that though. Conceptually
* dma_mask in struct device should not be a pointer.
* See http://thread.gmane.org/gmane.linux.kernel.pci/9081
*/
- pdev->dev.dma_mask =
- kmalloc(sizeof(*pdev->dev.dma_mask), GFP_KERNEL);
- if (!pdev->dev.dma_mask)
- goto err;
-
*pdev->dev.dma_mask = pdevinfo->dma_mask;
pdev->dev.coherent_dma_mask = pdevinfo->dma_mask;
}
@@ -464,7 +462,6 @@ struct platform_device *platform_device_register_full(
if (ret) {
err:
ACPI_COMPANION_SET(&pdev->dev, NULL);
- kfree(pdev->dev.dma_mask);
err_alloc:
platform_device_put(pdev);
--
1.8.5.3
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/[email protected]
https://patchwork.kernel.org/patch/3540081/
[2] http://lkml.kernel.org/r/[email protected]
https://patchwork.kernel.org/patch/3484411/
[3] http://lkml.kernel.org/r/[email protected]
https://patchwork.kernel.org/patch/3480961/
[4] http://lkml.kernel.org/r/[email protected]
Cc: Uwe Kleine-König <[email protected]>
Cc: Dmitry Torokhov <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Signed-off-by: Yann Droneaud <[email protected]>
---
Hi,
I've fixed the commit message to move the diff on pahole
outside of the scope of git am.
Regards.
drivers/base/platform.c | 17 +++++++----------
1 file changed, 7 insertions(+), 10 deletions(-)
diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index bc78848dd59a..1f6d1e832e03 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -157,7 +157,8 @@ EXPORT_SYMBOL_GPL(platform_add_devices);
struct platform_object {
struct platform_device pdev;
- char name[1];
+ u64 dma_mask;
+ char name[];
};
/**
@@ -193,16 +194,20 @@ static void platform_device_release(struct device *dev)
*
* Create a platform device object which can have other objects attached
* to it, and which will have attached objects freed when it is released.
+ *
+ * The associated struct device will be set up so that its dma_mask field
+ * is a valid pointer to an u64. This pointer must not be free'd manually.
*/
struct platform_device *platform_device_alloc(const char *name, int id)
{
struct platform_object *pa;
- pa = kzalloc(sizeof(struct platform_object) + strlen(name), GFP_KERNEL);
+ pa = kzalloc(sizeof(*pa) + strlen(name) + 1, GFP_KERNEL);
if (pa) {
strcpy(pa->name, name);
pa->pdev.name = pa->name;
pa->pdev.id = id;
+ pa->pdev.dev.dma_mask = &pa->dma_mask;
device_initialize(&pa->pdev.dev);
pa->pdev.dev.release = platform_device_release;
arch_setup_pdev_archdata(&pa->pdev);
@@ -436,16 +441,9 @@ struct platform_device *platform_device_register_full(
if (pdevinfo->dma_mask) {
/*
- * This memory isn't freed when the device is put,
- * I don't have a nice idea for that though. Conceptually
* dma_mask in struct device should not be a pointer.
* See http://thread.gmane.org/gmane.linux.kernel.pci/9081
*/
- pdev->dev.dma_mask =
- kmalloc(sizeof(*pdev->dev.dma_mask), GFP_KERNEL);
- if (!pdev->dev.dma_mask)
- goto err;
-
*pdev->dev.dma_mask = pdevinfo->dma_mask;
pdev->dev.coherent_dma_mask = pdevinfo->dma_mask;
}
@@ -464,7 +462,6 @@ struct platform_device *platform_device_register_full(
if (ret) {
err:
ACPI_COMPANION_SET(&pdev->dev, NULL);
- kfree(pdev->dev.dma_mask);
err_alloc:
platform_device_put(pdev);
--
1.8.5.3
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/[email protected]
> https://patchwork.kernel.org/patch/3540081/
>
> [2] http://lkml.kernel.org/r/[email protected]
> https://patchwork.kernel.org/patch/3484411/
>
> [3] http://lkml.kernel.org/r/[email protected]
> https://patchwork.kernel.org/patch/3480961/
>
> [4] http://lkml.kernel.org/r/[email protected]
>
> Cc: Uwe Kleine-K?nig <[email protected]>
> Cc: Dmitry Torokhov <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>
> Signed-off-by: Yann Droneaud <[email protected]>
> ---
> 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
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/[email protected]
> > https://patchwork.kernel.org/patch/3540081/
> >
> > [2] http://lkml.kernel.org/r/[email protected]
> > https://patchwork.kernel.org/patch/3484411/
> >
> > [3] http://lkml.kernel.org/r/[email protected]
> > https://patchwork.kernel.org/patch/3480961/
> >
> > [4] http://lkml.kernel.org/r/[email protected]
> >
> > Cc: Uwe Kleine-K?nig <[email protected]>
> > Cc: Dmitry Torokhov <[email protected]>
> > Cc: Greg Kroah-Hartman <[email protected]>
> > Signed-off-by: Yann Droneaud <[email protected]>
> > ---
> > 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/ |
Hi,
Le samedi 08 février 2014 à 16:09 +0100, Uwe Kleine-König a écrit :
> 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?
>
When name is defined as char name[1] at the end of the structure, the
compiler is required to add padding after it (since the structure is not
"packed" through some compiler extension).
This padding is added in order to have a size multiple of the highest
required alignement for types insided the structure. This is required so
that each item of an array of "struct platform_object" are aligned.
Changing name[1] to name[0] or name[] free the compiler from adding
storage space for name and thus remove the need for padding after it.
> Other than that the patch looks good.
>
Thanks a lot.
--
Yann Droneaud
OPTEYA
Hello Yann,
On Sun, Feb 09, 2014 at 08:47:22AM +0100, Yann Droneaud wrote:
> Hi,
>
> Le samedi 08 f?vrier 2014 ? 16:09 +0100, Uwe Kleine-K?nig a ?crit :
> > 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?
> >
>
> When name is defined as char name[1] at the end of the structure, the
> compiler is required to add padding after it (since the structure is not
> "packed" through some compiler extension).
> This padding is added in order to have a size multiple of the highest
> required alignement for types insided the structure. This is required so
> that each item of an array of "struct platform_object" are aligned.
>
> Changing name[1] to name[0] or name[] free the compiler from adding
> storage space for name and thus remove the need for padding after it.
Ah, I see, even though .name was used as a flexible array there was too
much allocated because sizeof(struct platform_object) includes the 7
bytes of padding.
Maybe it makes sense to split the patch into a) s/name[1]/name[]/ and b)
add dma_mask? Up to you, you can have my Ack also for the single patch.
Acked-by: Uwe Kleine-K?nig <[email protected]>
Thanks
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
On Sun, Feb 09, 2014 at 10:30:32AM +0100, Uwe Kleine-K?nig wrote:
> Hello Yann,
>
> On Sun, Feb 09, 2014 at 08:47:22AM +0100, Yann Droneaud wrote:
> > Hi,
> >
> > Le samedi 08 f?vrier 2014 ? 16:09 +0100, Uwe Kleine-K?nig a ?crit :
> > > 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?
> > >
> >
> > When name is defined as char name[1] at the end of the structure, the
> > compiler is required to add padding after it (since the structure is not
> > "packed" through some compiler extension).
> > This padding is added in order to have a size multiple of the highest
> > required alignement for types insided the structure. This is required so
> > that each item of an array of "struct platform_object" are aligned.
> >
> > Changing name[1] to name[0] or name[] free the compiler from adding
> > storage space for name and thus remove the need for padding after it.
> Ah, I see, even though .name was used as a flexible array there was too
> much allocated because sizeof(struct platform_object) includes the 7
> bytes of padding.
>
> Maybe it makes sense to split the patch into a) s/name[1]/name[]/ and b)
> add dma_mask? Up to you, you can have my Ack also for the single patch.
>
> Acked-by: Uwe Kleine-K?nig <[email protected]>
Breaking it up would be good, if for no other reason than people got
confused with it as-is.
thanks,
greg k-h