WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

Re: [Xen-devel] [PATCH 3/3] drivers/xen/balloon.c: Xen memory balloon dr

Hi,

On Mon, Dec 27, 2010 at 10:25:49AM -0500, Konrad Rzeszutek Wilk wrote:
> On Mon, Dec 20, 2010 at 02:48:03PM +0100, Daniel Kiper wrote:
> > +static inline void adjust_memory_resource(struct resource **r, unsigned 
> > long nr_pages)
> > +{
> > +   if ((*r)->end + 1 - (nr_pages << PAGE_SHIFT) == (*r)->start) {
> > +           BUG_ON(release_resource(*r) < 0);
> > +           kfree(*r);
> > +           *r = NULL;
> > +           return;
> > +   }
> > +
> > +   BUG_ON(adjust_resource(*r, (*r)->start, (*r)->end + 1 - (*r)->start -
> > +                           (nr_pages << PAGE_SHIFT)) < 0);
>
> Why not return a value here instead of BUG-ing out?

If release_resource()/adjust_resource() fail it means
there is no possibility to align resource descpription with
real memory address space. I think it is fatal error. If
we decide to leave system in that state it could lead
to data corruption or crash.

> > +   if (xen_pv_domain())
> > +           for (pfn = PFN_DOWN((*r)->start); pfn < PFN_UP((*r)->end); 
> > ++pfn)
> > +                   if (!PageHighMem(pfn_to_page(pfn)))
> > +                           BUG_ON(HYPERVISOR_update_va_mapping(
> > +                                   (unsigned long)__va(pfn << PAGE_SHIFT),
> > +                                   mfn_pte(pfn_to_mfn(pfn), PAGE_KERNEL), 
> > 0));
>
> Could we just stop here instead of bugging out? I mean we adding memory so
> if it does not work, the failure path seems to not add the memory?

Very good question. I based that fragment of code on original
increase_reservation()/decrease_reservation(). I attempted to find
any good explanation for that however, without success. That is why
I decided to not touch that fragment of code. If I had any
good argument to change that I will do that.

> > +static inline int request_additional_memory(long credit)
> > +{
> > +   int rc;
> > +   static struct resource *r;
>
> static?

Yes, request_additional_memory() allocate memory in chunks
and r is used between subsequent calls until all memory
is allocated.

> > +   static unsigned long pages_left;
> > +
> > +   if ((credit <= 0 || balloon_stats.balloon_low ||
> > +                           balloon_stats.balloon_high) && !r)
> > +           return 0;
> >
> > -   target = min(target,
> > -                balloon_stats.current_pages +
> > -                balloon_stats.balloon_low +
> > -                balloon_stats.balloon_high);
> > +   if (!r) {
> > +           rc = allocate_memory_resource(&r, credit);
> >
> > -   return target;
> > +           if (rc)
> > +                   return rc;
> > +
> > +           pages_left = credit;
> > +   }
> > +
> > +   rc = allocate_additional_memory(r, pages_left);
> > +
> > +   if (rc < 0) {
> > +           if (balloon_stats.mh_policy == MH_POLICY_TRY_UNTIL_SUCCESS)
> > +                   return rc;
>
> I think you are going to hit a memory leak. If we fail, you do not
> kfree(*r), should you be doing that?

If balloon_stats.mh_policy == MH_POLICY_TRY_UNTIL_SUCCESS then
balloon process attempt to allocate requested memory until success.
*r is freed only when any error appears and no memory is allocated.

> > +
> > +           adjust_memory_resource(&r, pages_left);
>
> Would it make sense to check here as well?

Please look above.

> > +
> > +           if (!r)
> > +                   return rc;
> > +   } else {
> > +           pages_left -= rc;
> > +
> > +           if (pages_left)
> > +                   return 1;
>
> So wouldn't that mean we could still online the 'rc' amount of pages?

Memory is onlined only when all of it is allocated.

> > +   }
> > +
> > +   hotplug_allocated_memory(&r);
> > +
> > +   return 0;
> >  }

Daniel

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel

<Prev in Thread] Current Thread [Next in Thread>