slow xp hibernation revisited

slow xp hibernation revisited

[James Harper]

I'm revisiting the problem where xp hangs on the first hibernation after
a boot. When the hibernate hangs for a while, strace -T -p shows around
600/second of:

mmap(NULL, 1048576, PROT_READ|PROT_WRITE, MAP_SHARED, 6, 0) =
0x7fb9cfa38000 <0.000036>
ioctl(6, IOCTL_PRIVCMD_MMAPBATCH_V2, 0x7fff2c8b0f20) = -1 EINVAL
(Invalid argument) <0.000027>
ioctl(6, IOCTL_PRIVCMD_MMAPBATCH, 0x7fff2c8b0f40) = 0 <0.002878>
munmap(0x7fb9cfa38000, 1048576) = 0 <0.000111>

Nothing like that is seen during normal execution, and the pause only
occurs on the first hibernate, never on subsequent hibernates (eg after
resume then hibernate again) until the DomU is rebooted. Working
backwards, those ioctl's appear to be called in libxc from
xc_map_foreign_xxx, but I'm getting a bit lost from there. Any
suggestions on how to track down what is causing this? Originally I
thought it might have been PoD memory causing the performance hit but
this DomU is fully populated aside from a few hundred kb.

Thanks

James

Re: slow xp hibernation revisited

[Tim Deegan]

At 01:34 +1000 on 04 Jun (1307151275), James Harper wrote:
> I'm revisiting the problem where xp hangs on the first hibernation after
> a boot. When the hibernate hangs for a while, strace -T -p shows around
> 600/second of:
> 
> mmap(NULL, 1048576, PROT_READ|PROT_WRITE, MAP_SHARED, 6, 0) =
> 0x7fb9cfa38000 <0.000036>
> ioctl(6, IOCTL_PRIVCMD_MMAPBATCH_V2, 0x7fff2c8b0f20) = -1 EINVAL
> (Invalid argument) <0.000027>
> ioctl(6, IOCTL_PRIVCMD_MMAPBATCH, 0x7fff2c8b0f40) = 0 <0.002878>
> munmap(0x7fb9cfa38000, 1048576) = 0 <0.000111>
> 
> Nothing like that is seen during normal execution, and the pause only
> occurs on the first hibernate, never on subsequent hibernates (eg after
> resume then hibernate again) until the DomU is rebooted. Working
> backwards, those ioctl's appear to be called in libxc from
> xc_map_foreign_xxx, but I'm getting a bit lost from there. Any
> suggestions on how to track down what is causing this? Originally I
> thought it might have been PoD memory causing the performance hit but
> this DomU is fully populated aside from a few hundred kb.

I think this is a bug in the qemu-dm mapcache code, which I saw recently
while trying to boot Xen inside Xen.  Large memcpys that are handled by
qemu seem to end up wwith a map and unmap for every byte of a REP MOVSB.

AIUI the logic in the mapcache is something like:
 - Each bucket contains a number of 'locked' mappings (which aren't used
   for this kind of copy).
 - At the bottom of each bucket is a possible 'unlocked' mapping. 
 - If the unlocked mapping matches the address you want, reuse it
 - Else discard it and replace it with a new unlocked mapping to your 
   target area.

But something is going on and the "else" clause is happening every
time. 

