[...]
>>> - 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 > > > . >