On Wed, 2013-04-10 at 15:24 -0700, Andrew Morton wrote:
> On Wed, 10 Apr 2013 15:08:29 -0700 (PDT) David Rientjes <rient...@google.com>
> wrote:
>
> > On Wed, 10 Apr 2013, Toshi Kani wrote:
> >
> > > > I'll switch it to GFP_ATOMIC. Which is horridly lame but the
> > > > allocation is small and alternatives are unobvious.
> > >
> > > Great! Again, thanks for the update!
> >
> > release_mem_region_adjustable() allocates at most one struct resource, so
> > why not do kmalloc(sizeof(struct resource), GFP_KERNEL) before taking
> > resource_lock and then testing whether it's NULL or not when splitting?
> > It unnecessarily allocates memory when there's no split, but
> > __remove_pages() shouldn't be a hotpath.
>
> yup.
>
> --- a/kernel/resource.c~resource-add-release_mem_region_adjustable-fix-fix
> +++ a/kernel/resource.c
> @@ -1046,7 +1046,8 @@ int release_mem_region_adjustable(struct
> resource_size_t start, resource_size_t size)
> {
> struct resource **p;
> - struct resource *res, *new;
> + struct resource *res;
> + struct resource *new_res;
> resource_size_t end;
> int ret = -EINVAL;
>
> @@ -1054,6 +1055,9 @@ int release_mem_region_adjustable(struct
> if ((start < parent->start) || (end > parent->end))
> return ret;
>
> + /* The kzalloc() result gets checked later */
> + new_res = kzalloc(sizeof(struct resource), GFP_KERNEL);
> +
> p = &parent->child;
> write_lock(&resource_lock);
>
> @@ -1091,32 +1095,33 @@ int release_mem_region_adjustable(struct
> start - res->start);
> } else {
> /* split into two entries */
> - new = kzalloc(sizeof(struct resource), GFP_ATOMIC);
> - if (!new) {
> + if (!new_res) {
> ret = -ENOMEM;
> break;
> }
> - new->name = res->name;
> - new->start = end + 1;
> - new->end = res->end;
> - new->flags = res->flags;
> - new->parent = res->parent;
> - new->sibling = res->sibling;
> - new->child = NULL;
> + new_res->name = res->name;
> + new_res->start = end + 1;
> + new_res->end = res->end;
> + new_res->flags = res->flags;
> + new_res->parent = res->parent;
> + new_res->sibling = res->sibling;
> + new_res->child = NULL;
>
> ret = __adjust_resource(res, res->start,
> start - res->start);
> if (ret) {
> - kfree(new);
> + kfree(new_res);
> break;
> }
The kfree() in the if-statement above is not necessary since kfree() is
called before the return at the end. That is, the if-statement needs to
be:
if (ret)
break;
With this change, I confirmed that all my test cases passed (with all
the config debug options this time :). With the change:
Reviewed-by: Toshi Kani <toshi.k...@hp.com>
Tested-by: Toshi Kani <toshi.k...@hp.com>
Thanks!
-Toshi
> - res->sibling = new;
> + res->sibling = new_res;
> + new_res = NULL;
> }
>
> break;
> }
>
> write_unlock(&resource_lock);
> + kfree(new_res);
> return ret;
> }
> #endif /* CONFIG_MEMORY_HOTPLUG */
> _
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