Unfortunately that's as far as I got before I needed to work on
something else. :( 

Tim.

-- 
Tim Deegan <Tim.Deegan@citrix.com>
Principal Software Engineer, Xen Platform Team
Citrix Systems UK Ltd.  (Company #02937203, SL9 0BG)

RE: slow xp hibernation revisited

[James Harper]

> At 01:34 +1000 on 04 Jun (1307151275), James Harper wrote:
> > I'm revisiting the problem where xp hangs on the first hibernation
after
> > a boot. When the hibernate hangs for a while, strace -T -p shows
around
> > 600/second of:
> >
> > mmap(NULL, 1048576, PROT_READ|PROT_WRITE, MAP_SHARED, 6, 0) =
> > 0x7fb9cfa38000 <0.000036>
> > ioctl(6, IOCTL_PRIVCMD_MMAPBATCH_V2, 0x7fff2c8b0f20) = -1 EINVAL
> > (Invalid argument) <0.000027>
> > ioctl(6, IOCTL_PRIVCMD_MMAPBATCH, 0x7fff2c8b0f40) = 0 <0.002878>
> > munmap(0x7fb9cfa38000, 1048576) = 0 <0.000111>
> >
> > Nothing like that is seen during normal execution, and the pause
only
> > occurs on the first hibernate, never on subsequent hibernates (eg
after
> > resume then hibernate again) until the DomU is rebooted. Working
> > backwards, those ioctl's appear to be called in libxc from
> > xc_map_foreign_xxx, but I'm getting a bit lost from there. Any
> > suggestions on how to track down what is causing this? Originally I
> > thought it might have been PoD memory causing the performance hit
but
> > this DomU is fully populated aside from a few hundred kb.
> 
> I think this is a bug in the qemu-dm mapcache code, which I saw
recently
> while trying to boot Xen inside Xen.  Large memcpys that are handled
by
> qemu seem to end up wwith a map and unmap for every byte of a REP
MOVSB.
> 
> AIUI the logic in the mapcache is something like:
>  - Each bucket contains a number of 'locked' mappings (which aren't
used
>    for this kind of copy).
>  - At the bottom of each bucket is a possible 'unlocked' mapping.
>  - If the unlocked mapping matches the address you want, reuse it
>  - Else discard it and replace it with a new unlocked mapping to your
>    target area.
> 
> But something is going on and the "else" clause is happening every
> time.
> 
> Unfortunately that's as far as I got before I needed to work on
> something else. :(
> 

Can you take a guess at what DomU behaviour would trigger the above?

Thanks

James

RE: slow xp hibernation revisited

[James Harper]

> AIUI the logic in the mapcache is something like:
>  - Each bucket contains a number of 'locked' mappings (which aren't
used
>    for this kind of copy).
>  - At the bottom of each bucket is a possible 'unlocked' mapping.
>  - If the unlocked mapping matches the address you want, reuse it
>  - Else discard it and replace it with a new unlocked mapping to your
>    target area.
> 
> But something is going on and the "else" clause is happening every
> time.
> 

It's the !test_bit(address_offset>>XC_PAGE_SHIFT, entry->valid_mapping)
that is causing the if expression to be true. From what I can see so
far, the bit representing the pfn in entry->valid_mapping is 0 because
err[] returned for that pfn was -EINVAL.

Maybe the test is superfluous? Is there a need to do the remap if all
the other variables in the expression are satisfied? If the remap was
already done and the page could not be mapped last time, what reasons
are there why it would succeed this time?

James

RE: slow xp hibernation revisited

[James Harper]

> 
> > AIUI the logic in the mapcache is something like:
> >  - Each bucket contains a number of 'locked' mappings (which aren't
> used
> >    for this kind of copy).
> >  - At the bottom of each bucket is a possible 'unlocked' mapping.
> >  - If the unlocked mapping matches the address you want, reuse it
> >  - Else discard it and replace it with a new unlocked mapping to
your
> >    target area.
> >
> > But something is going on and the "else" clause is happening every
> > time.
> >
> 
> It's the !test_bit(address_offset>>XC_PAGE_SHIFT,
entry->valid_mapping)
> that is causing the if expression to be true. From what I can see so
> far, the bit representing the pfn in entry->valid_mapping is 0 because
> err[] returned for that pfn was -EINVAL.
> 
> Maybe the test is superfluous? Is there a need to do the remap if all
> the other variables in the expression are satisfied? If the remap was
> already done and the page could not be mapped last time, what reasons
> are there why it would succeed this time?
> 

FWIW, removing the test_bit makes the hibernate go faster than my screen
can refresh over a slow DSL connection and in a quick 30 second test
doesn't appear to have any adverse effects.

If there is a chance that a subsequent call to qemu_remap_bucket with
identical parameters could successfully map a page that couldn't be
mapped in the previous call, are there any optimisations that could be
done? Maybe only attempt to map the page being accessed rather than all
pages in the bucket if the other parameters are identical?

James

Re: slow xp hibernation revisited

[Keir Fraser]

On 04/06/2011 05:54, "James Harper" <james.harper@bendigoit.com.au> wrote:

>> It's the !test_bit(address_offset>>XC_PAGE_SHIFT,
> entry->valid_mapping)
>> that is causing the if expression to be true. From what I can see so
>> far, the bit representing the pfn in entry->valid_mapping is 0 because
>> err[] returned for that pfn was -EINVAL.
>> 
>> Maybe the test is superfluous? Is there a need to do the remap if all
>> the other variables in the expression are satisfied? If the remap was
>> already done and the page could not be mapped last time, what reasons
>> are there why it would succeed this time?
>> 
> 
> FWIW, removing the test_bit makes the hibernate go faster than my screen
> can refresh over a slow DSL connection and in a quick 30 second test
> doesn't appear to have any adverse effects.
> 
> If there is a chance that a subsequent call to qemu_remap_bucket with
> identical parameters could successfully map a page that couldn't be
> mapped in the previous call, are there any optimisations that could be
> done? Maybe only attempt to map the page being accessed rather than all
> pages in the bucket if the other parameters are identical?

I'm guessing this happens because of frequent guest CPU access to non-RAM
during hibernate? Unfortunately really the qemu checks do make sense, I'd
say, since the memory map of the guest can be changed dynamically , and we
currently only flush the map_cache on XENMEM_decrease_reservation
hypercalls.

One fix would be for Xen to know which regions of non-RAM are actually
emulated device areas, and only forward those to qemu. It could then
quick-fail on the rest.

However, the easiest fix would be to only re-try to map the one pfn under
test. Reloading a whole bucket takes bloody ages as they are *huge*: 256kB
in 32-bit qemu; 4MB in 64-bit qemu. It might be easiest to do a test re-map
of the one page to a scratch area, then iff it succeeds, *then* call
qemu_remap_bucket(). Then you remap the bucket only if something really has
changed, and you don't have to mess too much with modifying the bucket
yourself outside of remap_bucket.

How does that sound?

 -- Keir


> James
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

RE: slow xp hibernation revisited

[James Harper]

> 
> On 04/06/2011 05:54, "James Harper" <james.harper@bendigoit.com.au>
wrote:
> 
> >> It's the !test_bit(address_offset>>XC_PAGE_SHIFT,
> > entry->valid_mapping)
> >> that is causing the if expression to be true. From what I can see
so
> >> far, the bit representing the pfn in entry->valid_mapping is 0
because
> >> err[] returned for that pfn was -EINVAL.
> >>
> >> Maybe the test is superfluous? Is there a need to do the remap if
all
> >> the other variables in the expression are satisfied? If the remap
was
> >> already done and the page could not be mapped last time, what
reasons
> >> are there why it would succeed this time?
> >>
> >
> > FWIW, removing the test_bit makes the hibernate go faster than my
screen
> > can refresh over a slow DSL connection and in a quick 30 second test
> > doesn't appear to have any adverse effects.
> >
> > If there is a chance that a subsequent call to qemu_remap_bucket
with
> > identical parameters could successfully map a page that couldn't be
> > mapped in the previous call, are there any optimisations that could
be
> > done? Maybe only attempt to map the page being accessed rather than
all
> > pages in the bucket if the other parameters are identical?
> 
> I'm guessing this happens because of frequent guest CPU access to
non-RAM
> during hibernate? Unfortunately really the qemu checks do make sense,
I'd
> say, since the memory map of the guest can be changed dynamically ,
and we
> currently only flush the map_cache on XENMEM_decrease_reservation
> hypercalls.
> 
> One fix would be for Xen to know which regions of non-RAM are actually
> emulated device areas, and only forward those to qemu. It could then
> quick-fail on the rest.
> 
> However, the easiest fix would be to only re-try to map the one pfn
under
> test. Reloading a whole bucket takes bloody ages as they are *huge*:
256kB
> in 32-bit qemu; 4MB in 64-bit qemu. It might be easiest to do a test
re-map
> of the one page to a scratch area, then iff it succeeds, *then* call
> qemu_remap_bucket(). Then you remap the bucket only if something
really has
> changed, and you don't have to mess too much with modifying the bucket
> yourself outside of remap_bucket.
> 
> How does that sound?
> 

