lkml.org 
[lkml]   [2013]   [Apr]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v3 2/3] resource: Add release_mem_region_adjustable()
From
Date
On Wed, 2013-04-24 at 16:42 +0800, Ram Pai wrote:
> On Thu, Apr 11, 2013 at 10:30:02AM -0600, Toshi Kani wrote:
> > On Wed, 2013-04-10 at 15:24 -0700, Andrew Morton wrote:
> > > On Wed, 10 Apr 2013 15:08:29 -0700 (PDT) David Rientjes <rientjes@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.kani@hp.com>
>
> I am not confortable witht the assumption, that when a split takes
> place, the children are assumed to be in the lower entry. Probably a
> warning to that effect, would help quickly
> nail down the problem, if such a case does encounter ?

Yes, __adjust_resource() fails with -EBUSY when such condition happens.
Hence, release_mem_region_adjustable() returns with -EBUSY, and
__remove_pages() emits a warning message per patch 3/3. So, it can be
quickly nailed down as this restriction is documented in the comment as
well.

At this point, the children are only used for Kernel code/data/bss as
follows. Hot-removable memory ranges are located at higher ranges than
them. So, I decided to simplify the implementation for this initial
version. We can always enhance it when needed.

# cat /proc/iomem
:
00100000-defa57ff : System RAM
01000000-0162f8d1 : Kernel code
0162f8d2-01ce52bf : Kernel data
01df1000-01fa5fff : Kernel bss
:
100000000-31fffffff : System RAM


> Otherwise this looks fine. Sorry for the delayed reply. Was out.
>
> Reviewed-by: Ram Pai <linuxram@us.ibm.com>

No problem. Thanks for reviewing!
-Toshi




\
 
 \ /
  Last update: 2013-04-24 17:42    [W:0.057 / U:0.144 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site