migration/postcopy: Disable shared RAM
v2 cover migration/postcopy: Disable shared RAM
From: "Dr. David Alan Gilbert" <[email protected]>
Disable postcopy migration when there's shared RAM, we're still figuring out the details. Without this patch there's a chance it might appear to succeed in some cases.
v2
Make sure user-only build works (Halil)
Dr. David Alan Gilbert (2):
RAMBlocks: qemu_ram_is_shared
postcopy: Check for shared memory
exec.c | 5 +++++ include/exec/cpu-common.h | 1 + migration/postcopy-ram.c | 18 ++++++++++++++++++ 3 files changed, 24 insertions(+)
Signature · 1 line
2.9.3
PATCH v2 1/2 RAMBlocks: qemu_ram_is_shared
From: "Dr. David Alan Gilbert" <[email protected]>
Provide a helper to say whether a RAMBlock was created as a shared mapping.
--- exec.c | 5 +++++ include/exec/cpu-common.h | 1 + 2 files changed, 6 insertions(+)
exec.c +5 -0
@@ -2593,6 +2593,11 @@ MemoryRegion *get_system_io(void) return system_io; } +bool qemu_ram_is_shared (RAMBlock *rb) +{ +return rb->flags &RAM_SHARED ; +} +#endif /* !defined(CONFIG_USER_ONLY) */ /* physical memory access (slow version, mainly for debug) */
include/exec/cpu-common.h +1 -0
@@ -69,6 +69,7 @@ RAMBlock *qemu_ram_block_from_host(void *ptr, bool round_offset,void qemu_ram_set_idstr (RAMBlock *block,const char *name,DeviceState *dev);void qemu_ram_unset_idstr (RAMBlock *block);const char *qemu_ram_get_idstr (RAMBlock *rb); +bool qemu_ram_is_shared (RAMBlock *rb);size_t qemu_ram_pagesize (RAMBlock *block);size_t qemu_ram_pagesize_largest (void );
Signature · 1 line
2.9.3
Re: [Qemu-devel] [PATCH v2 1/2] RAMBlocks: qemu_ram_is_shared
Quoted 6 lines · Dr. David Alan Gilbert (git)
From: "Dr. David Alan Gilbert" <[email protected]>
Provide a helper to say whether a RAMBlock was created as a shared mapping.
Signed-off-by Dr. David Alan Gilbert <[email protected]>
Quoted 34 lines
--- exec.c | 5 +++++ include/exec/cpu-common.h | 1 + 2 files changed, 6 insertions(+)exec.c +5
-0@@ -2593,6 +2593,11 @@ MemoryRegion *get_system_io(void) return system_io; } +bool qemu_ram_is_shared (RAMBlock *rb) +{ +return rb->flags &RAM_SHARED ; +} +#endif /* !defined(CONFIG_USER_ONLY) */ /* physical memory access (slow version, mainly for debug) */ include/exec/cpu-common.h +1
-0@@ -69,6 +69,7 @@ RAMBlock *qemu_ram_block_from_host(void *ptr, bool round_offset,void qemu_ram_set_idstr (RAMBlock *block,const char *name,DeviceState *dev);void qemu_ram_unset_idstr (RAMBlock *block);const char *qemu_ram_get_idstr (RAMBlock *rb); +bool qemu_ram_is_shared (RAMBlock *rb);size_t qemu_ram_pagesize (RAMBlock *block);size_t qemu_ram_pagesize_largest (void );
Re: [Qemu-devel] [PATCH v2 1/2] RAMBlocks: qemu_ram_is_shared
In reply to Dr. David Alan Gilbert (git)Quoted 6 lines · Dr. David Alan Gilbert (git)
From: "Dr. David Alan Gilbert" <[email protected]>
Provide a helper to say whether a RAMBlock was created as a shared mapping.
Signed-off-by Dr. David Alan Gilbert <[email protected]>
Quoted 33 lines
--- exec.c | 5 +++++ include/exec/cpu-common.h | 1 + 2 files changed, 6 insertions(+)exec.c +5
-0@@ -2593,6 +2593,11 @@ MemoryRegion *get_system_io(void) return system_io; } +bool qemu_ram_is_shared (RAMBlock *rb) +{ +return rb->flags &RAM_SHARED ; +} +#endif /* !defined(CONFIG_USER_ONLY) */ /* physical memory access (slow version, mainly for debug) */ include/exec/cpu-common.h +1
-0@@ -69,6 +69,7 @@ RAMBlock *qemu_ram_block_from_host(void *ptr, bool round_offset,void qemu_ram_set_idstr (RAMBlock *block,const char *name,DeviceState *dev);void qemu_ram_unset_idstr (RAMBlock *block);const char *qemu_ram_get_idstr (RAMBlock *rb); +bool qemu_ram_is_shared (RAMBlock *rb);size_t qemu_ram_pagesize (RAMBlock *block);size_t qemu_ram_pagesize_largest (void );
Re: [Qemu-devel] [PATCH v2 1/2] RAMBlocks: qemu_ram_is_shared
In reply to Dr. David Alan Gilbert (git)Quoted 6 lines · Dr. David Alan Gilbert (git)
From: "Dr. David Alan Gilbert" <[email protected]>
Provide a helper to say whether a RAMBlock was created as a shared mapping.
Signed-off-by Dr. David Alan Gilbert <[email protected]>
PATCH v2 2/2 postcopy: Check for shared memory
In reply to Dr. David Alan Gilbert (git)From: "Dr. David Alan Gilbert" <[email protected]>
Postcopy doesn't support migration of RAM shared with another process yet (we've got a bunch of things to understand).
Check for the case and don't allow postcopy to be enabled.
--- migration/postcopy-ram.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)
migration/postcopy-ram.c +18 -0
@@ -95,6 +95,19 @@ static bool ufd_version_check(int ufd)return true; } +/* Callback from postcopy_ram_supported_by_host block iterator. +*/ +static int test_range_shared (const char *block_name,void *host_addr, +ram_addr_t offset,ram_addr_t length,void *opaque) +{ +if (qemu_ram_is_shared (qemu_ram_block_by_name (block_name))) { +error_report ("Postcopy on shared RAM (%s) is not yet supported" , + block_name); +return 1 ; + } +return 0 ; +} +/* * Note: This has the side effect of munlock'ing all of RAM, that's * normally fine since if the postcopy succeeds it gets turned back on at the
@@ -127,6 +140,11 @@ bool postcopy_ram_supported_by_host(void) goto out; } +/* We don't support postcopy with shared RAM yet */ +if (qemu_ram_foreach_block (test_range_shared,NULL )) { + goto out; + } +/* * userfault and mlock don't go together; we'll put it back later if * it was enabled.
Signature · 1 line
2.9.3
Re: [Qemu-devel] [PATCH v2 2/2] postcopy: Check for shared memory
Quoted 22 lines · Dr. David Alan Gilbert (git)
From: "Dr. David Alan Gilbert" <[email protected]>
Postcopy doesn't support migration of RAM shared with another process yet (we've got a bunch of things to understand).
Check for the case and don't allow postcopy to be enabled.Signed-off-by Dr. David Alan Gilbert <[email protected]>--- migration/postcopy-ram.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)migration/postcopy-ram.c +3
-0@@ -95,6 +95,19 @@ static bool ufd_version_check(int ufd)return true; } +/* Callback from postcopy_ram_supported_by_host block iterator. +*/ +static int test_range_shared(const char *block_name, void *host_addr,
migration/postcopy-ram.c static bool ufd_version_check(int ufd) · line 110
Quoted 10 lines
migration/postcopy-ram.c +10
-0+ ram_addr_t offset, ram_addr_t length, void *opaque) +{ + if (qemu_ram_is_shared(qemu_ram_block_by_name(block_name))) { + error_report("Postcopy on shared RAM (%s) is not yet supported", + block_name); +return 1 ; + } +return 0 ; +} +
Hm, this stuff with the iterator seemed a bit strange (too complicated) first, but I'm not familiar with this code. I have no idea why is RAMBlockIterFunc
typedef int (RAMBlockIterFunc)(const char *block_name, void *host_addr,
ram_addr_t offset, ram_addr_t length, void *opaque)
and not
typedef int (RAMBlockIterFunc)(RAMBlock *block, void *opaque).
The reason does not seem to be abstraction.
migration/postcopy-ram.c bool postcopy_ram_supported_by_host(void) · line 147
Quoted 12 lines
migration/postcopy-ram.c +5
-0/* * Note: This has the side effect of munlock'ing all of RAM, that's * normally fine since if the postcopy succeeds it gets turned back on at the @@ -127,6 +140,11 @@ bool postcopy_ram_supported_by_host(void) goto out; } +/* We don't support postcopy with shared RAM yet */ +if (qemu_ram_foreach_block (test_range_shared,NULL )) { + goto out; + } +
But using ram_list directly does not seem to be a good alternative to me, and I do not see a third alternative.
So besides some cosmetic stuff I have nothing to add. Cosmetic stuff is:
* why range instead of block in test_range_shared
* I think we could move this up so that we can return directly and do not acquire resources which need cleanup
Regardless of the cosmetics:
Quoted 4 lines
migration/postcopy-ram.c +0
-0/* * userfault and mlock don't go together; we'll put it back later if * it was enabled.
Re: [Qemu-devel] [PATCH v2 2/2] postcopy: Check for shared memory
Quoted 40 lines · * Halil Pasic ([email protected]) wrote:
Quoted 22 lines · Dr. David Alan Gilbert (git)
From: "Dr. David Alan Gilbert" <[email protected]>
Postcopy doesn't support migration of RAM shared with another process yet (we've got a bunch of things to understand).
Check for the case and don't allow postcopy to be enabled.Signed-off-by Dr. David Alan Gilbert <[email protected]>--- migration/postcopy-ram.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)migration/postcopy-ram.c +3
-0@@ -95,6 +95,19 @@ static bool ufd_version_check(int ufd)return true; } +/* Callback from postcopy_ram_supported_by_host block iterator. +*/ +static int test_range_shared(const char *block_name, void *host_addr,migration/postcopy-ram.c static bool ufd_version_check(int ufd) · line 110
Quoted 10 lines
migration/postcopy-ram.c +10
-0+ ram_addr_t offset, ram_addr_t length, void *opaque) +{ + if (qemu_ram_is_shared(qemu_ram_block_by_name(block_name))) { + error_report("Postcopy on shared RAM (%s) is not yet supported", + block_name); +return 1 ; + } +return 0 ; +} +Hm, this stuff with the iterator seemed a bit strange (too complicated) first, but I'm not familiar with this code. I have no idea why is RAMBlockIterFunc
Quoted 8 lines
typedef int (RAMBlockIterFunc)(const char
*block_name, void*host_addr,
ram_addr_toffset,ram_addr_tlength, void *opaque)and not
typedef int (RAMBlockIterFunc)(RAMBlock *block, void *opaque).
The reason does not seem to be abstraction.
It is, it's because the contents of RAMBlock are private.
Quoted 16 lines
migration/postcopy-ram.c bool postcopy_ram_supported_by_host(void) · line 147
Quoted 12 lines
migration/postcopy-ram.c +5
-0/* * Note: This has the side effect of munlock'ing all of RAM, that's * normally fine since if the postcopy succeeds it gets turned back on at the @@ -127,6 +140,11 @@ bool postcopy_ram_supported_by_host(void) goto out; } +/* We don't support postcopy with shared RAM yet */ +if (qemu_ram_foreach_block (test_range_shared,NULL )) { + goto out; + } +But using
ram_listdirectly does not seem to be a good alternative to me, and I do not see a third alternative.
Quoted 7 lines
So besides some cosmetic stuff I have nothing to add. Cosmetic stuff is:
* why range instead of block intest_range_shared
* I think we could move this up so that we can return directly and do not acquire resources which need cleanupRegardless of the cosmetics:
Reviewed-by Halil Pasic <[email protected]>
Dave
Quoted 6 lines
migration/postcopy-ram.c bool postcopy_ram_supported_by_host(void) · line 150
Quoted 5 lines
migration/postcopy-ram.c +0
-0/* * userfault and mlock don't go together; we'll put it back later if * it was enabled.
--
Dr. David Alan Gilbert / [email protected] / Manchester, UK
Re: [Qemu-devel] [PATCH v2 2/2] postcopy: Check for shared memory
Quoted 44 lines · Dr. David Alan Gilbert
Quoted 43 lines · * Halil Pasic ([email protected]) wrote:
Quoted 22 lines · Dr. David Alan Gilbert (git)
From: "Dr. David Alan Gilbert" <[email protected]>
Postcopy doesn't support migration of RAM shared with another process yet (we've got a bunch of things to understand).
Check for the case and don't allow postcopy to be enabled.Signed-off-by Dr. David Alan Gilbert <[email protected]>--- migration/postcopy-ram.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)migration/postcopy-ram.c +3
-0@@ -95,6 +95,19 @@ static bool ufd_version_check(int ufd)return true; } +/* Callback from postcopy_ram_supported_by_host block iterator. +*/ +static int test_range_shared(const char *block_name, void *host_addr,migration/postcopy-ram.c static bool ufd_version_check(int ufd) · line 110
Quoted 10 lines
migration/postcopy-ram.c +10
-0+ ram_addr_t offset, ram_addr_t length, void *opaque) +{ + if (qemu_ram_is_shared(qemu_ram_block_by_name(block_name))) { + error_report("Postcopy on shared RAM (%s) is not yet supported", + block_name); +return 1 ; + } +return 0 ; +} +Hm, this stuff with the iterator seemed a bit strange (too complicated) first, but I'm not familiar with this code. I have no idea why is RAMBlockIterFunc
typedef int (RAMBlockIterFunc)(const char
*block_name, void*host_addr,
ram_addr_toffset,ram_addr_tlength, void *opaque)
Quoted 8 lines
Quoted 5 lines
and not
typedef int (RAMBlockIterFunc)(RAMBlock *block, void *opaque).
The reason does not seem to be abstraction.
It is, it's because the contents of RAMBlock are private.
That's kind of half-backed abstraction. One can get a pointer to RAMBlock, just like you did, and then do operations on it, just like you did. So it would have been perfectly normal to use a pointer to RAMBlock (incomplete type) and provide accessors (getters) reflecting the public interface of RAMBlock. Well it may be helpful for making undesired usage patterns hard, I do not know if this is the reason. (What I mean is if I wanted to see these just for a single block I have a pointer to, I would probably have to iterate over all blocks get a pointer by name and compare it with my pointer, if it matches I have the values. So if such usage is undesired, it's well reflected in the design).
The whole question is kind of off-topic -- my curious nature is making me less productive again...
Regards,
Halil
Re: [Qemu-devel] [PATCH v2 2/2] postcopy: Check for shared memory
In reply to Dr. David Alan Gilbert (git)Quoted 16 lines · Dr. David Alan Gilbert (git)
From: "Dr. David Alan Gilbert" <[email protected]>
Postcopy doesn't support migration of RAM shared with another process yet (we've got a bunch of things to understand).
Check for the case and don't allow postcopy to be enabled.Signed-off-by Dr. David Alan Gilbert <[email protected]>--- migration/postcopy-ram.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)migration/postcopy-ram.c +0
-0
migration/postcopy-ram.c static bool ufd_version_check(int ufd) · line 101
Quoted 7 lines
migration/postcopy-ram.c +4
-0@@ -95,6 +95,19 @@ static bool ufd_version_check(int ufd)return true; } +/* Callback from postcopy_ram_supported_by_host block iterator. +*/ +static int test_range_shared (const char *block_name,void *host_addr, +ram_addr_t offset,ram_addr_t length,void *opaque)
Direct question for the name?
ram_range_is_shared?
migration/postcopy-ram.c static bool ufd_version_check(int ufd) · line 109
Quoted 8 lines
migration/postcopy-ram.c +8
-0+{ +if (qemu_ram_is_shared (qemu_ram_block_by_name (block_name))) { +error_report ("Postcopy on shared RAM (%s) is not yet supported" , + block_name); +return 1 ; + } +return 0 ; +}
Wow, we still predate use of bool for questions :-p
Quoted 6 lines
migration/postcopy-ram.c +1
-0+/* * Note: This has the side effect of munlock'ing all of RAM, that's * normally fine since if the postcopy succeeds it gets turned back on at the @@ -127,6 +140,11 @@ bool postcopy_ram_supported_by_host(void) goto out;
migration/postcopy-ram.c bool postcopy_ram_supported_by_host(void) · line 150
Quoted 10 lines
migration/postcopy-ram.c +5
-0} +/* We don't support postcopy with shared RAM yet */ +if (qemu_ram_foreach_block (test_range_shared,NULL )) { + goto out; + } +/* * userfault and mlock don't go together; we'll put it back later if * it was enabled.
As my comments are only aestetic,