Re: [Qemu-devel] [PATCH v7 36/42] Host page!=target page: Cleanup bitmaps

[Dr. David Alan Gilbert]

* Juan Quintela (address@hidden) wrote:
> "Dr. David Alan Gilbert (git)" <address@hidden> wrote:
> > From: "Dr. David Alan Gilbert" <address@hidden>
> >
> > Prior to the start of postcopy, ensure that everything that will
> > be transferred later is a whole host-page in size.
> >
> > This is accomplished by discarding partially transferred host pages
> > and marking any that are partially dirty as fully dirty.
> >
> > Signed-off-by: Dr. David Alan Gilbert <address@hidden>
> 
> >  /*
> > + * Helper for postcopy_chunk_hostpages where HPS/TPS >= bits-in-long
> > + *
> > + * !! Untested !!
> 
> You continue in the race for best comment ever O:-)

I prefer honesty in comments, especially for the next person who tries
to use it!

> > + */
> > +static int hostpage_big_chunk_helper(const char *block_name, void 
> > *host_addr,
> > +                                     ram_addr_t offset, ram_addr_t length,
> > +                                     void *opaque)
> > +{
> > +    MigrationState *ms = opaque;
> > +    unsigned long long_bits = sizeof(long) * 8;
> > +    unsigned int host_len = (qemu_host_page_size / TARGET_PAGE_SIZE) /
> > +                            long_bits;
> > +    unsigned long first_long, last_long, cur_long, current_hp;
> > +    unsigned long first = offset >> TARGET_PAGE_BITS;
> > +    unsigned long last = (offset + (length - 1)) >> TARGET_PAGE_BITS;
> > +
> > +    PostcopyDiscardState *pds = postcopy_discard_send_init(ms,
> > +                                                           first,
> > +                                                           block_name);
> 
> Minor
> 
> PostcopyDiscardState *pds =
>                      postcopy_discard_send_init(ms, first, block_name);
> 
> ??

Done.

<snip>

> No need for this:
> 
> find_first_bit()
> find_first_zero_bit()
> 
> You are warking all the words when a single search is enough?

> creative use of bitmap_zero(), bitmap_fill() and just doing o whelo
> postcopy_discard_send_rand() would not be better?

<snip>

> > +        mask &= (((unsigned long)1) << ((end & long_bits_mask) + 1)) - 1;
> 
>            bitmap_set(&mask, 0, end);
> 
> 
> Adjust +1/-1 depending on how you do limits?
> 
> BTW, when I need inspiration about how to code functions that deal with
> bits,  I searc for inspiration in bitmap.c.  Sometimes function already
> exist, and otherwise, things like BITS_PER_LONG, etc, are already
> defined there.

OK, I've reworked it using the bitmap/bitops.h functions:
   1 file changed, 128 insertions(+), 220 deletions(-)

(still untested).
Doing it this way it's hand to be two iterations, one for
fixing up partially sent host pages, and the second for fixing up 
partially dirtied pages.

I'll try and find a !x86 to try it on.


> > +    /* Easiest way to make sure we don't resume in the middle of a 
> > host-page */
> > +    last_seen_block = NULL;
> > +    last_sent_block = NULL;
> 
> Best names ever.  And you have to blame me at least for the second one
> to appear :p
> 
> 
> > +
> > +    /*
> > +     * The currently worst known ratio is ARM that has 1kB target pages, 
> > and
> > +     * can have 64kB host pages, which is thus inconveniently larger than 
> > a long
> > +     * on ARM (32bits), and a long is the underlying element of the 
> > migration
> > +     * bitmaps.
> > +     */
> > +    if (host_bits >= long_bits) {
> > +        /* Deal with the odd case separately */
> > +        return qemu_ram_foreach_block(hostpage_big_chunk_helper, ms);
> > +    } else {
> > +        host_mask =  (1ul << host_bits) - 1;
> > +    }
> 
> You can remove the else enterily and just put the code at top level.
> 
> So, we have three cases:
> 
> - host_bits == target_bits -> NOP
> - host_bits >= long_bits
> - host_bits < long_bits
> 
> Couldn't we merge the last two?  they are very similar, and having two
> code paths looks too much to me?

Yep, so those are gone now.

> > @@ -1405,9 +1664,17 @@ int ram_postcopy_send_discard_bitmap(MigrationState 
> > *ms)
> >      int ret;
> >  
> >      rcu_read_lock();
> > +
> Another not needed.

Moved that back to where the rest of the function came from.

Dave

--
Dr. David Alan Gilbert / address@hidden / Manchester, UK