Sounds like a plan. Is there no way to detect the changes to the memory
map of the guest?

Looking past the test_bit call, the next statement does another test and
sets last_address_index to 0 and returns NULL. Is this just to ensure
that the next access isn't just trivially accepted?

James

Re: slow xp hibernation revisited

[Keir Fraser]

On 04/06/2011 08:38, "James Harper" <james.harper@bendigoit.com.au> wrote:

> Looking past the test_bit call, the next statement does another test and
> sets last_address_index to 0 and returns NULL. Is this just to ensure
> that the next access isn't just trivially accepted?

Yes, first test is on a potentially stale bucket. Second test is on a fresh
bucket.

 -- Keir

RE: slow xp hibernation revisited

[James Harper]

> 
> On 04/06/2011 08:38, "James Harper" <james.harper@bendigoit.com.au>
wrote:
> 
> > Looking past the test_bit call, the next statement does another test
and
> > sets last_address_index to 0 and returns NULL. Is this just to
ensure
> > that the next access isn't just trivially accepted?
> 
> Yes, first test is on a potentially stale bucket. Second test is on a
fresh
> bucket.
> 

How about the following patch? Is munmap the correct way to unmap or is
an IOCTL required too?

The exit condition is what would happen anyway after the remap is done
and the page is still invalid.

