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

[Stefan Hajnoczi]

On Sat, Dec 26, 2020 at 03:11:53PM +0800, Keqian Zhu wrote:
> 
> [...]
> 
> >>> -        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.

Growth is safe because other threads only access pre-existing ranges
(below the old maximum size). They will only start accessing the newly
added ranges after resize.

Did you find a code path where this constraint is violated?

Stefan

signature.asc
Description: PGP signature