Re: [Qemu-devel] [PATCH v8 32/54] Postcopy: Maintain sentmap and calculate discard

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

* Juan Quintela (quintela@redhat.com) wrote:
> > +/* **** functions for postcopy ***** */
> > +
> > +/*
> > + * Callback from postcopy_each_ram_send_discard for each RAMBlock
> > + * start,end: Indexes into the bitmap for the first and last bit
> > + *            representing the named block
> > + */
> > +static int postcopy_send_discard_bm_ram(MigrationState *ms,
> > +                                        PostcopyDiscardState *pds,
> > +                                        unsigned long start, unsigned long end)
> > +{
> > +    unsigned long current;
> > +
> > +    for (current = start; current <= end; ) {
> > +        unsigned long set = find_next_bit(ms->sentmap, end + 1, current);
> > +
> > +        if (set <= end) {
> > +            unsigned long zero = find_next_zero_bit(ms->sentmap,
> > +                                                    end + 1, set + 1);
> > +
> > +            if (zero > end) {
> > +                zero = end + 1;
> > +            }
> > +            postcopy_discard_send_range(ms, pds, set, zero - 1);
> > +            current = zero + 1;
> > +        } else {
> > +            current = set;
> > +        }
> > +    }
> 
> I think I undrestood the logic  here at the end....
> 
> But if we change the meaning of postcopy_discard_send_range() from
> (begin, end), to (begin, length), I think everything goes clearer, no?
> 
>         if (set <= end) {
>             unsigned long zero = find_next_zero_bit(ms->sentmap,
>                                                     end + 1, set + 1);
>             unsigned long length;
> 
>             if (zero > end) {
>                 length = end - set;
>             } else {
>                 lenght = zero - 1 - set;
>                 current = zero + 1;
>             }
>             postcopy_discard_send_range(ms, pds, set, len);
>         } else {
>             current = set;
>         }
>     }
> 
> Y would clame that if we call one zero, the other would be called one.
> Or change to set/unset, but that is just me.  Yes, I haven't tested, and
> it is possible that there is a off-by-one somewhere...
> 
> Looking at postocpy_eand_ram_send_discard, I even think that it would be
> a good idea to pass length to this function.

OK, so I've ended up with (build tested only so far):
/*
 * Callback from postcopy_each_ram_send_discard for each RAMBlock
 * Note: At this point the 'unsentmap' is the processed bitmap combined
 *       with the dirtymap; so a '1' means it's either dirty or unsent.
 * start,length: Indexes into the bitmap for the first bit
 *            representing the named block and length in target-pages
 */
static int postcopy_send_discard_bm_ram(MigrationState *ms,
                                        PostcopyDiscardState *pds,
                                        unsigned long start,
                                        unsigned long length)
{
    unsigned long end = start + length; /* one after the end */
    unsigned long current;

    for (current = start; current < end; ) {
        unsigned long one = find_next_bit(ms->unsentmap, end, current);

        if (one <= end) {
            unsigned long zero = find_next_zero_bit(ms->unsentmap,
                                                    end, one + 1);
            unsigned long discard_length;

            if (zero >= end) {
                discard_length = end - one;
            } else {
                discard_length = zero - one;
            }
            postcopy_discard_send_range(ms, pds, one, discard_length);
            current = one + discard_length;
        } else {
            current = one;
        }
    }

    return 0;
}

Dave

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