diff --git a/hw/xen_machine_fv.c b/hw/xen_machine_fv.c
index d02e23f..1ff80bb 100644
--- a/hw/xen_machine_fv.c
+++ b/hw/xen_machine_fv.c
@@ -151,6 +151,24 @@ uint8_t *qemu_map_cache(target_phys_addr_t
phys_addr, uint8_t lock)
         pentry->next = entry;
         qemu_remap_bucket(entry, address_index);
     } else if (!entry->lock) {
+        if (entry->vaddr_base && entry->paddr_index == address_index &&
!test_bit(address_offset>>XC_PAGE_SHIFT, entry->valid_mapping))
+        {
+            /* The page was invalid previously. Test if it is valid now
and only remap if so */
+            xen_pfn_t pfn;
+            int err;
+            void *tmp_vaddr;
+
+            pfn = phys_addr >> XC_PAGE_SHIFT;
+            tmp_vaddr = xc_map_foreign_bulk(xc_handle, domid,
PROT_READ|PROT_WRITE, &pfn, &err, 1);
+            if (tmp_vaddr)
+                munmap(tmp_vaddr, PAGE_SIZE);
+
+            if (!tmp_vaddr || err)
+            {
+                last_address_index = ~0UL;
+                return NULL;
+            }
+        }
         if (!entry->vaddr_base || entry->paddr_index != address_index
|| !test_bit(address_offset>>XC_PAGE_SHIFT, entry->valid_mapping))
             qemu_remap_bucket(entry, address_index);
     }

Re: slow xp hibernation revisited

[Keir Fraser]

On 04/06/2011 09:05, "James Harper" <james.harper@bendigoit.com.au> wrote:

>> 
>> On 04/06/2011 08:38, "James Harper" <james.harper@bendigoit.com.au>
> wrote:
>> 
>>> Looking past the test_bit call, the next statement does another test
> and
>>> sets last_address_index to 0 and returns NULL. Is this just to
> ensure
>>> that the next access isn't just trivially accepted?
>> 
>> Yes, first test is on a potentially stale bucket. Second test is on a
> fresh
>> bucket.
>> 
> 
> How about the following patch? Is munmap the correct way to unmap or is
> an IOCTL required too?
> 
> The exit condition is what would happen anyway after the remap is done
> and the page is still invalid.

Looks fine to me, although maybe the function needs refactoring a little. Cc
Stefano and Ian who are more involved in qemu maintenance.

Also, looking at qemu_map_cache() now, the early exit at the top of the
function looks a bit bogus to me. It exits successfully if we hit the same
address_index as last invocation, even though we might be hitting a
different pfn within the indexed range, and a possibly invalid/unmapped pfn
at that.

 -- Keir

> diff --git a/hw/xen_machine_fv.c b/hw/xen_machine_fv.c
> index d02e23f..1ff80bb 100644
> --- a/hw/xen_machine_fv.c
> +++ b/hw/xen_machine_fv.c
> @@ -151,6 +151,24 @@ uint8_t *qemu_map_cache(target_phys_addr_t
> phys_addr, uint8_t lock)
>          pentry->next = entry;
>          qemu_remap_bucket(entry, address_index);
>      } else if (!entry->lock) {
> +        if (entry->vaddr_base && entry->paddr_index == address_index &&
> !test_bit(address_offset>>XC_PAGE_SHIFT, entry->valid_mapping))
> +        {
> +            /* The page was invalid previously. Test if it is valid now
> and only remap if so */
> +            xen_pfn_t pfn;
> +            int err;
> +            void *tmp_vaddr;
> +
> +            pfn = phys_addr >> XC_PAGE_SHIFT;
> +            tmp_vaddr = xc_map_foreign_bulk(xc_handle, domid,
> PROT_READ|PROT_WRITE, &pfn, &err, 1);
> +            if (tmp_vaddr)
> +                munmap(tmp_vaddr, PAGE_SIZE);
> +
> +            if (!tmp_vaddr || err)
> +            {
> +                last_address_index = ~0UL;
> +                return NULL;
> +            }
> +        }
>          if (!entry->vaddr_base || entry->paddr_index != address_index
> || !test_bit(address_offset>>XC_PAGE_SHIFT, entry->valid_mapping))
>              qemu_remap_bucket(entry, address_index);
>      }
> 

Re: slow xp hibernation revisited

[Keir Fraser]

