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

[Keqian Zhu]

[...]

>>> -        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?
Hi,

After my analysis, it's both unsafe to grow or shrink under RCU.

ram_list.blocks and ram_list.dirty_memory[X] are closely related and
both protected by RCU. For the lockless RCU readers, we can't promise they
always see consistent version of the two structures.

For grow, a reader may see un-growed @dirty_memory and growed @blocks, causing 
out-of-bound access.
For shrink, a reader may see shrinked @dirty_memory and un-shrinked @blocks, 
causing out-of-bound access too.

I think it's a design problem, RCU can just protect one structure, not two.

Thanks,
Keqian.
> 
> [...]
>  * 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
> 
> 
> .
>