[BUG] Xen-ballooned memory never returned to domain after partial-free

[BUG] Xen-ballooned memory never returned to domain after partial-free

[Nicholas Tsirakis]

Hello,

The issue I'm seeing is that pages of previously-xenballooned memory are getting
trapped in the balloon on free, specifically when they are free'd in batches
(i.e. not all at once). The first batch is restored to the domain properly, but
subsequent frees are not.

Truthfully I'm not sure if this is a bug or not, but the behavior I'm seeing
doesn't seem to make sense. Note that this "bug" is in the balloon driver, but
the behavior is seen when using the gnttab API, which utilizes the balloon in
the background.

------------------------------------------------------------------------------

This issue is better illustrated as an example, seen below. Note that the file
in question is drivers/xen/balloon.c:

Kernel version: 4.19.*, code seems consistent on master as well
Relevant configs:
    - CONFIG_MEMORY_HOTPLUG not set
    - CONFIG_XEN_BALLOON_MEMORY_HOTPLUG not set

* current_pages = # of pages assigned to domain
* target_pages = # of pages we want assigned to domain
* credit = target - current

Start with current_pages/target_pages = 20 pages

1. alloc 5 pages with gnttab_alloc_pages(). current_pages = 15, credit = 5.
2. alloc 3 pages with gnttab_alloc_pages(). current_pages = 12, credit = 8.
3. some time later, free the last 3 pages with gnttab_free_pages().
4. 3 pages go back to balloon and balloon worker is scheduled since credit > 0.
    * Relevant part of balloon worker shown below:

    do {
        ...

        credit = current_credit();

        if (credit > 0) {
            if (balloon_is_inflated())
                state = increase_reservation(credit);
            else
                state = reserve_additional_memory();
        }

        ...

    } while (credit && state == BP_DONE);

5. credit > 0 and the balloon contains 3 pages, so run increase_reservation. 3
   pages are restored to domain, correctly. current_pages = 15, credit = 5.
6. at this point credit is still > 0, so we loop again.
7. this time, the balloon has 0 pages, so we call reserve_additional_memory,
   seen below. note that CONFIG_XEN_BALLOON_MEMORY_HOTPLUG is disabled, so this
   funciton is very sparse.

    static enum bp_state reserve_additional_memory(void)
    {
        balloon_stats.target_pages = balloon_stats.current_pages;
        return BP_ECANCELED;
    }

8. now target = current = 15, which drops our credit down to 0.
9. at some point later we attempt to free the remaining 5 pages with
   gnttab_free_pages().