On 04/06/2011 09:05, "James Harper" <james.harper@bendigoit.com.au> wrote:

>> 
>> On 04/06/2011 08:38, "James Harper" <james.harper@bendigoit.com.au>
> wrote:
>> 
>>> Looking past the test_bit call, the next statement does another test
> and
>>> sets last_address_index to 0 and returns NULL. Is this just to
> ensure
>>> that the next access isn't just trivially accepted?
>> 
>> Yes, first test is on a potentially stale bucket. Second test is on a
> fresh
>> bucket.
>> 
> 
> How about the following patch? Is munmap the correct way to unmap or is
> an IOCTL required too?

By the way, depending on how this patch performs for you, another
alternative I thought of would be to fail this function if the address
passed in is the same as the address in a io-request we are currently
processing from Xen. After all, if Xen punted the memory access to qemu,
obviously there is no RAM there!

Could be an uglier patch than what you have below however, and maybe below
patch is good enough.

 -- Keir

> The exit condition is what would happen anyway after the remap is done
> and the page is still invalid.
> 
> diff --git a/hw/xen_machine_fv.c b/hw/xen_machine_fv.c
> index d02e23f..1ff80bb 100644
> --- a/hw/xen_machine_fv.c
> +++ b/hw/xen_machine_fv.c
> @@ -151,6 +151,24 @@ uint8_t *qemu_map_cache(target_phys_addr_t
> phys_addr, uint8_t lock)
>          pentry->next = entry;
>          qemu_remap_bucket(entry, address_index);
>      } else if (!entry->lock) {
> +        if (entry->vaddr_base && entry->paddr_index == address_index &&
> !test_bit(address_offset>>XC_PAGE_SHIFT, entry->valid_mapping))
> +        {
> +            /* The page was invalid previously. Test if it is valid now
> and only remap if so */
> +            xen_pfn_t pfn;
> +            int err;
> +            void *tmp_vaddr;
> +
> +            pfn = phys_addr >> XC_PAGE_SHIFT;
> +            tmp_vaddr = xc_map_foreign_bulk(xc_handle, domid,
> PROT_READ|PROT_WRITE, &pfn, &err, 1);
> +            if (tmp_vaddr)
> +                munmap(tmp_vaddr, PAGE_SIZE);
> +
> +            if (!tmp_vaddr || err)
> +            {
> +                last_address_index = ~0UL;
> +                return NULL;
> +            }
> +        }
>          if (!entry->vaddr_base || entry->paddr_index != address_index
> || !test_bit(address_offset>>XC_PAGE_SHIFT, entry->valid_mapping))
>              qemu_remap_bucket(entry, address_index);
>      }
> 

RE: slow xp hibernation revisited

[Stefano Stabellini]

On Sat, 4 Jun 2011, James Harper wrote:
> > 
> > On 04/06/2011 08:38, "James Harper" <james.harper@bendigoit.com.au>
> wrote:
> > 
> > > Looking past the test_bit call, the next statement does another test
> and
> > > sets last_address_index to 0 and returns NULL. Is this just to
> ensure
> > > that the next access isn't just trivially accepted?
> > 
> > Yes, first test is on a potentially stale bucket. Second test is on a
> fresh
> > bucket.
> > 
> 
> How about the following patch? Is munmap the correct way to unmap or is
> an IOCTL required too?
> 

munmap is OK


> The exit condition is what would happen anyway after the remap is done
> and the page is still invalid.
> 
> diff --git a/hw/xen_machine_fv.c b/hw/xen_machine_fv.c
> index d02e23f..1ff80bb 100644
> --- a/hw/xen_machine_fv.c
> +++ b/hw/xen_machine_fv.c
> @@ -151,6 +151,24 @@ uint8_t *qemu_map_cache(target_phys_addr_t
> phys_addr, uint8_t lock)
>          pentry->next = entry;
>          qemu_remap_bucket(entry, address_index);
>      } else if (!entry->lock) {
> +        if (entry->vaddr_base && entry->paddr_index == address_index &&
> !test_bit(address_offset>>XC_PAGE_SHIFT, entry->valid_mapping))
> +        {
> +            /* The page was invalid previously. Test if it is valid now
> and only remap if so */
> +            xen_pfn_t pfn;
> +            int err;
> +            void *tmp_vaddr;
> +
> +            pfn = phys_addr >> XC_PAGE_SHIFT;
> +            tmp_vaddr = xc_map_foreign_bulk(xc_handle, domid,
> PROT_READ|PROT_WRITE, &pfn, &err, 1);
> +            if (tmp_vaddr)
> +                munmap(tmp_vaddr, PAGE_SIZE);
> +
> +            if (!tmp_vaddr || err)
> +            {
> +                last_address_index = ~0UL;
> +                return NULL;
> +            }
> +        }
>          if (!entry->vaddr_base || entry->paddr_index != address_index
> || !test_bit(address_offset>>XC_PAGE_SHIFT, entry->valid_mapping))
>              qemu_remap_bucket(entry, address_index);
>      }

