During ram initialization for migration dirty/clear bitmaps are
allocated for all migratable blocks, irrespective of their shared
status. However, during ram migration cleanup those bitmaps are freed
only for those blocks which aren't shared, in case x-ignore-shared
capability is used. This leads to a situation where the bitmaps aren't
freed for such blocks.

Fix this by switching the cleanup code to also free bitmaps for all
migratable blocks.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 migration/ram.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/migration/ram.c b/migration/ram.c
index dc1de9ddbc68..2e40166d2f9e 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -2678,7 +2678,7 @@ static void ram_save_cleanup(void *opaque)
         }
     }
 
-    RAMBLOCK_FOREACH_NOT_IGNORED(block) {
+    RAMBLOCK_FOREACH_MIGRATABLE(block) {
         g_free(block->clear_bmap);
         block->clear_bmap = NULL;
         g_free(block->bmap);
-- 
2.34.1

* Nikolay Borisov (nborisov@suse.com) wrote:
> During ram initialization for migration dirty/clear bitmaps are
> allocated for all migratable blocks, irrespective of their shared
> status. However, during ram migration cleanup those bitmaps are freed
> only for those blocks which aren't shared, in case x-ignore-shared
> capability is used. This leads to a situation where the bitmaps aren't
> freed for such blocks.

Can you show me where you're seeing the allocation based on MIGRATABLE?
I'm looking at ram_list_init_bitmaps:


        RAMBLOCK_FOREACH_NOT_IGNORED(block) {
            block->bmap = bitmap_new(pages);
....
            block->clear_bmap = bitmap_new(clear_bmap_size(pages, shift));


So that's based on NOT_IGNORED.

Dave

> Fix this by switching the cleanup code to also free bitmaps for all
> migratable blocks.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
>  migration/ram.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/migration/ram.c b/migration/ram.c
> index dc1de9ddbc68..2e40166d2f9e 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -2678,7 +2678,7 @@ static void ram_save_cleanup(void *opaque)
>          }
>      }
>  
> -    RAMBLOCK_FOREACH_NOT_IGNORED(block) {
> +    RAMBLOCK_FOREACH_MIGRATABLE(block) {
>          g_free(block->clear_bmap);
>          block->clear_bmap = NULL;
>          g_free(block->bmap);
> -- 
> 2.34.1
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

On 22.09.22 г. 20:42 ч., Dr. David Alan Gilbert wrote:
> * Nikolay Borisov (nborisov@suse.com) wrote:
>> During ram initialization for migration dirty/clear bitmaps are
>> allocated for all migratable blocks, irrespective of their shared
>> status. However, during ram migration cleanup those bitmaps are freed
>> only for those blocks which aren't shared, in case x-ignore-shared
>> capability is used. This leads to a situation where the bitmaps aren't
>> freed for such blocks.
> 
> Can you show me where you're seeing the allocation based on MIGRATABLE?
> I'm looking at ram_list_init_bitmaps:
> 
> 
>          RAMBLOCK_FOREACH_NOT_IGNORED(block) {
>              block->bmap = bitmap_new(pages);
> ....
>              block->clear_bmap = bitmap_new(clear_bmap_size(pages, shift));
> 
> 
> So that's based on NOT_IGNORED.

Huhz, you are perfectly right and I assume I got confused by the 
RAMBLOCK_FOREACH_MIGRATABLE in ram_save_setup as opposed to the code in 
ram_list_init_bitmaps. Apologies for the noise...

> 
> Dave
> 
>> Fix this by switching the cleanup code to also free bitmaps for all
>> migratable blocks.
>>
>> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
>> ---
>>   migration/ram.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/migration/ram.c b/migration/ram.c
>> index dc1de9ddbc68..2e40166d2f9e 100644
>> --- a/migration/ram.c
>> +++ b/migration/ram.c
>> @@ -2678,7 +2678,7 @@ static void ram_save_cleanup(void *opaque)
>>           }
>>       }
>>   
>> -    RAMBLOCK_FOREACH_NOT_IGNORED(block) {
>> +    RAMBLOCK_FOREACH_MIGRATABLE(block) {
>>           g_free(block->clear_bmap);
>>           block->clear_bmap = NULL;
>>           g_free(block->bmap);
>> -- 
>> 2.34.1
>>