Re: [PATCH v2 1/2] ramlist: Make dirty bitmap blocks of ramlist resizable

[Keqian Zhu]

On 2020/12/17 18:05, Stefan Hajnoczi wrote:
> On Mon, Nov 30, 2020 at 09:11:03PM +0800, Keqian Zhu wrote:
>> @@ -1839,15 +1841,26 @@ static void dirty_memory_extend(ram_addr_t 
>> old_ram_size,
>>          new_blocks = g_malloc(sizeof(*new_blocks) +
>>                                sizeof(new_blocks->blocks[0]) * 
>> new_num_blocks);
>>  
>> -        if (old_num_blocks) {
>> +        if (cpy_num_blocks) {
>>              memcpy(new_blocks->blocks, old_blocks->blocks,
>> -                   old_num_blocks * sizeof(old_blocks->blocks[0]));
>> +                   cpy_num_blocks * sizeof(old_blocks->blocks[0]));
>>          }
>>  
>> -        for (j = old_num_blocks; j < new_num_blocks; j++) {
>> -            new_blocks->blocks[j] = bitmap_new(DIRTY_MEMORY_BLOCK_SIZE);
>> +        if (extend) {
>> +            for (j = cpy_num_blocks; j < new_num_blocks; j++) {
>> +                new_blocks->blocks[j] = bitmap_new(DIRTY_MEMORY_BLOCK_SIZE);
>> +            }
>> +        } else {
>> +            for (j = cpy_num_blocks; j < old_num_blocks; j++) {
>> +                /* We are safe to free it, for that it is out-of-use */
>> +                g_free(old_blocks->blocks[j]);
> 
> This looks unsafe because this code uses Read Copy Update (RCU):
> 
>   old_blocks = qatomic_rcu_read(&ram_list.dirty_memory[i]);
> 
> Other threads may still be accessing old_blocks so we cannot modify it
> immediately. Changes need to be deferred until the next RCU period.
> g_free_rcu() needs to be used to do this.
> 
Hi Stefan,

You are right. I was thinking about the VM life cycle before. We shrink the 
dirty_memory
when we are removing unused ramblock. However we can not rely on this.

I also notice that "Organization into blocks allows dirty memory to grow (but 
not shrink)
under RCU". Why "but not shrink"? Any thoughts?

[...]
 * Organization into blocks allows dirty memory to grow (but not shrink) under
 * RCU.  When adding new RAMBlocks requires the dirty memory to grow, a new
 * DirtyMemoryBlocks array is allocated with pointers to existing blocks kept
 * the same.  Other threads can safely access existing blocks while dirty
 * memory is being grown.  When no threads are using the old DirtyMemoryBlocks
 * anymore it is freed by RCU (but the underlying blocks stay because they are
 * pointed to from the new DirtyMemoryBlocks).
 */
#define DIRTY_MEMORY_BLOCK_SIZE ((ram_addr_t)256 * 1024 * 8)
typedef struct {
    struct rcu_head rcu;
    unsigned long *blocks[];
} DirtyMemoryBlocks;
[...]

Thanks,
Keqian