This is just a matter of aesthetic, but probably something like the
following would be clearer?

if (entry->vaddr_base && entry->paddr_index == address_index) {
    if (!test_bit(address_offset>>XC_PAGE_SHIFT, entry->valid_mapping)) {
            /* your code + remap bucket */
    }
} else {
    qemu_remap_bucket(entry, address_index);
}


Could you also please send a similar patch for upstream qemu?
The code should be quite similar in this area, look at xen-mapcache.c.

RE: slow xp hibernation revisited

[James Harper]

> > The exit condition is what would happen anyway after the remap is
done
> > and the page is still invalid.
> >
> > diff --git a/hw/xen_machine_fv.c b/hw/xen_machine_fv.c
> > index d02e23f..1ff80bb 100644
> > --- a/hw/xen_machine_fv.c
> > +++ b/hw/xen_machine_fv.c
> > @@ -151,6 +151,24 @@ uint8_t *qemu_map_cache(target_phys_addr_t
> > phys_addr, uint8_t lock)
> >          pentry->next = entry;
> >          qemu_remap_bucket(entry, address_index);
> >      } else if (!entry->lock) {
> > +        if (entry->vaddr_base && entry->paddr_index ==
address_index &&
> > !test_bit(address_offset>>XC_PAGE_SHIFT, entry->valid_mapping))
> > +        {
> > +            /* The page was invalid previously. Test if it is valid
now
> > and only remap if so */
> > +            xen_pfn_t pfn;
> > +            int err;
> > +            void *tmp_vaddr;
> > +
> > +            pfn = phys_addr >> XC_PAGE_SHIFT;
> > +            tmp_vaddr = xc_map_foreign_bulk(xc_handle, domid,
> > PROT_READ|PROT_WRITE, &pfn, &err, 1);
> > +            if (tmp_vaddr)
> > +                munmap(tmp_vaddr, PAGE_SIZE);
> > +
> > +            if (!tmp_vaddr || err)
> > +            {
> > +                last_address_index = ~0UL;
> > +                return NULL;
> > +            }
> > +        }
> >          if (!entry->vaddr_base || entry->paddr_index !=
address_index
> > || !test_bit(address_offset>>XC_PAGE_SHIFT, entry->valid_mapping))
> >              qemu_remap_bucket(entry, address_index);
> >      }
> 
> This is just a matter of aesthetic, but probably something like the
> following would be clearer?
> 
> if (entry->vaddr_base && entry->paddr_index == address_index) {
>     if (!test_bit(address_offset>>XC_PAGE_SHIFT,
entry->valid_mapping)) {
>             /* your code + remap bucket */
>     }
> } else {
>     qemu_remap_bucket(entry, address_index);
> }
> 

Yes that should work, although I think I'm too tired now for Boolean
algebra :)

Is my calculation of the pfn correct? I think I don't need to fuss
around with decoding from the bucket index and bucket offset if I'm just
doing a trial map of one page, so using phys_addr directory is correct
right?

James

RE: slow xp hibernation revisited

[James Harper]

> > This is just a matter of aesthetic, but probably something like the
> > following would be clearer?
> >
> > if (entry->vaddr_base && entry->paddr_index == address_index) {
> >     if (!test_bit(address_offset>>XC_PAGE_SHIFT,
> entry->valid_mapping)) {
> >             /* your code + remap bucket */
> >     }
> > } else {
> >     qemu_remap_bucket(entry, address_index);
> > }
> >
> 
> Yes that should work, although I think I'm too tired now for Boolean
> algebra :)
> 
> Is my calculation of the pfn correct? I think I don't need to fuss
> around with decoding from the bucket index and bucket offset if I'm
just
> doing a trial map of one page, so using phys_addr directory is correct
> right?
> 

Is this what you had in mind?

