Re: [Qemu-devel] [PATCH 04/15] Fold postcopy_ram_discard_range into ram_discard_range

[Dr. David Alan Gilbert]

* Juan Quintela (address@hidden) wrote:
> "Dr. David Alan Gilbert (git)" <address@hidden> wrote:
> > From: "Dr. David Alan Gilbert" <address@hidden>
> >
> > I'd created this weird pair of:
> >   ram_discard_range   that lived in migration/ram.c
> >      Which is built target-dependent so can access the insides of
> >      RAMBlock
> >
> >   postcopy_ram_discard_range
> >      Which actually does the discard.
> >
> > Flatten these down into ram_discard_range, this relies on the
> > CONFIG_MADVISE check to be sure we have the real DONTNEED madvise
> > rather than the posix variant that isn't guaranteed to discard pages.
> >
> > Signed-off-by: Dr. David Alan Gilbert <address@hidden>
> 
> Reviewed-by: Juan Quintela <address@hidden>
> 
> > @@ -1877,7 +1879,15 @@ int ram_discard_range(MigrationIncomingState *mis,
> >                           host_endaddr);
> >              goto err;
> >          }
> > -        ret = postcopy_ram_discard_range(mis, host_startaddr, length);
> > +        errno = ENOTSUP;
> > +#if defined(CONFIG_MADVISE)
> > +        ret = qemu_madvise(host_startaddr, length, QEMU_MADV_DONTNEED);
> > +#endif
> 
> This really looks weird, we shouldn't arrive here if CONFIG_MADVISE is
> not configured, no?

You'd hope so ...but.

ram.c is compiled for everyone, so it's not Linux specific for example.
The definitions of QEMU_MADV_DONTNEED and qemu_madvise is a bit odd;
if you're unlucky enough to be on a platform that is
 !CONFIG_MADVISE but is CONFIG_POSIX_MADVISE  then 
you end up using posix_madvise(POSIX_MADV_DONTNEED) - which is different
semantics than madvise MADV_DONTNEED - because the posix variant treats
it as advise for the kernel, where the madvise() case treats
it as an instruction to throw the page away (I'm not sure how
portable that semantic is).

Dave

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