Re: [Xen-devel] [PATCHv1 6/8] xen/balloon: only hotplug additional memory if required

[Daniel Kiper]

On Fri, Jun 26, 2015 at 09:56:31AM +0100, David Vrabel wrote:
> On 25/06/15 22:18, Daniel Kiper wrote:
> > On Thu, Jun 25, 2015 at 06:11:01PM +0100, David Vrabel wrote:
> >> Now that we track the total number of pages (included hotplugged
> >> regions), it is easy to determine if more memory needs to be
> >> hotplugged.
> >>
> >> Signed-off-by: David Vrabel <david.vrabel@xxxxxxxxxx>
> >> ---
> >>  drivers/xen/balloon.c |   16 +++++++++++++---
> >>  1 file changed, 13 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
> >> index 960ac79..dd41da8 100644
> >> --- a/drivers/xen/balloon.c
> >> +++ b/drivers/xen/balloon.c
> >> @@ -241,12 +241,22 @@ static void release_memory_resource(struct resource *resource)
> >>   * bit set). Real size of added memory is established at page onlining stage.
> >>   */
> >>
> >> -static enum bp_state reserve_additional_memory(long credit)
> >> +static enum bp_state reserve_additional_memory(void)
> >>  {
> >> +	long credit;
> >>  	struct resource *resource;
> >>  	int nid, rc;
> >>  	unsigned long balloon_hotplug;
> >>
> >> +	credit = balloon_stats.target_pages - balloon_stats.total_pages;
> >> +
> >> +	/*
> >> +	 * Already hotplugged enough pages?  Wait for them to be
> >> +	 * onlined.
> >> +	 */
> >
> > Comment is wrong or at least misleading. Both values does not depend on onlining.
>
> If we get here and credit <=0 then the balloon is empty and we have

Right.

> already hotplugged enough sections to reach target.  We need to wait for

OK.

> userspace to online the sections that already exist.

This is not true. You do not need to online sections to reserve new
memory region. Onlining does not change balloon_stats.target_pages
nor balloon_stats.total_pages. You must increase balloon_stats.target_pages
above balloon_stats.total_pages to reserve new memory region. And
balloon_stats.target_pages increase is not related to onlining.

> >> +	if (credit <= 0)
> >> +		return BP_EAGAIN;
> >
> > Not BP_EAGAIN for sure. It should be BP_DONE but then balloon_process() will go
> > into loop until memory is onlined at least up to balloon_stats.target_pages.
> > BP_ECANCELED does work but it is misleading because it is not an error. So, maybe
> > we should introduce BP_STOP (or something like that) which works like BP_ECANCELED
> > and is not BP_ECANCELED.
>
> We don't want to spin while waiting for userspace to online a new

Right.

> section so BP_EAGAIN is correct here as it causes the balloon process to
> be rescheduled at a later time.

And this is wrong. We do not want that balloon process wakes up and
looks for onlined pages. Onlinig may happen long time after memory
reservation. So, it means that until all needed sections are not onlined
then balloon process will be woken up for nothing (I assume that nobody
changes balloon_stats.target_pages). xen_memory_notifier() does work
for us. It wakes up balloon process after onlinig and then it can
do relevant work.

Daniel
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/