On 27.08.24 19:50, Peter Xu wrote:
On Tue, Aug 27, 2024 at 01:28:02PM -0400, Stefan Hajnoczi wrote:
On Tue, 27 Aug 2024 at 13:24, David Hildenbrand <da...@redhat.com> wrote:
On 27.08.24 18:52, Stefan Hajnoczi wrote:
On Tue, 27 Aug 2024 at 04:38, David Hildenbrand <da...@redhat.com> wrote:
As reported by Peter, we might be leaking memory when removing the
highest RAMBlock (in the weird ram_addr_t space), and adding a new one.
We will fail to realize that we already allocated bitmaps for more
dirty memory blocks, and effectively discard the pointers to them.
Fix it by getting rid of last_ram_page() and simply storing the number
of dirty memory blocks that have been allocated. We'll store the number
of blocks along with the actual pointer to keep it simple.
Looks like this leak was introduced as we switched from using a single
bitmap_zero_extend() to allocating multiple bitmaps:
bitmap_zero_extend() relies on g_renew() which should have taken care of
this.
Resolves:
https://lkml.kernel.org/r/CAFEAcA-k7a+VObGAfCFNygQNfCKL=AfX6A4kScq=vssk0pe...@mail.gmail.com
Reported-by: Peter Maydell <peter.mayd...@linaro.org>
Fixes: 5b82b703b69a ("memory: RCU ram_list.dirty_memory[] for safe RAM hotplug")
Cc: qemu-sta...@nongnu.org
Cc: Stefan Hajnoczi <stefa...@redhat.com>
Cc: Paolo Bonzini <pbonz...@redhat.com>
Cc: Peter Xu <pet...@redhat.com>
Cc: "Philippe Mathieu-Daudé" <phi...@linaro.org>
Signed-off-by: David Hildenbrand <da...@redhat.com>
---
include/exec/ramlist.h | 1 +
system/physmem.c | 44 ++++++++++++++----------------------------
2 files changed, 16 insertions(+), 29 deletions(-)
diff --git a/include/exec/ramlist.h b/include/exec/ramlist.h
index 2ad2a81acc..f2a965f293 100644
--- a/include/exec/ramlist.h
+++ b/include/exec/ramlist.h
@@ -41,6 +41,7 @@ typedef struct RAMBlockNotifier RAMBlockNotifier;
#define DIRTY_MEMORY_BLOCK_SIZE ((ram_addr_t)256 * 1024 * 8)
typedef struct {
struct rcu_head rcu;
+ unsigned int num_blocks;
The maximum amount of memory supported by unsigned int is:
(2 ^ 32 - 1) * 4KB * DIRTY_MEMORY_BLOCK_SIZE
= ~32 exabytes
True, should we simply use ram_addr_t ?
Sounds good to me. In practice scalability bottlenecks are likely with
those memory sizes and it will be necessary to change how guest memory
is organized anyway. But it doesn't hurt to make this counter
future-proof.
IMHO it'll be nice to only use ram_addr_t when a variable is describing the
ramblock address space (with an offset, or a length there). In this case
it is a pure counter for how many bitmap chunks we allocated, so maybe
"unsigned long" or "uint64_t" would suite more?
Though I'd think "unsigned int" is good enough per the calculation Stefan
provided.
Likely best, "ram_addr_t" requires including "exec/cpu-common.h".
So let's stick to "unsigned int" for now. Likely best to also include for
consistency:
diff --git a/system/physmem.c b/system/physmem.c
index fa48ff8333..e1391492fd 100644
--- a/system/physmem.c
+++ b/system/physmem.c
@@ -1789,14 +1789,14 @@ void qemu_ram_msync(RAMBlock *block, ram_addr_t start,
ram_addr_t length)
/* Called with ram_list.mutex held */
static void dirty_memory_extend(ram_addr_t new_ram_size)
{
- ram_addr_t new_num_blocks = DIV_ROUND_UP(new_ram_size,
- DIRTY_MEMORY_BLOCK_SIZE);
+ unsigned int new_num_blocks = DIV_ROUND_UP(new_ram_size,
+ DIRTY_MEMORY_BLOCK_SIZE);
int i;
for (i = 0; i < DIRTY_MEMORY_NUM; i++) {
DirtyMemoryBlocks *old_blocks;
DirtyMemoryBlocks *new_blocks;
- ram_addr_t old_num_blocks = 0;
+ unsigned int old_num_blocks = 0;
int j;
old_blocks = qatomic_rcu_read(&ram_list.dirty_memory[i]);
Reviewed-by: Peter Xu <pet...@redhat.com>
Thanks!
--
Cheers,
David / dhildenb