10. 5 pages go back into the balloon, but this time credit = 0, so we never
    trigger our balloon worker (it wouldn't do anything anyway).
11. since we've essentially irreversibly decreased target_pages, we'll never
    attempt to re-add those pages to our domain, and those pages are reserved
    in the balloon forever.
12. this can be verified by running "free", "cat /proc/meminfo", etc. to show
    that the total memory has indeed decreased permanently until host reboot.

Is this desired behavior? Why would we decrease our target pages if there's no
way to restore them? I understand there is a helper function to manually reset
the target, but the caller would need to manually keep track of the starting
pages; that seems like unnecessary maintenance that the balloon should handle.

Additionally, why should any of the above code be possible if we have memory
hotplugging disabled? I'm surprised we are able to balloon any memory out from
the domain in the first place. I would have expected gnttab_alloc_pages to fail.

Please CC niko.tsirakis@gmail.com on any replies. Thank you,

--Niko

Re: [BUG] Xen-ballooned memory never returned to domain after partial-free

[Jürgen Groß]

[-- Attachment #1: Type: text/plain, Size: 2891 bytes --]

On 11.12.19 23:08, Nicholas Tsirakis wrote:
> Hello,
> 
> The issue I'm seeing is that pages of previously-xenballooned memory are getting
> trapped in the balloon on free, specifically when they are free'd in batches
> (i.e. not all at once). The first batch is restored to the domain properly, but
> subsequent frees are not.
> 
> Truthfully I'm not sure if this is a bug or not, but the behavior I'm seeing
> doesn't seem to make sense. Note that this "bug" is in the balloon driver, but
> the behavior is seen when using the gnttab API, which utilizes the balloon in
> the background.
> 
> ------------------------------------------------------------------------------
> 
> This issue is better illustrated as an example, seen below. Note that the file
> in question is drivers/xen/balloon.c:
> 
> Kernel version: 4.19.*, code seems consistent on master as well
> Relevant configs:
>      - CONFIG_MEMORY_HOTPLUG not set
>      - CONFIG_XEN_BALLOON_MEMORY_HOTPLUG not set
> 
> * current_pages = # of pages assigned to domain
> * target_pages = # of pages we want assigned to domain
> * credit = target - current
> 
> Start with current_pages/target_pages = 20 pages
> 
> 1. alloc 5 pages with gnttab_alloc_pages(). current_pages = 15, credit = 5.
> 2. alloc 3 pages with gnttab_alloc_pages(). current_pages = 12, credit = 8.
> 3. some time later, free the last 3 pages with gnttab_free_pages().
> 4. 3 pages go back to balloon and balloon worker is scheduled since credit > 0.
>      * Relevant part of balloon worker shown below:
> 
>      do {
>          ...
> 
>          credit = current_credit();
> 
>          if (credit > 0) {
>              if (balloon_is_inflated())
>                  state = increase_reservation(credit);
>              else
>                  state = reserve_additional_memory();
>          }
> 
>          ...
> 
>      } while (credit && state == BP_DONE);
> 
> 5. credit > 0 and the balloon contains 3 pages, so run increase_reservation. 3
>     pages are restored to domain, correctly. current_pages = 15, credit = 5.
> 6. at this point credit is still > 0, so we loop again.
> 7. this time, the balloon has 0 pages, so we call reserve_additional_memory,
>     seen below. note that CONFIG_XEN_BALLOON_MEMORY_HOTPLUG is disabled, so this
>     funciton is very sparse.
> 
>      static enum bp_state reserve_additional_memory(void)
>      {
>          balloon_stats.target_pages = balloon_stats.current_pages;
>          return BP_ECANCELED;
>      }
> 
> 8. now target = current = 15, which drops our credit down to 0.

And I think this is the problem. We want here:

     balloon_stats.target_pages = balloon_stats.current_pages +
                                  balloon_stats.target_unpopulated;

This should fix it. Thanks for the detailed analysis!

Does the attached patch work for you?

And are you fine with the "Reported-by:" added?


Juergen

[-- Attachment #2: 0001-xen-balloon-fix-ballooned-page-accounting-without-ho.patch --]
[-- Type: text/x-patch, Size: 1286 bytes --]

From 7cf6cf2b94ee11002dab439fb4ed5c7dcc1a971b Mon Sep 17 00:00:00 2001
From: Juergen Gross <jgross@suse.com>
Date: Thu, 12 Dec 2019 08:12:26 +0100
Subject: [PATCH] xen/balloon: fix ballooned page accounting without hotplug
 enabled

When CONFIG_XEN_BALLOON_MEMORY_HOTPLUG is not defined
reserve_additional_memory() will set balloon_stats.target_pages to a
wrong value in case there are still some ballooned pages allocated via
alloc_xenballooned_pages().

This will result in balloon_process() no longer be triggered when
ballooned pages are freed in batches.

Reported-by: Nicholas Tsirakis <niko.tsirakis@gmail.com>
Signed-off-by: Juergen Gross <jgross@suse.com>
---
 drivers/xen/balloon.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
index 4f2e78a5e4db..0c142bcab79d 100644
--- a/drivers/xen/balloon.c
+++ b/drivers/xen/balloon.c
@@ -394,7 +394,8 @@ static struct notifier_block xen_memory_nb = {
 #else
 static enum bp_state reserve_additional_memory(void)
 {
-	balloon_stats.target_pages = balloon_stats.current_pages;
+	balloon_stats.target_pages = balloon_stats.current_pages +
+				     balloon_stats.target_unpopulated;
 	return BP_ECANCELED;
 }
 #endif /* CONFIG_XEN_BALLOON_MEMORY_HOTPLUG */
-- 
2.16.4

Re: [BUG] Xen-ballooned memory never returned to domain after partial-free

[Nicholas Tsirakis]

> And I think this is the problem. We want here:
>
>     balloon_stats.target_pages = balloon_stats.current_pages +
>                                  balloon_stats.target_unpopulated;

Ahh I knew I was missing something. Tested the patch, works great! "Reported by"
is fine with me.

Do you happen to know the answer to my second question? It's not as important,
but it does confuse me as I wouldn't expect the total memory to be
balloon-able at
all with the hotplugging configs disabled.

--Niko

On Thu, Dec 12, 2019 at 2:18 AM Jürgen Groß <jgross@suse.com> wrote:
>
> On 11.12.19 23:08, Nicholas Tsirakis wrote:
> > Hello,
> >
> > The issue I'm seeing is that pages of previously-xenballooned memory are getting
> > trapped in the balloon on free, specifically when they are free'd in batches
> > (i.e. not all at once). The first batch is restored to the domain properly, but
> > subsequent frees are not.
> >
> > Truthfully I'm not sure if this is a bug or not, but the behavior I'm seeing
> > doesn't seem to make sense. Note that this "bug" is in the balloon driver, but
> > the behavior is seen when using the gnttab API, which utilizes the balloon in
> > the background.
> >
> > ------------------------------------------------------------------------------
> >
> > This issue is better illustrated as an example, seen below. Note that the file
> > in question is drivers/xen/balloon.c:
> >
> > Kernel version: 4.19.*, code seems consistent on master as well
> > Relevant configs:
> >      - CONFIG_MEMORY_HOTPLUG not set
> >      - CONFIG_XEN_BALLOON_MEMORY_HOTPLUG not set
> >
> > * current_pages = # of pages assigned to domain
> > * target_pages = # of pages we want assigned to domain
> > * credit = target - current
> >
> > Start with current_pages/target_pages = 20 pages
> >
> > 1. alloc 5 pages with gnttab_alloc_pages(). current_pages = 15, credit = 5.
> > 2. alloc 3 pages with gnttab_alloc_pages(). current_pages = 12, credit = 8.
> > 3. some time later, free the last 3 pages with gnttab_free_pages().
> > 4. 3 pages go back to balloon and balloon worker is scheduled since credit > 0.
> >      * Relevant part of balloon worker shown below:
> >
> >      do {
> >          ...
> >
> >          credit = current_credit();
> >
> >          if (credit > 0) {
> >              if (balloon_is_inflated())
> >                  state = increase_reservation(credit);
> >              else
> >                  state = reserve_additional_memory();
> >          }
> >
> >          ...
> >
> >      } while (credit && state == BP_DONE);
> >
> > 5. credit > 0 and the balloon contains 3 pages, so run increase_reservation. 3
> >     pages are restored to domain, correctly. current_pages = 15, credit = 5.
> > 6. at this point credit is still > 0, so we loop again.
> > 7. this time, the balloon has 0 pages, so we call reserve_additional_memory,
> >     seen below. note that CONFIG_XEN_BALLOON_MEMORY_HOTPLUG is disabled, so this
> >     funciton is very sparse.
> >
> >      static enum bp_state reserve_additional_memory(void)
> >      {
> >          balloon_stats.target_pages = balloon_stats.current_pages;
> >          return BP_ECANCELED;
> >      }
> >
> > 8. now target = current = 15, which drops our credit down to 0.
>
> And I think this is the problem. We want here:
>
>      balloon_stats.target_pages = balloon_stats.current_pages +
>                                   balloon_stats.target_unpopulated;
>
> This should fix it. Thanks for the detailed analysis!
>
> Does the attached patch work for you?
>
> And are you fine with the "Reported-by:" added?
>
>
> Juergen

Re: [BUG] Xen-ballooned memory never returned to domain after partial-free

[Jürgen Groß]

On 12.12.19 15:10, Nicholas Tsirakis wrote:
>> And I think this is the problem. We want here:
>>
>>      balloon_stats.target_pages = balloon_stats.current_pages +
>>                                   balloon_stats.target_unpopulated;
> 
> Ahh I knew I was missing something. Tested the patch, works great! "Reported by"
> is fine with me.

Thanks.

> 
> Do you happen to know the answer to my second question? It's not as important,
> but it does confuse me as I wouldn't expect the total memory to be
> balloon-able at
> all with the hotplugging configs disabled.

Ballooning != hotplugging memory

With memory hotplug you can add (or - in theory - remove) memory to the
kernel it didn't know about before.

With ballooning you just give some memory back to the hypervisor, but
kernel still has some knowledge about it (e.g. keeps struct page for
each ballooned memory page).

HTH, Juergen

Re: [BUG] Xen-ballooned memory never returned to domain after partial-free

[Nicholas Tsirakis]

>> Do you happen to know the answer to my second question? It's not as important,
>> but it does confuse me as I wouldn't expect the total memory to be
>> balloon-able at
>> all with the hotplugging configs disabled.

> Ballooning != hotplugging memory
>
> With memory hotplug you can add (or - in theory - remove) memory to the
> kernel it didn't know about before.
>
> With ballooning you just give some memory back to the hypervisor, but
> kernel still has some knowledge about it (e.g. keeps struct page for
> each ballooned memory page).

Got it, thanks for that clarification and for all your help!

--Niko

On Thu, Dec 12, 2019 at 9:21 AM Jürgen Groß <jgross@suse.com> wrote:
>
> On 12.12.19 15:10, Nicholas Tsirakis wrote:
> >> And I think this is the problem. We want here:
> >>
> >>      balloon_stats.target_pages = balloon_stats.current_pages +
> >>                                   balloon_stats.target_unpopulated;
> >
> > Ahh I knew I was missing something. Tested the patch, works great! "Reported by"
> > is fine with me.
>
> Thanks.
>
> >
> > Do you happen to know the answer to my second question? It's not as important,
> > but it does confuse me as I wouldn't expect the total memory to be
> > balloon-able at
> > all with the hotplugging configs disabled.
>
> Ballooning != hotplugging memory
>
> With memory hotplug you can add (or - in theory - remove) memory to the
> kernel it didn't know about before.
>
> With ballooning you just give some memory back to the hypervisor, but
> kernel still has some knowledge about it (e.g. keeps struct page for
> each ballooned memory page).
>
> HTH, Juergen