softmmu/physmem.c | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-)
We have a thin wrapper around madvise, called qemu_madvise, which
provides consistent behavior for the !CONFIG_MADVISE case, and works
around some platform-specific quirks (some platforms only provide
posix_madvise, and some don't offer all 'advise' types). This specific
caller of madvise has never used it, tracing back to its original
introduction in commit e0b266f01dd2 ("migration_completion: Take
current state").
Call qemu_madvise here, to follow the same logic as all of our other
madvise callers. This slightly changes the behavior for
!CONFIG_MADVISE (EINVAL instead of ENOSYS, and a slightly different
error message), but this is now more consistent with other callers
that use qemu_madvise.
Signed-off-by: Andrew Deason <adeason@sinenomine.net>
---
Looking at the history of commits that touch this madvise() call, it
doesn't _look_ like there's any reason to be directly calling madvise vs
qemu_advise (I don't see anything mentioned), but I'm not sure.
softmmu/physmem.c | 12 ++----------
1 file changed, 2 insertions(+), 10 deletions(-)
diff --git a/softmmu/physmem.c b/softmmu/physmem.c
index 43ae70fbe2..900c692b5e 100644
--- a/softmmu/physmem.c
+++ b/softmmu/physmem.c
@@ -3584,40 +3584,32 @@ int ram_block_discard_range(RAMBlock *rb, uint64_t start, size_t length)
rb->idstr, start, length, ret);
goto err;
#endif
}
if (need_madvise) {
/* For normal RAM this causes it to be unmapped,
* for shared memory it causes the local mapping to disappear
* and to fall back on the file contents (which we just
* fallocate'd away).
*/
-#if defined(CONFIG_MADVISE)
if (qemu_ram_is_shared(rb) && rb->fd < 0) {
- ret = madvise(host_startaddr, length, QEMU_MADV_REMOVE);
+ ret = qemu_madvise(host_startaddr, length, QEMU_MADV_REMOVE);
} else {
- ret = madvise(host_startaddr, length, QEMU_MADV_DONTNEED);
+ ret = qemu_madvise(host_startaddr, length, QEMU_MADV_DONTNEED);
}
if (ret) {
ret = -errno;
error_report("ram_block_discard_range: Failed to discard range "
"%s:%" PRIx64 " +%zx (%d)",
rb->idstr, start, length, ret);
goto err;
}
-#else
- ret = -ENOSYS;
- error_report("ram_block_discard_range: MADVISE not available"
- "%s:%" PRIx64 " +%zx (%d)",
- rb->idstr, start, length, ret);
- goto err;
-#endif
}
trace_ram_block_discard_range(rb->idstr, host_startaddr, length,
need_madvise, need_fallocate, ret);
} else {
error_report("ram_block_discard_range: Overrun block '%s' (%" PRIu64
"/%zx/" RAM_ADDR_FMT")",
rb->idstr, start, length, rb->max_length);
}
err:
--
2.11.0On 16.03.22 05:04, Andrew Deason wrote:
> We have a thin wrapper around madvise, called qemu_madvise, which
> provides consistent behavior for the !CONFIG_MADVISE case, and works
> around some platform-specific quirks (some platforms only provide
> posix_madvise, and some don't offer all 'advise' types). This specific
> caller of madvise has never used it, tracing back to its original
> introduction in commit e0b266f01dd2 ("migration_completion: Take
> current state").
>
> Call qemu_madvise here, to follow the same logic as all of our other
> madvise callers. This slightly changes the behavior for
> !CONFIG_MADVISE (EINVAL instead of ENOSYS, and a slightly different
> error message), but this is now more consistent with other callers
> that use qemu_madvise.
>
> Signed-off-by: Andrew Deason <adeason@sinenomine.net>
> ---
> Looking at the history of commits that touch this madvise() call, it
> doesn't _look_ like there's any reason to be directly calling madvise vs
> qemu_advise (I don't see anything mentioned), but I'm not sure.
>
> softmmu/physmem.c | 12 ++----------
> 1 file changed, 2 insertions(+), 10 deletions(-)
>
> diff --git a/softmmu/physmem.c b/softmmu/physmem.c
> index 43ae70fbe2..900c692b5e 100644
> --- a/softmmu/physmem.c
> +++ b/softmmu/physmem.c
> @@ -3584,40 +3584,32 @@ int ram_block_discard_range(RAMBlock *rb, uint64_t start, size_t length)
> rb->idstr, start, length, ret);
> goto err;
> #endif
> }
> if (need_madvise) {
> /* For normal RAM this causes it to be unmapped,
> * for shared memory it causes the local mapping to disappear
> * and to fall back on the file contents (which we just
> * fallocate'd away).
> */
> -#if defined(CONFIG_MADVISE)
> if (qemu_ram_is_shared(rb) && rb->fd < 0) {
> - ret = madvise(host_startaddr, length, QEMU_MADV_REMOVE);
> + ret = qemu_madvise(host_startaddr, length, QEMU_MADV_REMOVE);
> } else {
> - ret = madvise(host_startaddr, length, QEMU_MADV_DONTNEED);
> + ret = qemu_madvise(host_startaddr, length, QEMU_MADV_DONTNEED);
posix_madvise(QEMU_MADV_DONTNEED) has completely different semantics
then madvise() -- it's not a discard that we need here.
So ram_block_discard_range() would now succeed in environments (BSD?)
where it's supposed to fail.
So AFAIKs this isn't sane.
--
Thanks,
David / dhildenb
On Wed, 16 Mar 2022 at 07:53, David Hildenbrand <david@redhat.com> wrote:
>
> On 16.03.22 05:04, Andrew Deason wrote:
> > We have a thin wrapper around madvise, called qemu_madvise, which
> > provides consistent behavior for the !CONFIG_MADVISE case, and works
> > around some platform-specific quirks (some platforms only provide
> > posix_madvise, and some don't offer all 'advise' types). This specific
> > caller of madvise has never used it, tracing back to its original
> > introduction in commit e0b266f01dd2 ("migration_completion: Take
> > current state").
> >
> > Call qemu_madvise here, to follow the same logic as all of our other
> > madvise callers. This slightly changes the behavior for
> > !CONFIG_MADVISE (EINVAL instead of ENOSYS, and a slightly different
> > error message), but this is now more consistent with other callers
> > that use qemu_madvise.
> >
> > Signed-off-by: Andrew Deason <adeason@sinenomine.net>
> > ---
> > Looking at the history of commits that touch this madvise() call, it
> > doesn't _look_ like there's any reason to be directly calling madvise vs
> > qemu_advise (I don't see anything mentioned), but I'm not sure.
> >
> > softmmu/physmem.c | 12 ++----------
> > 1 file changed, 2 insertions(+), 10 deletions(-)
> >
> > diff --git a/softmmu/physmem.c b/softmmu/physmem.c
> > index 43ae70fbe2..900c692b5e 100644
> > --- a/softmmu/physmem.c
> > +++ b/softmmu/physmem.c
> > @@ -3584,40 +3584,32 @@ int ram_block_discard_range(RAMBlock *rb, uint64_t start, size_t length)
> > rb->idstr, start, length, ret);
> > goto err;
> > #endif
> > }
> > if (need_madvise) {
> > /* For normal RAM this causes it to be unmapped,
> > * for shared memory it causes the local mapping to disappear
> > * and to fall back on the file contents (which we just
> > * fallocate'd away).
> > */
> > -#if defined(CONFIG_MADVISE)
> > if (qemu_ram_is_shared(rb) && rb->fd < 0) {
> > - ret = madvise(host_startaddr, length, QEMU_MADV_REMOVE);
> > + ret = qemu_madvise(host_startaddr, length, QEMU_MADV_REMOVE);
> > } else {
> > - ret = madvise(host_startaddr, length, QEMU_MADV_DONTNEED);
> > + ret = qemu_madvise(host_startaddr, length, QEMU_MADV_DONTNEED);
>
> posix_madvise(QEMU_MADV_DONTNEED) has completely different semantics
> then madvise() -- it's not a discard that we need here.
>
> So ram_block_discard_range() would now succeed in environments (BSD?)
> where it's supposed to fail.
>
> So AFAIKs this isn't sane.
But CONFIG_MADVISE just means "host has madvise()"; it doesn't imply
"this is a Linux madvise() with MADV_DONTNEED". Solaris madvise()
doesn't seem to have MADV_DONTNEED at all; a quick look at the
FreeBSD manpage suggests its madvise MADV_DONTNEED is identical
to its posix_madvise MADV_DONTNEED.
If we need "specifically Linux MADV_DONTNEED semantics" maybe we
should define a QEMU_MADV_LINUX_DONTNEED which either (a) does the
right thing or (b) fails, and use qemu_madvise() regardless.
Certainly the current code is pretty fragile to being changed by
people who don't understand the undocumented subtlety behind
the use of a direct madvise() call here.
-- PMM
* Peter Maydell (peter.maydell@linaro.org) wrote:
> On Wed, 16 Mar 2022 at 07:53, David Hildenbrand <david@redhat.com> wrote:
> >
> > On 16.03.22 05:04, Andrew Deason wrote:
> > > We have a thin wrapper around madvise, called qemu_madvise, which
> > > provides consistent behavior for the !CONFIG_MADVISE case, and works
> > > around some platform-specific quirks (some platforms only provide
> > > posix_madvise, and some don't offer all 'advise' types). This specific
> > > caller of madvise has never used it, tracing back to its original
> > > introduction in commit e0b266f01dd2 ("migration_completion: Take
> > > current state").
> > >
> > > Call qemu_madvise here, to follow the same logic as all of our other
> > > madvise callers. This slightly changes the behavior for
> > > !CONFIG_MADVISE (EINVAL instead of ENOSYS, and a slightly different
> > > error message), but this is now more consistent with other callers
> > > that use qemu_madvise.
> > >
> > > Signed-off-by: Andrew Deason <adeason@sinenomine.net>
> > > ---
> > > Looking at the history of commits that touch this madvise() call, it
> > > doesn't _look_ like there's any reason to be directly calling madvise vs
> > > qemu_advise (I don't see anything mentioned), but I'm not sure.
> > >
> > > softmmu/physmem.c | 12 ++----------
> > > 1 file changed, 2 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/softmmu/physmem.c b/softmmu/physmem.c
> > > index 43ae70fbe2..900c692b5e 100644
> > > --- a/softmmu/physmem.c
> > > +++ b/softmmu/physmem.c
> > > @@ -3584,40 +3584,32 @@ int ram_block_discard_range(RAMBlock *rb, uint64_t start, size_t length)
> > > rb->idstr, start, length, ret);
> > > goto err;
> > > #endif
> > > }
> > > if (need_madvise) {
> > > /* For normal RAM this causes it to be unmapped,
> > > * for shared memory it causes the local mapping to disappear
> > > * and to fall back on the file contents (which we just
> > > * fallocate'd away).
> > > */
> > > -#if defined(CONFIG_MADVISE)
> > > if (qemu_ram_is_shared(rb) && rb->fd < 0) {
> > > - ret = madvise(host_startaddr, length, QEMU_MADV_REMOVE);
> > > + ret = qemu_madvise(host_startaddr, length, QEMU_MADV_REMOVE);
> > > } else {
> > > - ret = madvise(host_startaddr, length, QEMU_MADV_DONTNEED);
> > > + ret = qemu_madvise(host_startaddr, length, QEMU_MADV_DONTNEED);
> >
> > posix_madvise(QEMU_MADV_DONTNEED) has completely different semantics
> > then madvise() -- it's not a discard that we need here.
> >
> > So ram_block_discard_range() would now succeed in environments (BSD?)
> > where it's supposed to fail.
> >
> > So AFAIKs this isn't sane.
>
> But CONFIG_MADVISE just means "host has madvise()"; it doesn't imply
> "this is a Linux madvise() with MADV_DONTNEED". Solaris madvise()
> doesn't seem to have MADV_DONTNEED at all; a quick look at the
> FreeBSD manpage suggests its madvise MADV_DONTNEED is identical
> to its posix_madvise MADV_DONTNEED.
>
> If we need "specifically Linux MADV_DONTNEED semantics" maybe we
> should define a QEMU_MADV_LINUX_DONTNEED which either (a) does the
> right thing or (b) fails, and use qemu_madvise() regardless.
>
> Certainly the current code is pretty fragile to being changed by
> people who don't understand the undocumented subtlety behind
> the use of a direct madvise() call here.
Yeh and I'm not sure I can remembe rall the subtleties; there's a big
hairy set of ifdef's in include/qemu/madvise.h that makes
sure we always have the definition of QEMU_MADV_REMOVE/DONTNEED
even on platforms that might not define it themselves.
But I think this code is used for things with different degrees
of care about the semantics; e.g. 'balloon' just cares that
it frees memory up and doesn't care about the detailed semantics
that much; so it's probably fine with that.
Postcopy is much more touchy, but then it's only going to be
calling this on Linux anyway (because of the userfault dependency).
Dave
> -- PMM
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 16.03.22 10:37, Dr. David Alan Gilbert wrote:
> * Peter Maydell (peter.maydell@linaro.org) wrote:
>> On Wed, 16 Mar 2022 at 07:53, David Hildenbrand <david@redhat.com> wrote:
>>>
>>> On 16.03.22 05:04, Andrew Deason wrote:
>>>> We have a thin wrapper around madvise, called qemu_madvise, which
>>>> provides consistent behavior for the !CONFIG_MADVISE case, and works
>>>> around some platform-specific quirks (some platforms only provide
>>>> posix_madvise, and some don't offer all 'advise' types). This specific
>>>> caller of madvise has never used it, tracing back to its original
>>>> introduction in commit e0b266f01dd2 ("migration_completion: Take
>>>> current state").
>>>>
>>>> Call qemu_madvise here, to follow the same logic as all of our other
>>>> madvise callers. This slightly changes the behavior for
>>>> !CONFIG_MADVISE (EINVAL instead of ENOSYS, and a slightly different
>>>> error message), but this is now more consistent with other callers
>>>> that use qemu_madvise.
>>>>
>>>> Signed-off-by: Andrew Deason <adeason@sinenomine.net>
>>>> ---
>>>> Looking at the history of commits that touch this madvise() call, it
>>>> doesn't _look_ like there's any reason to be directly calling madvise vs
>>>> qemu_advise (I don't see anything mentioned), but I'm not sure.
>>>>
>>>> softmmu/physmem.c | 12 ++----------
>>>> 1 file changed, 2 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/softmmu/physmem.c b/softmmu/physmem.c
>>>> index 43ae70fbe2..900c692b5e 100644
>>>> --- a/softmmu/physmem.c
>>>> +++ b/softmmu/physmem.c
>>>> @@ -3584,40 +3584,32 @@ int ram_block_discard_range(RAMBlock *rb, uint64_t start, size_t length)
>>>> rb->idstr, start, length, ret);
>>>> goto err;
>>>> #endif
>>>> }
>>>> if (need_madvise) {
>>>> /* For normal RAM this causes it to be unmapped,
>>>> * for shared memory it causes the local mapping to disappear
>>>> * and to fall back on the file contents (which we just
>>>> * fallocate'd away).
>>>> */
>>>> -#if defined(CONFIG_MADVISE)
>>>> if (qemu_ram_is_shared(rb) && rb->fd < 0) {
>>>> - ret = madvise(host_startaddr, length, QEMU_MADV_REMOVE);
>>>> + ret = qemu_madvise(host_startaddr, length, QEMU_MADV_REMOVE);
>>>> } else {
>>>> - ret = madvise(host_startaddr, length, QEMU_MADV_DONTNEED);
>>>> + ret = qemu_madvise(host_startaddr, length, QEMU_MADV_DONTNEED);
>>>
>>> posix_madvise(QEMU_MADV_DONTNEED) has completely different semantics
>>> then madvise() -- it's not a discard that we need here.
>>>
>>> So ram_block_discard_range() would now succeed in environments (BSD?)
>>> where it's supposed to fail.
>>>
>>> So AFAIKs this isn't sane.
>>
>> But CONFIG_MADVISE just means "host has madvise()"; it doesn't imply
>> "this is a Linux madvise() with MADV_DONTNEED". Solaris madvise()
>> doesn't seem to have MADV_DONTNEED at all; a quick look at the
>> FreeBSD manpage suggests its madvise MADV_DONTNEED is identical
>> to its posix_madvise MADV_DONTNEED.
>>
>> If we need "specifically Linux MADV_DONTNEED semantics" maybe we
>> should define a QEMU_MADV_LINUX_DONTNEED which either (a) does the
>> right thing or (b) fails, and use qemu_madvise() regardless.
>>
>> Certainly the current code is pretty fragile to being changed by
>> people who don't understand the undocumented subtlety behind
>> the use of a direct madvise() call here.
>
> Yeh and I'm not sure I can remembe rall the subtleties; there's a big
> hairy set of ifdef's in include/qemu/madvise.h that makes
> sure we always have the definition of QEMU_MADV_REMOVE/DONTNEED
> even on platforms that might not define it themselves.
>
> But I think this code is used for things with different degrees
> of care about the semantics; e.g. 'balloon' just cares that
> it frees memory up and doesn't care about the detailed semantics
> that much; so it's probably fine with that.
> Postcopy is much more touchy, but then it's only going to be
> calling this on Linux anyway (because of the userfault dependency).
MADV_DONTNEED/MADV_REMOVE only provides discard semantics on Linux IIRC
-- and that's what we want to achieve: ram_block_discard_range()
So I agree with Peter that we might want to make this more explicit.
--
Thanks,
David / dhildenb
On Wed, 16 Mar 2022 10:41:41 +0100
David Hildenbrand <david@redhat.com> wrote:
> On 16.03.22 10:37, Dr. David Alan Gilbert wrote:
> > * Peter Maydell (peter.maydell@linaro.org) wrote:
> >> On Wed, 16 Mar 2022 at 07:53, David Hildenbrand <david@redhat.com> wrote:
> >>>
> >>> On 16.03.22 05:04, Andrew Deason wrote:
> >>>> We have a thin wrapper around madvise, called qemu_madvise, which
> >>>> provides consistent behavior for the !CONFIG_MADVISE case, and works
> >>>> around some platform-specific quirks (some platforms only provide
> >>>> posix_madvise, and some don't offer all 'advise' types). This specific
> >>>> caller of madvise has never used it, tracing back to its original
> >>>> introduction in commit e0b266f01dd2 ("migration_completion: Take
> >>>> current state").
> >>>>
> >>>> Call qemu_madvise here, to follow the same logic as all of our other
> >>>> madvise callers. This slightly changes the behavior for
> >>>> !CONFIG_MADVISE (EINVAL instead of ENOSYS, and a slightly different
> >>>> error message), but this is now more consistent with other callers
> >>>> that use qemu_madvise.
> >>>>
> >>>> Signed-off-by: Andrew Deason <adeason@sinenomine.net>
> >>>> ---
> >>>> Looking at the history of commits that touch this madvise() call, it
> >>>> doesn't _look_ like there's any reason to be directly calling madvise vs
> >>>> qemu_advise (I don't see anything mentioned), but I'm not sure.
> >>>>
> >>>> softmmu/physmem.c | 12 ++----------
> >>>> 1 file changed, 2 insertions(+), 10 deletions(-)
> >>>>
> >>>> diff --git a/softmmu/physmem.c b/softmmu/physmem.c
> >>>> index 43ae70fbe2..900c692b5e 100644
> >>>> --- a/softmmu/physmem.c
> >>>> +++ b/softmmu/physmem.c
> >>>> @@ -3584,40 +3584,32 @@ int ram_block_discard_range(RAMBlock *rb, uint64_t start, size_t length)
> >>>> rb->idstr, start, length, ret);
> >>>> goto err;
> >>>> #endif
> >>>> }
> >>>> if (need_madvise) {
> >>>> /* For normal RAM this causes it to be unmapped,
> >>>> * for shared memory it causes the local mapping to disappear
> >>>> * and to fall back on the file contents (which we just
> >>>> * fallocate'd away).
> >>>> */
> >>>> -#if defined(CONFIG_MADVISE)
> >>>> if (qemu_ram_is_shared(rb) && rb->fd < 0) {
> >>>> - ret = madvise(host_startaddr, length, QEMU_MADV_REMOVE);
> >>>> + ret = qemu_madvise(host_startaddr, length, QEMU_MADV_REMOVE);
> >>>> } else {
> >>>> - ret = madvise(host_startaddr, length, QEMU_MADV_DONTNEED);
> >>>> + ret = qemu_madvise(host_startaddr, length, QEMU_MADV_DONTNEED);
> >>>
> >>> posix_madvise(QEMU_MADV_DONTNEED) has completely different semantics
> >>> then madvise() -- it's not a discard that we need here.
> >>>
> >>> So ram_block_discard_range() would now succeed in environments (BSD?)
> >>> where it's supposed to fail.
> >>>
> >>> So AFAIKs this isn't sane.
> >>
> >> But CONFIG_MADVISE just means "host has madvise()"; it doesn't imply
> >> "this is a Linux madvise() with MADV_DONTNEED". Solaris madvise()
> >> doesn't seem to have MADV_DONTNEED at all; a quick look at the
> >> FreeBSD manpage suggests its madvise MADV_DONTNEED is identical
> >> to its posix_madvise MADV_DONTNEED.
> >>
> >> If we need "specifically Linux MADV_DONTNEED semantics" maybe we
> >> should define a QEMU_MADV_LINUX_DONTNEED which either (a) does the
> >> right thing or (b) fails, and use qemu_madvise() regardless.
> >>
> >> Certainly the current code is pretty fragile to being changed by
> >> people who don't understand the undocumented subtlety behind
> >> the use of a direct madvise() call here.
> >
> > Yeh and I'm not sure I can remembe rall the subtleties; there's a big
> > hairy set of ifdef's in include/qemu/madvise.h that makes
> > sure we always have the definition of QEMU_MADV_REMOVE/DONTNEED
> > even on platforms that might not define it themselves.
> >
> > But I think this code is used for things with different degrees
> > of care about the semantics; e.g. 'balloon' just cares that
> > it frees memory up and doesn't care about the detailed semantics
> > that much; so it's probably fine with that.
> > Postcopy is much more touchy, but then it's only going to be
> > calling this on Linux anyway (because of the userfault dependency).
>
> MADV_DONTNEED/MADV_REMOVE only provides discard semantics on Linux IIRC
> -- and that's what we want to achieve: ram_block_discard_range()
>
> So I agree with Peter that we might want to make this more explicit.
I was looking at the comments/history around this code to try to make
this more explicit/clear, and it seems like the whole function is very
Linux-specific. All we ever do is:
- fallocate(FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE)
- madvise(MADV_REMOVE)
- madvise(MADV_DONTNEED) with Linux semantics
All of those operations are Linux-only, so trying to figure out the
cross-platform way to model this seems kind of pointless. Is it fine to
just #ifdef the whole thing to be just for Linux?
--
Andrew Deason
adeason@sinenomine.net
On 22.03.22 17:39, Andrew Deason wrote:
> On Wed, 16 Mar 2022 10:41:41 +0100
> David Hildenbrand <david@redhat.com> wrote:
>
>> On 16.03.22 10:37, Dr. David Alan Gilbert wrote:
>>> * Peter Maydell (peter.maydell@linaro.org) wrote:
>>>> On Wed, 16 Mar 2022 at 07:53, David Hildenbrand <david@redhat.com> wrote:
>>>>>
>>>>> On 16.03.22 05:04, Andrew Deason wrote:
>>>>>> We have a thin wrapper around madvise, called qemu_madvise, which
>>>>>> provides consistent behavior for the !CONFIG_MADVISE case, and works
>>>>>> around some platform-specific quirks (some platforms only provide
>>>>>> posix_madvise, and some don't offer all 'advise' types). This specific
>>>>>> caller of madvise has never used it, tracing back to its original
>>>>>> introduction in commit e0b266f01dd2 ("migration_completion: Take
>>>>>> current state").
>>>>>>
>>>>>> Call qemu_madvise here, to follow the same logic as all of our other
>>>>>> madvise callers. This slightly changes the behavior for
>>>>>> !CONFIG_MADVISE (EINVAL instead of ENOSYS, and a slightly different
>>>>>> error message), but this is now more consistent with other callers
>>>>>> that use qemu_madvise.
>>>>>>
>>>>>> Signed-off-by: Andrew Deason <adeason@sinenomine.net>
>>>>>> ---
>>>>>> Looking at the history of commits that touch this madvise() call, it
>>>>>> doesn't _look_ like there's any reason to be directly calling madvise vs
>>>>>> qemu_advise (I don't see anything mentioned), but I'm not sure.
>>>>>>
>>>>>> softmmu/physmem.c | 12 ++----------
>>>>>> 1 file changed, 2 insertions(+), 10 deletions(-)
>>>>>>
>>>>>> diff --git a/softmmu/physmem.c b/softmmu/physmem.c
>>>>>> index 43ae70fbe2..900c692b5e 100644
>>>>>> --- a/softmmu/physmem.c
>>>>>> +++ b/softmmu/physmem.c
>>>>>> @@ -3584,40 +3584,32 @@ int ram_block_discard_range(RAMBlock *rb, uint64_t start, size_t length)
>>>>>> rb->idstr, start, length, ret);
>>>>>> goto err;
>>>>>> #endif
>>>>>> }
>>>>>> if (need_madvise) {
>>>>>> /* For normal RAM this causes it to be unmapped,
>>>>>> * for shared memory it causes the local mapping to disappear
>>>>>> * and to fall back on the file contents (which we just
>>>>>> * fallocate'd away).
>>>>>> */
>>>>>> -#if defined(CONFIG_MADVISE)
>>>>>> if (qemu_ram_is_shared(rb) && rb->fd < 0) {
>>>>>> - ret = madvise(host_startaddr, length, QEMU_MADV_REMOVE);
>>>>>> + ret = qemu_madvise(host_startaddr, length, QEMU_MADV_REMOVE);
>>>>>> } else {
>>>>>> - ret = madvise(host_startaddr, length, QEMU_MADV_DONTNEED);
>>>>>> + ret = qemu_madvise(host_startaddr, length, QEMU_MADV_DONTNEED);
>>>>>
>>>>> posix_madvise(QEMU_MADV_DONTNEED) has completely different semantics
>>>>> then madvise() -- it's not a discard that we need here.
>>>>>
>>>>> So ram_block_discard_range() would now succeed in environments (BSD?)
>>>>> where it's supposed to fail.
>>>>>
>>>>> So AFAIKs this isn't sane.
>>>>
>>>> But CONFIG_MADVISE just means "host has madvise()"; it doesn't imply
>>>> "this is a Linux madvise() with MADV_DONTNEED". Solaris madvise()
>>>> doesn't seem to have MADV_DONTNEED at all; a quick look at the
>>>> FreeBSD manpage suggests its madvise MADV_DONTNEED is identical
>>>> to its posix_madvise MADV_DONTNEED.
>>>>
>>>> If we need "specifically Linux MADV_DONTNEED semantics" maybe we
>>>> should define a QEMU_MADV_LINUX_DONTNEED which either (a) does the
>>>> right thing or (b) fails, and use qemu_madvise() regardless.
>>>>
>>>> Certainly the current code is pretty fragile to being changed by
>>>> people who don't understand the undocumented subtlety behind
>>>> the use of a direct madvise() call here.
>>>
>>> Yeh and I'm not sure I can remembe rall the subtleties; there's a big
>>> hairy set of ifdef's in include/qemu/madvise.h that makes
>>> sure we always have the definition of QEMU_MADV_REMOVE/DONTNEED
>>> even on platforms that might not define it themselves.
>>>
>>> But I think this code is used for things with different degrees
>>> of care about the semantics; e.g. 'balloon' just cares that
>>> it frees memory up and doesn't care about the detailed semantics
>>> that much; so it's probably fine with that.
>>> Postcopy is much more touchy, but then it's only going to be
>>> calling this on Linux anyway (because of the userfault dependency).
>>
>> MADV_DONTNEED/MADV_REMOVE only provides discard semantics on Linux IIRC
>> -- and that's what we want to achieve: ram_block_discard_range()
>>
>> So I agree with Peter that we might want to make this more explicit.
>
> I was looking at the comments/history around this code to try to make
> this more explicit/clear, and it seems like the whole function is very
> Linux-specific. All we ever do is:
>
> - fallocate(FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE)
> - madvise(MADV_REMOVE)
> - madvise(MADV_DONTNEED) with Linux semantics
>
> All of those operations are Linux-only, so trying to figure out the
> cross-platform way to model this seems kind of pointless. Is it fine to
> just #ifdef the whole thing to be just for Linux?
>
Fine with me, as long as it compiles on other OSs :)
--
Thanks,
David / dhildenb
* Andrew Deason (adeason@sinenomine.net) wrote:
> On Wed, 16 Mar 2022 10:41:41 +0100
> David Hildenbrand <david@redhat.com> wrote:
>
> > On 16.03.22 10:37, Dr. David Alan Gilbert wrote:
> > > * Peter Maydell (peter.maydell@linaro.org) wrote:
> > >> On Wed, 16 Mar 2022 at 07:53, David Hildenbrand <david@redhat.com> wrote:
> > >>>
> > >>> On 16.03.22 05:04, Andrew Deason wrote:
> > >>>> We have a thin wrapper around madvise, called qemu_madvise, which
> > >>>> provides consistent behavior for the !CONFIG_MADVISE case, and works
> > >>>> around some platform-specific quirks (some platforms only provide
> > >>>> posix_madvise, and some don't offer all 'advise' types). This specific
> > >>>> caller of madvise has never used it, tracing back to its original
> > >>>> introduction in commit e0b266f01dd2 ("migration_completion: Take
> > >>>> current state").
> > >>>>
> > >>>> Call qemu_madvise here, to follow the same logic as all of our other
> > >>>> madvise callers. This slightly changes the behavior for
> > >>>> !CONFIG_MADVISE (EINVAL instead of ENOSYS, and a slightly different
> > >>>> error message), but this is now more consistent with other callers
> > >>>> that use qemu_madvise.
> > >>>>
> > >>>> Signed-off-by: Andrew Deason <adeason@sinenomine.net>
> > >>>> ---
> > >>>> Looking at the history of commits that touch this madvise() call, it
> > >>>> doesn't _look_ like there's any reason to be directly calling madvise vs
> > >>>> qemu_advise (I don't see anything mentioned), but I'm not sure.
> > >>>>
> > >>>> softmmu/physmem.c | 12 ++----------
> > >>>> 1 file changed, 2 insertions(+), 10 deletions(-)
> > >>>>
> > >>>> diff --git a/softmmu/physmem.c b/softmmu/physmem.c
> > >>>> index 43ae70fbe2..900c692b5e 100644
> > >>>> --- a/softmmu/physmem.c
> > >>>> +++ b/softmmu/physmem.c
> > >>>> @@ -3584,40 +3584,32 @@ int ram_block_discard_range(RAMBlock *rb, uint64_t start, size_t length)
> > >>>> rb->idstr, start, length, ret);
> > >>>> goto err;
> > >>>> #endif
> > >>>> }
> > >>>> if (need_madvise) {
> > >>>> /* For normal RAM this causes it to be unmapped,
> > >>>> * for shared memory it causes the local mapping to disappear
> > >>>> * and to fall back on the file contents (which we just
> > >>>> * fallocate'd away).
> > >>>> */
> > >>>> -#if defined(CONFIG_MADVISE)
> > >>>> if (qemu_ram_is_shared(rb) && rb->fd < 0) {
> > >>>> - ret = madvise(host_startaddr, length, QEMU_MADV_REMOVE);
> > >>>> + ret = qemu_madvise(host_startaddr, length, QEMU_MADV_REMOVE);
> > >>>> } else {
> > >>>> - ret = madvise(host_startaddr, length, QEMU_MADV_DONTNEED);
> > >>>> + ret = qemu_madvise(host_startaddr, length, QEMU_MADV_DONTNEED);
> > >>>
> > >>> posix_madvise(QEMU_MADV_DONTNEED) has completely different semantics
> > >>> then madvise() -- it's not a discard that we need here.
> > >>>
> > >>> So ram_block_discard_range() would now succeed in environments (BSD?)
> > >>> where it's supposed to fail.
> > >>>
> > >>> So AFAIKs this isn't sane.
> > >>
> > >> But CONFIG_MADVISE just means "host has madvise()"; it doesn't imply
> > >> "this is a Linux madvise() with MADV_DONTNEED". Solaris madvise()
> > >> doesn't seem to have MADV_DONTNEED at all; a quick look at the
> > >> FreeBSD manpage suggests its madvise MADV_DONTNEED is identical
> > >> to its posix_madvise MADV_DONTNEED.
> > >>
> > >> If we need "specifically Linux MADV_DONTNEED semantics" maybe we
> > >> should define a QEMU_MADV_LINUX_DONTNEED which either (a) does the
> > >> right thing or (b) fails, and use qemu_madvise() regardless.
> > >>
> > >> Certainly the current code is pretty fragile to being changed by
> > >> people who don't understand the undocumented subtlety behind
> > >> the use of a direct madvise() call here.
> > >
> > > Yeh and I'm not sure I can remembe rall the subtleties; there's a big
> > > hairy set of ifdef's in include/qemu/madvise.h that makes
> > > sure we always have the definition of QEMU_MADV_REMOVE/DONTNEED
> > > even on platforms that might not define it themselves.
> > >
> > > But I think this code is used for things with different degrees
> > > of care about the semantics; e.g. 'balloon' just cares that
> > > it frees memory up and doesn't care about the detailed semantics
> > > that much; so it's probably fine with that.
> > > Postcopy is much more touchy, but then it's only going to be
> > > calling this on Linux anyway (because of the userfault dependency).
> >
> > MADV_DONTNEED/MADV_REMOVE only provides discard semantics on Linux IIRC
> > -- and that's what we want to achieve: ram_block_discard_range()
> >
> > So I agree with Peter that we might want to make this more explicit.
>
> I was looking at the comments/history around this code to try to make
> this more explicit/clear, and it seems like the whole function is very
> Linux-specific. All we ever do is:
>
> - fallocate(FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE)
> - madvise(MADV_REMOVE)
> - madvise(MADV_DONTNEED) with Linux semantics
>
> All of those operations are Linux-only, so trying to figure out the
> cross-platform way to model this seems kind of pointless. Is it fine to
> just #ifdef the whole thing to be just for Linux?
For ballooning we don't really need Linux semantics; we just need it to
use less host memory. Postcopy needs the more careful semantics though.
Dave
> --
> Andrew Deason
> adeason@sinenomine.net
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On Tue, 22 Mar 2022 16:53:05 +0000
"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> * Andrew Deason (adeason@sinenomine.net) wrote:
> > On Wed, 16 Mar 2022 10:41:41 +0100
> > David Hildenbrand <david@redhat.com> wrote:
> >
> > > On 16.03.22 10:37, Dr. David Alan Gilbert wrote:
> > > > * Peter Maydell (peter.maydell@linaro.org) wrote:
> > > >> On Wed, 16 Mar 2022 at 07:53, David Hildenbrand <david@redhat.com> wrote:
> > > >>>
> > > >>> On 16.03.22 05:04, Andrew Deason wrote:
> > > >>>> We have a thin wrapper around madvise, called qemu_madvise, which
> > > >>>> provides consistent behavior for the !CONFIG_MADVISE case, and works
> > > >>>> around some platform-specific quirks (some platforms only provide
> > > >>>> posix_madvise, and some don't offer all 'advise' types). This specific
> > > >>>> caller of madvise has never used it, tracing back to its original
> > > >>>> introduction in commit e0b266f01dd2 ("migration_completion: Take
> > > >>>> current state").
> > > >>>>
> > > >>>> Call qemu_madvise here, to follow the same logic as all of our other
> > > >>>> madvise callers. This slightly changes the behavior for
> > > >>>> !CONFIG_MADVISE (EINVAL instead of ENOSYS, and a slightly different
> > > >>>> error message), but this is now more consistent with other callers
> > > >>>> that use qemu_madvise.
> > > >>>>
> > > >>>> Signed-off-by: Andrew Deason <adeason@sinenomine.net>
> > > >>>> ---
> > > >>>> Looking at the history of commits that touch this madvise() call, it
> > > >>>> doesn't _look_ like there's any reason to be directly calling madvise vs
> > > >>>> qemu_advise (I don't see anything mentioned), but I'm not sure.
> > > >>>>
> > > >>>> softmmu/physmem.c | 12 ++----------
> > > >>>> 1 file changed, 2 insertions(+), 10 deletions(-)
> > > >>>>
> > > >>>> diff --git a/softmmu/physmem.c b/softmmu/physmem.c
> > > >>>> index 43ae70fbe2..900c692b5e 100644
> > > >>>> --- a/softmmu/physmem.c
> > > >>>> +++ b/softmmu/physmem.c
> > > >>>> @@ -3584,40 +3584,32 @@ int ram_block_discard_range(RAMBlock *rb, uint64_t start, size_t length)
> > > >>>> rb->idstr, start, length, ret);
> > > >>>> goto err;
> > > >>>> #endif
> > > >>>> }
> > > >>>> if (need_madvise) {
> > > >>>> /* For normal RAM this causes it to be unmapped,
> > > >>>> * for shared memory it causes the local mapping to disappear
> > > >>>> * and to fall back on the file contents (which we just
> > > >>>> * fallocate'd away).
> > > >>>> */
> > > >>>> -#if defined(CONFIG_MADVISE)
> > > >>>> if (qemu_ram_is_shared(rb) && rb->fd < 0) {
> > > >>>> - ret = madvise(host_startaddr, length, QEMU_MADV_REMOVE);
> > > >>>> + ret = qemu_madvise(host_startaddr, length, QEMU_MADV_REMOVE);
> > > >>>> } else {
> > > >>>> - ret = madvise(host_startaddr, length, QEMU_MADV_DONTNEED);
> > > >>>> + ret = qemu_madvise(host_startaddr, length, QEMU_MADV_DONTNEED);
> > > >>>
> > > >>> posix_madvise(QEMU_MADV_DONTNEED) has completely different semantics
> > > >>> then madvise() -- it's not a discard that we need here.
> > > >>>
> > > >>> So ram_block_discard_range() would now succeed in environments (BSD?)
> > > >>> where it's supposed to fail.
> > > >>>
> > > >>> So AFAIKs this isn't sane.
> > > >>
> > > >> But CONFIG_MADVISE just means "host has madvise()"; it doesn't imply
> > > >> "this is a Linux madvise() with MADV_DONTNEED". Solaris madvise()
> > > >> doesn't seem to have MADV_DONTNEED at all; a quick look at the
> > > >> FreeBSD manpage suggests its madvise MADV_DONTNEED is identical
> > > >> to its posix_madvise MADV_DONTNEED.
> > > >>
> > > >> If we need "specifically Linux MADV_DONTNEED semantics" maybe we
> > > >> should define a QEMU_MADV_LINUX_DONTNEED which either (a) does the
> > > >> right thing or (b) fails, and use qemu_madvise() regardless.
> > > >>
> > > >> Certainly the current code is pretty fragile to being changed by
> > > >> people who don't understand the undocumented subtlety behind
> > > >> the use of a direct madvise() call here.
> > > >
> > > > Yeh and I'm not sure I can remembe rall the subtleties; there's a big
> > > > hairy set of ifdef's in include/qemu/madvise.h that makes
> > > > sure we always have the definition of QEMU_MADV_REMOVE/DONTNEED
> > > > even on platforms that might not define it themselves.
> > > >
> > > > But I think this code is used for things with different degrees
> > > > of care about the semantics; e.g. 'balloon' just cares that
> > > > it frees memory up and doesn't care about the detailed semantics
> > > > that much; so it's probably fine with that.
> > > > Postcopy is much more touchy, but then it's only going to be
> > > > calling this on Linux anyway (because of the userfault dependency).
> > >
> > > MADV_DONTNEED/MADV_REMOVE only provides discard semantics on Linux IIRC
> > > -- and that's what we want to achieve: ram_block_discard_range()
> > >
> > > So I agree with Peter that we might want to make this more explicit.
> >
> > I was looking at the comments/history around this code to try to make
> > this more explicit/clear, and it seems like the whole function is very
> > Linux-specific. All we ever do is:
> >
> > - fallocate(FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE)
> > - madvise(MADV_REMOVE)
> > - madvise(MADV_DONTNEED) with Linux semantics
> >
> > All of those operations are Linux-only, so trying to figure out the
> > cross-platform way to model this seems kind of pointless. Is it fine to
> > just #ifdef the whole thing to be just for Linux?
>
> For ballooning we don't really need Linux semantics; we just need it to
> use less host memory. Postcopy needs the more careful semantics though.
Right, sorry, you mentioned that, but I was thinking it sounded like
that applied to the MADV_DONTNEED path, which is only 1 case of the 3.
But reading into this a bit, maybe all of these cases are fine on
non-Linux for ballooning: fallocate is never called ('rb->fd' is always
-1), and QEMU_MADV_REMOVE falls back to QEMU_MADV_DONTNEED, which is
fine for ballooning. Am I understanding that correctly?
But regardless, it's simpler/more-conservative to make it all
Linux-specific. If I just take ram_block_discard_range away from
non-Linux (that is, make it always return an error), is that breaking
actual functionality, or is it removing a niche code path that non-Linux
isn't really using anyway, and it's not worth the time to figure out? I
am not familiar with qemu internals, so this is not obvious to me.
For context: I'm just trying to get the tree to compile on other
platforms (immediate focus is the guest agent on Solaris). The madvise()
calls here generate warnings due to platform-specific quirks that
qemu_madvise() has some logic to deal with. So my question is whether
to adapt these to the cross-platform qemu_advise(), or treat the
function as platform-specific code.
--
Andrew Deason
adeason@sinenomine.net
On Wed, 16 Mar 2022 10:41:41 +0100
David Hildenbrand <david@redhat.com> wrote:
> On 16.03.22 10:37, Dr. David Alan Gilbert wrote:
> > * Peter Maydell (peter.maydell@linaro.org) wrote:
> >> On Wed, 16 Mar 2022 at 07:53, David Hildenbrand <david@redhat.com> wrote:
> >>>
> >>> On 16.03.22 05:04, Andrew Deason wrote:
> >>>> We have a thin wrapper around madvise, called qemu_madvise, which
> >>>> provides consistent behavior for the !CONFIG_MADVISE case, and works
> >>>> around some platform-specific quirks (some platforms only provide
> >>>> posix_madvise, and some don't offer all 'advise' types). This specific
> >>>> caller of madvise has never used it, tracing back to its original
> >>>> introduction in commit e0b266f01dd2 ("migration_completion: Take
> >>>> current state").
> >>>>
> >>>> Call qemu_madvise here, to follow the same logic as all of our other
> >>>> madvise callers. This slightly changes the behavior for
> >>>> !CONFIG_MADVISE (EINVAL instead of ENOSYS, and a slightly different
> >>>> error message), but this is now more consistent with other callers
> >>>> that use qemu_madvise.
> >>>>
> >>>> Signed-off-by: Andrew Deason <adeason@sinenomine.net>
> >>>> ---
> >>>> Looking at the history of commits that touch this madvise() call, it
> >>>> doesn't _look_ like there's any reason to be directly calling madvise vs
> >>>> qemu_advise (I don't see anything mentioned), but I'm not sure.
> >>>>
> >>>> softmmu/physmem.c | 12 ++----------
> >>>> 1 file changed, 2 insertions(+), 10 deletions(-)
> >>>>
> >>>> diff --git a/softmmu/physmem.c b/softmmu/physmem.c
> >>>> index 43ae70fbe2..900c692b5e 100644
> >>>> --- a/softmmu/physmem.c
> >>>> +++ b/softmmu/physmem.c
> >>>> @@ -3584,40 +3584,32 @@ int ram_block_discard_range(RAMBlock *rb, uint64_t start, size_t length)
> >>>> rb->idstr, start, length, ret);
> >>>> goto err;
> >>>> #endif
> >>>> }
> >>>> if (need_madvise) {
> >>>> /* For normal RAM this causes it to be unmapped,
> >>>> * for shared memory it causes the local mapping to disappear
> >>>> * and to fall back on the file contents (which we just
> >>>> * fallocate'd away).
> >>>> */
> >>>> -#if defined(CONFIG_MADVISE)
> >>>> if (qemu_ram_is_shared(rb) && rb->fd < 0) {
> >>>> - ret = madvise(host_startaddr, length, QEMU_MADV_REMOVE);
> >>>> + ret = qemu_madvise(host_startaddr, length, QEMU_MADV_REMOVE);
> >>>> } else {
> >>>> - ret = madvise(host_startaddr, length, QEMU_MADV_DONTNEED);
> >>>> + ret = qemu_madvise(host_startaddr, length, QEMU_MADV_DONTNEED);
> >>>
> >>> posix_madvise(QEMU_MADV_DONTNEED) has completely different semantics
> >>> then madvise() -- it's not a discard that we need here.
> >>>
> >>> So ram_block_discard_range() would now succeed in environments (BSD?)
> >>> where it's supposed to fail.
> >>>
> >>> So AFAIKs this isn't sane.
> >>
> >> But CONFIG_MADVISE just means "host has madvise()"; it doesn't imply
> >> "this is a Linux madvise() with MADV_DONTNEED". Solaris madvise()
> >> doesn't seem to have MADV_DONTNEED at all; a quick look at the
> >> FreeBSD manpage suggests its madvise MADV_DONTNEED is identical
> >> to its posix_madvise MADV_DONTNEED.
> >>
> >> If we need "specifically Linux MADV_DONTNEED semantics" maybe we
> >> should define a QEMU_MADV_LINUX_DONTNEED which either (a) does the
> >> right thing or (b) fails, and use qemu_madvise() regardless.
> >>
> >> Certainly the current code is pretty fragile to being changed by
> >> people who don't understand the undocumented subtlety behind
> >> the use of a direct madvise() call here.
> >
> > Yeh and I'm not sure I can remembe rall the subtleties; there's a big
> > hairy set of ifdef's in include/qemu/madvise.h that makes
> > sure we always have the definition of QEMU_MADV_REMOVE/DONTNEED
> > even on platforms that might not define it themselves.
> >
> > But I think this code is used for things with different degrees
> > of care about the semantics; e.g. 'balloon' just cares that
> > it frees memory up and doesn't care about the detailed semantics
> > that much; so it's probably fine with that.
> > Postcopy is much more touchy, but then it's only going to be
> > calling this on Linux anyway (because of the userfault dependency).
>
> MADV_DONTNEED/MADV_REMOVE only provides discard semantics on Linux IIRC
> -- and that's what we want to achieve: ram_block_discard_range()
>
> So I agree with Peter that we might want to make this more explicit.
Maybe this could use an argument like QEMU_MADV_DONTNEED_DISCARD, which
gets #define'd to QEMU_MADV_INVALID for posix_madvise, or based on a
configure-time test, or just all non-Linux to be safe? Regardless, it
means "MADV_DONTNEED with discard semantics".
Solaris does have MADV_DONTNEED (possibly new in Solaris 11; I haven't
checked the history), but no MADV_REMOVE. As of 11.4.42 CBE, madvise(3C) says:
MADV_DONTNEED
Tell the kernel that the specified address range is no longer
needed, so the system starts to free the resources associated with
the address range.
MADV_DONTNEED and MADV_FREE perform related but distinct opera-
tions. MADV_DONTNEED tries to move any data from the specified
address range out of memory, but it ensures that the contents of
that range will be recovered when they are next referenced.
MADV_FREE does not attempt to preserve the contents of the address
range. As a result, subsequent references to an address range that
received madvise (MADV_DONTNEED) are likely to be slower than ref-
erences to a range that received madvise (MADV_FREE).
Is there a small test program I could run to see if the semantics are
what we want? Or should we just assume only Linux works here; I assume
that's safer.
--
Andrew Deason
adeason@sinenomine.net
© 2016 - 2024 Red Hat, Inc.