Re: [Qemu-devel] [PATCH v9 35/56] Postcopy: Calculate discard

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

* Juan Quintela (quintela@redhat.com) wrote:
> "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> >
> > Where postcopy is preceeded by a period of precopy, the destination will
> > have received pages that may have been dirtied on the source after the
> > page was sent.  The destination must throw these pages away before
> > starting it's CPUs.
> >
> > Calculate list of sent & dirty pages
> > Provide helpers on the destination side to discard these.
> >
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> 
> Reviewed-by: Juan Quintela <quintela@redhat.com>
> 
> But please, send a fix for:
> 
> >      return ret;
> >  }
> >  
> > +/**
> > + * postcopy_ram_discard_range: Discard a range of memory.
> > + * We can assume that if we've been called postcopy_ram_hosttest returned true.
> > + *
> > + * @mis: Current incoming migration state.
> > + * @start, @length: range of memory to discard.
> > + *
> > + * returns: 0 on success.
> > + */
> > +int postcopy_ram_discard_range(MigrationIncomingState *mis, uint8_t *start,
> > +                               size_t length)
> > +{
> > +    trace_postcopy_ram_discard_range(start, length);
> > +    if (madvise(start, length, MADV_DONTNEED)) {
> 
> qemu_madvise(start, length, QEMU_MADV_DONTNEED)

I thought I'd better check before making a patch for that,
and it turns out you have to be really careful.
qemu_madvise may call posix_madvise instead of madvise (depending
what config decides); you might think that a
madvise(..,..,MADV_DONTNEED) is the same as a
posix_madvise(..,..,POSIX_MADV_DONTNEED)

but actually on Linux a posix_madvise does precisely nothing;
from the glibc source:

{<>}{<>}{<>}{<>}{<>}{<>}{<>}{<>}{<>}{<>}{<>}{<>}{<>}{<>}{<>}{<>}{<>}{<>}{<>}{<>}
posix_madvise (void *addr, size_t len, int advice)
{
  /* We have one problem: the kernel's MADV_DONTNEED does not
     correspond to POSIX's POSIX_MADV_DONTNEED.  The former simply
     discards changes made to the memory without writing it back to
     disk, if this would be necessary.  The POSIX behavior does not
     allow this.  There is no functionality mapping the POSIX behavior
     so far so we ignore that advice for now.  */
  if (advice == POSIX_MADV_DONTNEED)
    return 0;

{<>}{<>}{<>}{<>}{<>}{<>}{<>}{<>}{<>}{<>}{<>}{<>}{<>}{<>}{<>}{<>}{<>}{<>}{<>}{<>}

oh and notice how it returns with a 'return 0' so we can't tell if we
fall down that trap.

So, it's best to stick to an actual, solid, good old fashioned madvise
which does what it was asked.

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