| Message ID | 20131010160603.1489fe054c2bdc0a0eb9460f@linux-foundation.org |
|---|---|
| State | Accepted |
| Headers | show |
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.
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.
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.
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.
--- 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);
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.