Re: [Qemu-devel] [PATCH v4 29/47] Postcopy page-map-incoming (PMI) structure

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* David Gibson (david@gibson.dropbear.id.au) wrote:
> On Fri, Oct 03, 2014 at 06:47:35PM +0100, Dr. David Alan Gilbert (git) wrote:
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > 
> > The PMI holds the state of each page on the incoming side,
> > so that we can tell if the page is missing, already received
> > or there is a request outstanding for it.
> > 
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> 
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> 
> Though there are a couple of minor comments below:

<snip>

> > +/* ---------------------------------------------------------------------- */
> > +/* Postcopy pagemap-inbound (pmi) - data structures that record the       */
> > +/* state of each page used by the inbound postcopy                        */
> > +/* It's a pair of bitmaps (of the same structure as the migration bitmaps)*/
> > +/* holding one bit per target-page, although all operations work on host  */
> > +/* pages.                                                                 */
> > +__attribute__ (( unused )) /* Until later in patch series */
> > +static void postcopy_pmi_init(MigrationIncomingState *mis, size_t ram_pages)
> > +{
> > +    unsigned int tpb = qemu_target_page_bits();
> > +    unsigned long host_bits;
> > +
> > +    qemu_mutex_init(&mis->postcopy_pmi.mutex);
> > +    mis->postcopy_pmi.received_map = bitmap_new(ram_pages);
> > +    mis->postcopy_pmi.requested_map = bitmap_new(ram_pages);
> > +    bitmap_clear(mis->postcopy_pmi.received_map, 0, ram_pages);
> > +    bitmap_clear(mis->postcopy_pmi.requested_map, 0, ram_pages);
> > +    /*
> > +     * Each bit in the map represents one 'target page' which is no bigger
> > +     * than a host page but can be smaller.  It's useful to have some
> > +     * convenience masks for later
> 
> So, there's no inherent reason a target page couldn't be bigger than a
> host page.  It's fair enough not to handle that case for now, but
> something somewhere should probably verify that it's no the case.

I've added a guard for this in the host test.

> > +     */
> > +
> > +    /*
> > +     * The number of bits one host page takes up in the bitmap
> > +     * e.g. on a 64k host page, 4k Target page, host_bits=64/4=16
> > +     */
> > +    host_bits = sysconf(_SC_PAGESIZE) / (1ul << tpb);
> > +    /* Should be a power of 2 */
> > +    assert(host_bits && !(host_bits & (host_bits - 1)));
> > +    /*
> > +     * If the host_bits isn't a division of the number of bits in long
> > +     * then the code gets a lot more complex; disallow for now
> > +     * (I'm not aware of a system where it's true anyway)
> > +     */
> > +    assert(((sizeof(long) * 8) % host_bits) == 0);
> > +
> > +    mis->postcopy_pmi.host_bits = host_bits;
> > +    /* A mask, starting at bit 0, containing host_bits continuous set bits */
> > +    mis->postcopy_pmi.host_mask =  (1ul << host_bits) - 1;
> > +
> > +    assert((ram_pages % host_bits) == 0);
> > +}
> > +
> > +void postcopy_pmi_destroy(MigrationIncomingState *mis)
> > +{
> > +    if (mis->postcopy_pmi.received_map) {
> > +        g_free(mis->postcopy_pmi.received_map);
> 
> g_free() is safe to call on NULL anyway, isn't it?

It is; fixed.

> > +/*
> > + * Retrieve the state of the given page
> > + * Note: This version for use by callers already holding the lock
> > + */
> > +static PostcopyPMIState postcopy_pmi_get_state_nolock(
> > +                            MigrationIncomingState *mis,
> > +                            size_t bitmap_index)
> > +{
> > +    bool received, requested;
> > +
> > +    received = test_hpbits(mis, bitmap_index, mis->postcopy_pmi.received_map);
> > +    requested = test_hpbits(mis, bitmap_index, mis->postcopy_pmi.requested_map);
> > +
> > +    if (received) {
> > +        assert(!requested);
> 
> Clearing the requested bit when you set the received bit seems a bit
> pointless.  (requested && received) isn't meaningfully different from
> (!requested && received) but there seems no reason to go to extra
> trouble to avoid that state, and having the record might be
> interesting for gathering statistics.

Hmm yes I think you're right; but I want to think about it to convince me a
bit more; this code originally started off as two really seprate bitmaps
and has slowly morphed into really representing 3 states.  I've added it
to a TODO.

> > +/* Called by ram_load prior to mapping the page */
> > +void postcopy_hook_early_receive(MigrationIncomingState *mis,
> > +                                 size_t bitmap_index)
> > +{
> > +    if (mis->postcopy_ram_state == POSTCOPY_RAM_INCOMING_ADVISE) {
> 
> A silent no-op if you're not in the expected migration phase doesn't
> seem right.  Should this be an assert() instead?

No.  This routine is called by the RAM code prior to doing anything with
the page, but it does it in all postcopy states; it's just that we only
care about it in the ADVISE state (it makes things a little cleaner - 
the RAM code no longer has to know about the postcopy stages).

Dave

> > +        /*
> > +         * If we're in precopy-advise mode we need to track received pages even
> > +         * though we don't need to place pages atomically yet.
> > +         * In advise mode there's only a single thread, so don't need locks
> > +         */
> > +        set_bit(bitmap_index, mis->postcopy_pmi.received_map);
> > +    }
> > +}
> > +
> >  int postcopy_ram_hosttest(void)
> >  {
> >      /* TODO: Needs guarding with CONFIG_ once we have libc's that have the defs
> > @@ -156,5 +369,12 @@ int postcopy_ram_hosttest(void)
> >      return -1;
> >  }
> >  
> > +/* Called by ram_load prior to mapping the page */
> > +void postcopy_hook_early_receive(MigrationIncomingState *mis,
> > +                                 size_t bitmap_index)
> > +{
> > +    /* We don't support postcopy so don't care */
> > +}
> > +
> >  #endif
> >  
> 
> -- 
> David Gibson			| I'll have my music baroque, and my code
> david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
> 				| _way_ _around_!
> http://www.ozlabs.org/~dgibson


--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK