From mboxrd@z Thu Jan 1 00:00:00 1970 From: Keir Fraser Subject: Re: slow xp hibernation revisited Date: Sat, 04 Jun 2011 09:30:38 +0100 Message-ID: References: Mime-Version: 1.0 Content-Type: text/plain; charset="US-ASCII" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: James Harper , Tim Deegan Cc: xen-devel@lists.xensource.com, Ian Jackson , Stefano Stabellini List-Id: xen-devel@lists.xenproject.org On 04/06/2011 09:05, "James Harper" wrote: >> >> On 04/06/2011 08:38, "James Harper" > 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); > } >