diff --git a/hw/xen_machine_fv.c b/hw/xen_machine_fv.c
index d02e23f..3c9fc0f 100644
--- a/hw/xen_machine_fv.c
+++ b/hw/xen_machine_fv.c
@@ -151,8 +151,30 @@ uint8_t *qemu_map_cache(target_phys_addr_t
phys_addr, uint8_t lock)
         pentry->next = entry;
         qemu_remap_bucket(entry, address_index);
     } else if (!entry->lock) {
-        if (!entry->vaddr_base || entry->paddr_index != address_index
|| !test_bit(address_offset>>XC_PAGE_SHIFT, entry->valid_mapping))
+        if (entry->vaddr_base && entry->paddr_index == address_index)
+        {
+            if (!test_bit(address_offset>>XC_PAGE_SHIFT,
entry->valid_mapping))
+            {
+                /* The page was invalid previously. Test if it is valid
now and only remap if so */
+                xen_pfn_t pfn;
+                int err;
+                void *tmp_vaddr;
+
+                pfn = phys_addr >> XC_PAGE_SHIFT;
+                tmp_vaddr = xc_map_foreign_bulk(xc_handle, domid,
PROT_READ|PROT_WRITE, &pfn, &err, 1);
+                if (tmp_vaddr)
+                    munmap(tmp_vaddr, PAGE_SIZE);
+
+                if (!tmp_vaddr || err)
+                {
+                    last_address_index = ~0UL;
+                    return NULL;
+                }
+                qemu_remap_bucket(entry, address_index);
+            }
+        } else {
             qemu_remap_bucket(entry, address_index);
+        }
     }

     if (!test_bit(address_offset>>XC_PAGE_SHIFT, entry->valid_mapping))
{

RE: slow xp hibernation revisited

[Stefano Stabellini]

On Mon, 6 Jun 2011, James Harper wrote:
> Is my calculation of the pfn correct? I think I don't need to fuss
> around with decoding from the bucket index and bucket offset if I'm just
> doing a trial map of one page, so using phys_addr directory is correct
> right?
 
Yes, I think is correct.

RE: slow xp hibernation revisited

[Stefano Stabellini]

On Mon, 6 Jun 2011, James Harper wrote:
> > > This is just a matter of aesthetic, but probably something like the
> > > following would be clearer?
> > >
> > > if (entry->vaddr_base && entry->paddr_index == address_index) {
> > >     if (!test_bit(address_offset>>XC_PAGE_SHIFT,
> > entry->valid_mapping)) {
> > >             /* your code + remap bucket */
> > >     }
> > > } else {
> > >     qemu_remap_bucket(entry, address_index);
> > > }
> > >
> > 
> > Yes that should work, although I think I'm too tired now for Boolean
> > algebra :)
> > 
> > Is my calculation of the pfn correct? I think I don't need to fuss
> > around with decoding from the bucket index and bucket offset if I'm
> just
> > doing a trial map of one page, so using phys_addr directory is correct
> > right?
> > 
> 
> Is this what you had in mind?
> 

Yes, it is fine for me.
Could you please send another version for upstream?
Use this git tree, just to be sure you have the latest mapcache fixes:

git://xenbits.xen.org/people/sstabellini/qemu-dm.git stable



