Patchwork Re: [PATCH] rtc: pl030: Use devm_kzalloc() instead of kmalloc()

login
register
mail settings
Submitter Andrew Morton
Date Oct. 10, 2013, 11:06 p.m.
Message ID <20131010160603.1489fe054c2bdc0a0eb9460f@linux-foundation.org>
Download mbox | patch
Permalink /patch/282494/
State New
Headers show

Comments

Andrew Morton - Oct. 10, 2013, 11:06 p.m.
On Tue, 08 Oct 2013 21:59:27 -0700 Joe Perches <joe@perches.com> wrote:

> I was a bit surprised to find there isn't a devm_kmalloc.

Yes, the unconditional memset is silly.  Especially when the
function has a handy gfp_t and could be passed __GFP_ZERO.

The comment says "managed kzalloc/kfree for device drivers, no kmalloc,
always use kzalloc".  There's no explanation for this - it looks like
some ideological thing.
Joe Perches - Oct. 10, 2013, 11:14 p.m.
On Thu, 2013-10-10 at 16:06 -0700, Andrew Morton wrote:
> On Tue, 08 Oct 2013 21:59:27 -0700 Joe Perches <joe@perches.com> wrote:
> 
> > I was a bit surprised to find there isn't a devm_kmalloc.
> 
> Yes, the unconditional memset is silly.  Especially when the
> function has a handy gfp_t and could be passed __GFP_ZERO.
> 
> The comment says "managed kzalloc/kfree for device drivers, no kmalloc,
> always use kzalloc".  There's no explanation for this - it looks like
> some ideological thing.

Try this patch instead:
https://lkml.org/lkml/2013/10/9/14

Yours has an unnecessary duplicate memset of the whole
block when __GFP_ZERO is passed when that's already
done by the kmalloc_track_caller allocator.

Also if __GFP_ZERO is not passed, you should still
zero the struct devres header.
Tejun Heo - Oct. 10, 2013, 11:18 p.m.
Hello,

On Thu, Oct 10, 2013 at 04:06:03PM -0700, Andrew Morton wrote:
> On Tue, 08 Oct 2013 21:59:27 -0700 Joe Perches <joe@perches.com> wrote:
> 
> > I was a bit surprised to find there isn't a devm_kmalloc.
> 
> Yes, the unconditional memset is silly.  Especially when the
> function has a handy gfp_t and could be passed __GFP_ZERO.
> 
> The comment says "managed kzalloc/kfree for device drivers, no kmalloc,
> always use kzalloc".  There's no explanation for this - it looks like
> some ideological thing.

Skipping resetting just is highly unlikely to make any meaningful
difference in the paths devres is supposed to be used.  These don't
and shouldn't get called from hot paths.  Wouldn't it be more
idelogical to have something just because it feels weird not to have
it even if it is not necessary?  I don't necessarily object to having
non-zeroing interface but it seems rather pointless.  Do we have an
actual cases where this makes meaningful differences?

> -	memset(dr, 0, tot_size);
> +	if (gfp & __GFP_ZERO)
> +		memset(dr, 0, tot_size);

I'd much prefer to have at least the devres_node part cleared
regardless of __GFP_ZERO and Joe already has better patches doing
this.

Thanks.
Joe Perches - Oct. 10, 2013, 11:25 p.m.
On Thu, 2013-10-10 at 19:18 -0400, Tejun Heo wrote:
> Do we have an
> actual cases where this makes meaningful differences?

There are already a few array allocations
where the array is completely reinitialized.

Does it matter?  Shrug.

It's more API compatible and more symmetric.

Direct conversions of kmalloc blocks don't
involve kmalloc->kzalloc translations, so
it'll be bug-for-bug compatible.
Andrew Morton - Oct. 10, 2013, 11:46 p.m.
On Thu, 10 Oct 2013 16:14:00 -0700 Joe Perches <joe@perches.com> wrote:

> On Thu, 2013-10-10 at 16:06 -0700, Andrew Morton wrote:
> > On Tue, 08 Oct 2013 21:59:27 -0700 Joe Perches <joe@perches.com> wrote:
> > 
> > > I was a bit surprised to find there isn't a devm_kmalloc.
> > 
> > Yes, the unconditional memset is silly.  Especially when the
> > function has a handy gfp_t and could be passed __GFP_ZERO.
> > 
> > The comment says "managed kzalloc/kfree for device drivers, no kmalloc,
> > always use kzalloc".  There's no explanation for this - it looks like
> > some ideological thing.
> 
> Try this patch instead:
> https://lkml.org/lkml/2013/10/9/14
> 

That looks rather better.

Apart from forcing a needless memset, the current code will defeat
kmemcheck used-uninitialized checking.

Patch

--- a/drivers/base/devres.c~a
+++ a/drivers/base/devres.c
@@ -91,7 +91,8 @@  static __always_inline struct devres * a
 	if (unlikely(!dr))
 		return NULL;
 
-	memset(dr, 0, tot_size);
+	if (gfp & __GFP_ZERO)
+		memset(dr, 0, tot_size);
 	INIT_LIST_HEAD(&dr->node.entry);
 	dr->node.release = release;
 	return dr;
@@ -770,7 +771,7 @@  static int devm_kzalloc_match(struct dev
  * RETURNS:
  * Pointer to allocated memory on success, NULL on failure.
  */
-void * devm_kzalloc(struct device *dev, size_t size, gfp_t gfp)
+static void *__devm_kzalloc(struct device *dev, size_t size, gfp_t gfp)
 {
 	struct devres *dr;
 
@@ -783,8 +784,19 @@  void * devm_kzalloc(struct device *dev,
 	devres_add(dev, dr->data);
 	return dr->data;
 }
+
+void *devm_kzalloc(struct device *dev, size_t size, gfp_t gfp)
+{
+	return __devm_kzalloc(dev, size, gfp | __GFP_ZERO);
+}
 EXPORT_SYMBOL_GPL(devm_kzalloc);
 
+void *devm_kmalloc(struct device *dev, size_t size, gfp_t gfp)
+{
+	return __devm_kzalloc(dev, size, gfp);
+}
+EXPORT_SYMBOL_GPL(devm_kmalloc);
+
 /**
  * devm_kfree - Resource-managed kfree
  * @dev: Device this memory belongs to
--- a/include/linux/device.h~a
+++ a/include/linux/device.h
@@ -602,8 +602,9 @@  extern void devres_close_group(struct de
 extern void devres_remove_group(struct device *dev, void *id);
 extern int devres_release_group(struct device *dev, void *id);
 
-/* managed kzalloc/kfree for device drivers, no kmalloc, always use kzalloc */
+/* managed kmalloc/kzalloc/kfree for device drivers */
 extern void *devm_kzalloc(struct device *dev, size_t size, gfp_t gfp);
+extern void *devm_kmalloc(struct device *dev, size_t size, gfp_t gfp);
 extern void devm_kfree(struct device *dev, void *p);
 
 void __iomem *devm_ioremap_resource(struct device *dev, struct resource *res);