Re: slow xp hibernation revisited

[Keir Fraser <keir@xen.org>]

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);
>      }
>