> diff --git a/hw/xen_machine_fv.c b/hw/xen_machine_fv.c
> index d02e23f..3c9fc0f 100644
> --- a/hw/xen_machine_fv.c
> +++ b/hw/xen_machine_fv.c
> @@ -151,8 +151,30 @@ uint8_t *qemu_map_cache(target_phys_addr_t
> phys_addr, uint8_t lock)
>          pentry->next = entry;
>          qemu_remap_bucket(entry, address_index);
>      } else if (!entry->lock) {
> -        if (!entry->vaddr_base || entry->paddr_index != address_index
> || !test_bit(address_offset>>XC_PAGE_SHIFT, entry->valid_mapping))
> +        if (entry->vaddr_base && entry->paddr_index == address_index)
> +        {
> +            if (!test_bit(address_offset>>XC_PAGE_SHIFT,
> entry->valid_mapping))
> +            {
> +                /* The page was invalid previously. Test if it is valid
> now and only remap if so */
> +                xen_pfn_t pfn;
> +                int err;
> +                void *tmp_vaddr;
> +
> +                pfn = phys_addr >> XC_PAGE_SHIFT;
> +                tmp_vaddr = xc_map_foreign_bulk(xc_handle, domid,
> PROT_READ|PROT_WRITE, &pfn, &err, 1);
> +                if (tmp_vaddr)
> +                    munmap(tmp_vaddr, PAGE_SIZE);
> +
> +                if (!tmp_vaddr || err)
> +                {
> +                    last_address_index = ~0UL;
> +                    return NULL;
> +                }
> +                qemu_remap_bucket(entry, address_index);
> +            }
> +        } else {
>              qemu_remap_bucket(entry, address_index);
> +        }
>      }
> 
>      if (!test_bit(address_offset>>XC_PAGE_SHIFT, entry->valid_mapping))
> {
> 

Re: slow xp hibernation revisited

[Stefano Stabellini]

On Sat, 4 Jun 2011, Keir Fraser wrote:
> Also, looking at qemu_map_cache() now, the early exit at the top of the
> function looks a bit bogus to me. It exits successfully if we hit the same
> address_index as last invocation, even though we might be hitting a
> different pfn within the indexed range, and a possibly invalid/unmapped pfn
> at that.
> 

Yes, I am afraid that you are right.
The mistake is memorizing the last address_index rather than
the last page address.
I'll submit a similar patch to upstream qemu.

---

mapcache: remember the last page address rather then the last address_index

A single address_index corresponds to multiple pages that might or
might not be mapped.
It is better to just remember the last page address for the sake of this
optimization, so that we are sure that it is mapped.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>


diff --git a/hw/xen_machine_fv.c b/hw/xen_machine_fv.c
index a353ee6..603a508 100644
--- a/hw/xen_machine_fv.c
+++ b/hw/xen_machine_fv.c
@@ -63,7 +63,7 @@ static unsigned long nr_buckets;
 TAILQ_HEAD(map_cache_head, map_cache_rev) locked_entries = TAILQ_HEAD_INITIALIZER(locked_entries);
 
 /* For most cases (>99.9%), the page address is the same. */
-static unsigned long last_address_index = ~0UL;
+static unsigned long last_address_page = ~0UL;
 static uint8_t      *last_address_vaddr;
 
 static int qemu_map_cache_init(void)
@@ -138,7 +138,7 @@ uint8_t *qemu_map_cache(target_phys_addr_t phys_addr, uint8_t lock)
     unsigned long address_index  = phys_addr >> MCACHE_BUCKET_SHIFT;
     unsigned long address_offset = phys_addr & (MCACHE_BUCKET_SIZE-1);
 
-    if (address_index == last_address_index && !lock)
+    if ((phys_addr >> XC_PAGE_SHIFT) == last_address_page && !lock)
         return last_address_vaddr + address_offset;
 
     entry = &mapcache_entry[address_index % nr_buckets];
@@ -157,17 +157,17 @@ uint8_t *qemu_map_cache(target_phys_addr_t phys_addr, uint8_t lock)
     }
 
     if (!test_bit(address_offset>>XC_PAGE_SHIFT, entry->valid_mapping)) {
-        last_address_index = ~0UL;
+        last_address_page = ~0UL;
         return NULL;
     }
 
-    last_address_index = address_index;
+    last_address_page = phys_addr >> XC_PAGE_SHIFT;
     last_address_vaddr = entry->vaddr_base;
     if (lock) {
         struct map_cache_rev *reventry = qemu_mallocz(sizeof(struct map_cache_rev));
         entry->lock++;
         reventry->vaddr_req = last_address_vaddr + address_offset;
-        reventry->paddr_index = last_address_index;
+        reventry->paddr_index = address_index;
         TAILQ_INSERT_TAIL(&locked_entries, reventry, next);
     }
 
@@ -182,7 +182,7 @@ void qemu_invalidate_entry(uint8_t *buffer)
     int found = 0;
     
     if (last_address_vaddr == buffer)
-        last_address_index =  ~0UL;
+        last_address_page =  ~0UL;
 
     TAILQ_FOREACH(reventry, &locked_entries, next) {
         if (reventry->vaddr_req == buffer) {
@@ -252,7 +252,7 @@ void qemu_invalidate_map_cache(void)
         entry->vaddr_base  = NULL;
     }
 
-    last_address_index =  ~0UL;
+    last_address_page =  ~0UL;
     last_address_vaddr = NULL;
 
     mapcache_unlock();

Re: slow xp hibernation revisited

[Ian Jackson]

Stefano Stabellini writes ("Re: [Xen-devel] slow xp hibernation revisited"):
> mapcache: remember the last page address rather then the last address_index

Thanks, I have applied this.  It should go into 4.1 too I guess ?

Ian.

Re: slow xp hibernation revisited

[Stefano Stabellini]

On Tue, 21 Jun 2011, Ian Jackson wrote:
> Stefano Stabellini writes ("Re: [Xen-devel] slow xp hibernation revisited"):
> > mapcache: remember the last page address rather then the last address_index
> 
> Thanks, I have applied this.  It should go into 4.1 too I guess ?
 
Yes indeed.