Re: [Qemu-devel] [Qemu-block] [PATCH] block, migration: Use qemu_madvise inplace of madvise

[Pankaj Gupta <pagupta@redhat.com>]

> 
> * Pankaj Gupta (pagupta@redhat.com) wrote:
> > 
> > Thanks for your comments. I have below query.
> > > 
> > > On Fri 17 Feb 2017 09:06:04 AM CET, Pankaj Gupta wrote:
> > > >  To maintain consistency at all the places use qemu_madvise wrapper
> > > >  inplace of madvise call.
> > > 
> > > >      if (length > 0) {
> > > > -        madvise((uint8_t *) t + offset, length, MADV_DONTNEED);
> > > > +        qemu_madvise((uint8_t *) t + offset, length,
> > > > QEMU_MADV_DONTNEED);
> > > 
> > > This was changed two months ago from qemu_madvise() to madvise(), is
> > > there any reason why you want to revert that change? Those two calls are
> > > not equivalent, please see commit 2f2c8d6b371cfc6689affb0b7e for an
> > > explanation.
> > > 
> > > > -    if (madvise(start, length, MADV_DONTNEED)) {
> > > > +    if (qemu_madvise(start, length, QEMU_MADV_DONTNEED)) {
> > > >          error_report("%s MADV_DONTNEED: %s", __func__,
> > > >          strerror(errno));
> > 
> > I checked history of only change related to 'postcopy'.
> > 
> > For my linux machine:
> > 
> > ./config-host.mak
> > 
> > CONFIG_MADVISE=y
> > CONFIG_POSIX_MADVISE=y
> > 
> > As both these options are set for Linux, every time we call call
> > 'qemu_madvise' ==>"madvise(addr, len, advice);" will
> > be compiled/called. I don't understand why '2f2c8d6b371cfc6689affb0b7e'
> > explicitly changed for :"#ifdef CONFIG_LINUX"
> > I think its better to write generic function maybe in a wrapper then to
> > conditionally set something at different places.
> 
> No; the problem is that the behaviours are different.
> You're right that the current build on Linux defines MADVISE and thus we are
> safe because qemu_madvise
> takes teh CONFIG_MADVISE/madvise route - but we need to be explicit that it's
> only
> the madvise() route that's safe, not any of the calls implemented by
> qemu_madvise, because if in the future someone was to rearrange qemu_madvise
> to prefer posix_madvise postcopy would break in a very subtle way.

Agree. 
We can add comment explaining this?

> 
> IMHO it might even be better to remove the definition of QEMU_MADV_DONTNEED
> altogether
> and make a name that wasn't ambiguous between the two, since the posix
> definition is
> so different.

I think 'posix_madvise' was added for systems which didnot have 'madvise'.
If I look at makefile, first we check what all calls are available and then 
set config option accordingly. We give 'madvise' precedence over 'posix_madvise' 
if both are present. 

For the systems which don't have madvise call 'posix_madvise' is called which as per
discussion is not right thing for 'DONTNEED' option. It will not give desired results.

Either we have to find right alternative or else it is already broken for systems which
don't support madvise.

> 
> Dave
> 
> > int qemu_madvise(void *addr, size_t len, int advice)
> > {
> >     if (advice == QEMU_MADV_INVALID) {
> >         errno = EINVAL;
> >         return -1;
> >     }
> > #if defined(CONFIG_MADVISE)
> >     return madvise(addr, len, advice);
> > #elif defined(CONFIG_POSIX_MADVISE)
> >     return posix_madvise(addr, len, advice);
> > #else
> >     errno = EINVAL;
> >     return -1;
> > #endif
> > }
> > 
> > > 
> > > And this is the same case.
> > > 
> > > Berto
> > > 
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> 
>