Re: [PATCH v2] migration: clear the memory region dirty bitmap when skipping free pages

[David Hildenbrand]

> > > +    /*
> > > +     * CLEAR_BITMAP_SHIFT_MIN should always guarantee this... this
> > > +     * can make things easier sometimes since then start address
> > > +     * of the small chunk will always be 64 pages aligned so the
> > > +     * bitmap will always be aligned to unsigned long. We should
> > > +     * even be able to remove this restriction but I'm simply
> > > +     * keeping it.
> > > +     */
> > > +    assert(shift >= 6);
> > > +
> > > +    size = 1ULL << (TARGET_PAGE_BITS + shift);
> > > +    start = (((ram_addr_t)page) << TARGET_PAGE_BITS) & (-size);
> >
> > these as well as.
>
> Is there any coding style requirement for this?

Don't think so. It simply results in less LOC and less occurrences of 
variables.

> My thought was that those operations could mostly be avoided if they don't pass 
> the
> above if condition (e.g. just once per 1GB chunk).

Usually the compiler will reshuffle as possible to optimize. But in this 
case, due to clear_bmap_test_and_clear(), it might not be able to move 
the computations behind that call. So the final code might actually differ.

Not that we really care about this micro-optimization, though.

> > > +    trace_migration_bitmap_clear_dirty(rb->idstr, start, size, page);
> > > +    memory_region_clear_dirty_bitmap(rb->mr, start, size); }
> > > +
> > > +static void
> > > +migration_clear_memory_region_dirty_bitmap_range(RAMState *rs,
> > > +                                                 RAMBlock *rb,
> > > +                                                 unsigned long
> > start,
> > > +                                                 unsigned long
> > > +npages) {
> > > +    unsigned long page_to_clear, i, nchunks;
> > > +    unsigned long chunk_pages = 1UL << rb->clear_bmap_shift;
> > > +
> > > +    nchunks = (start + npages) / chunk_pages - start / chunk_pages +
> > > + 1;
> >
> > Wouldn't you have to align the start and the end range up/down to properly
> > calculate the number of chunks?
>
> No, divide will round it to the integer (beginning of the chunk to clear).


nchunks = (start + npages) / chunk_pages - start / chunk_pages + 1;

For simplicity:

nchunks = (addr + size) / chunk_size - addr / chunk_size + 1;

addr=1GB
size=3GB
chunk_size=2GB

So for that range [1GB, 3GB), we'd have to clear [0GB,2GB), [2GB,4GB)

Range:    [      ]
Chunks: [ - ][ - ][ - ][ - ] ...
        ^0   ^2   ^4   ^6

nchunks = (1 + 3) / 2 - 1 / 2 + 1
        = 4 / 2 - 0 + 1
        = 2 + 1
        = 3

Which is wrong.

While my variant will give you

aligned_start = 0GB
aligned_end = 4GB

And consequently clear [0GB,2GB) and [2GB,4GB).


Am I making a stupid mistake and should rather get another cup of coffee? :)


> > The following might be better and a little easier to grasp:
> >
> > unsigned long chunk_pages = 1ULL << rb->clear_bmap_shift; unsigned long
> > aligned_start = QEMU_ALIGN_DOWN(start, chunk_pages); unsigned long
> > aligned_end = QEMU_ALIGN_UP(start + npages, chunk_pages)
> >
> > /*
> >    * Clear the clar_bmap of all covered chunks. It's sufficient to call it for
> >    * one page within a chunk.
> >    */
> > for (start = aligned_start, start != aligned_end, start += chunk_pages) {
>
> What if "aligned_end == start + npages"?
> i.e the above start + npages is aligned by itself without QEMU_ALIGN_UP().
> For example, chunk size is 1GB, and start+npages=2GB, which is right at the 
> beginning of [2GB,3GB) chunk.
> Then aligned_end is also 2GB, but we need to clear the [2GB, 3GB) chunk, right?

Again, let's work with sizes instead of PFNs:

addr=1GB
size=1GB
chunk_size=1GB

Range:       [   ]
Chunks: [ - ][ - ][ - ][ - ] ...
        ^0   ^1   ^2   ^3

aligned_start = 1GB
aligned_end = 2GB

As you say, we'd clear the [1GB,2GB) chunk, but not the [2GB,3GB) chunk. 
But that's correct, as our range to hint is actually [start, 
start+npages) == [1GB,2GB).

> Best,
> Wei


--
Thanks,

David / dhildenb