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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
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) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
> At 01:34 +1000 on 04 Jun (1307151275), James Harper wrote: > > I''m revisiting the problem where xp hangs on the first hibernationafter> > a boot. When the hibernate hangs for a while, strace -T -p showsaround> > 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 pauseonly> > occurs on the first hibernate, never on subsequent hibernates (egafter> > 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 hitbut> > 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 sawrecently> while trying to boot Xen inside Xen. Large memcpys that are handledby> qemu seem to end up wwith a map and unmap for every byte of a REPMOVSB.> > AIUI the logic in the mapcache is something like: > - Each bucket contains a number of ''locked'' mappings (which aren''tused> 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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
> AIUI the logic in the mapcache is something like: > - Each bucket contains a number of ''locked'' mappings (which aren''tused> 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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
> > > 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 toyour> > 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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
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_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
> > 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 seeso> >> far, the bit representing the pfn in entry->valid_mapping is 0because> >> err[] returned for that pfn was -EINVAL. > >> > >> Maybe the test is superfluous? Is there a need to do the remap ifall> >> the other variables in the expression are satisfied? If the remapwas> >> already done and the page could not be mapped last time, whatreasons> >> are there why it would succeed this time? > >> > > > > FWIW, removing the test_bit makes the hibernate go faster than myscreen> > 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_bucketwith> > identical parameters could successfully map a page that couldn''t be > > mapped in the previous call, are there any optimisations that couldbe> > done? Maybe only attempt to map the page being accessed rather thanall> > pages in the bucket if the other parameters are identical? > > I''m guessing this happens because of frequent guest CPU access tonon-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 pfnunder> 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 testre-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 somethingreally 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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
> > 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 testand> > sets last_address_index to 0 and returns NULL. Is this just toensure> > that the next access isn''t just trivially accepted? > > Yes, first test is on a potentially stale bucket. Second test is on afresh> 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); } _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
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); > } >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
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); > } >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
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. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
> > The exit condition is what would happen anyway after the remap isdone> > 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 validnow> > 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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
> > 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''mjust> 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)) { _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
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. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
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)) > { >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
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(); _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
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.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
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. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel