Re: [Qemu-devel] frequently ballooning results in qemu exit

[Stefano Stabellini <stefano.stabellini@eu.citrix.com>]

[-- Attachment #1: Type: text/plain, Size: 6176 bytes --]

On Sat, 30 Mar 2013, Hanweidong wrote:
> > -----Original Message-----
> > From: Stefano Stabellini [mailto:stefano.stabellini@eu.citrix.com]
> > Sent: 2013年3月29日 20:37
> > To: Hanweidong
> > Cc: Tim (Xen.org); George Dunlap; Andrew Cooper; Yanqiangjun; qemu-
> > devel@nongnu.org; xen-devel@lists.xen.org; Gonglei (Arei); Anthony
> > Perard; Wangzhenguo
> > Subject: Re: [Qemu-devel] [Xen-devel] frequently ballooning results in
> > qemu exit
> > 
> > On Mon, 25 Mar 2013, Hanweidong wrote:
> > > We fixed this issue by below patch which computed the correct size
> > for test_bits(). qemu_get_ram_ptr() and qemu_safe_ram_ptr() will call
> > xen_map_cache() with size is 0 if the requested address is in the RAM.
> > Then xen_map_cache() will pass the size 0 to test_bits() for checking
> > if the corresponding pfn was mapped in cache. But test_bits() will
> > always return 1 when size is 0 without any bit testing. Actually, for
> > this case, test_bits should check one bit. So this patch introduced a
> > __test_bit_size which is greater than 0 and a multiple of XC_PAGE_SIZE,
> > then test_bits can work correctly with __test_bit_size >> XC_PAGE_SHIFT
> > as its size.
> > >
> > > Signed-off-by: Zhenguo Wang <wangzhenguo@huawei.com>
> > > Signed-off-by: Weidong Han <hanweidong@huawei.com>
> > 
> > Thanks for the patch and for debugging this difficult problem.
> > The reality is that size is never actually 0: when qemu_get_ram_ptr
> > calls xen_map_cache with size 0, it actually means "map until the end
> > of
> > the page". As a consequence test_bits should always test at least 1 bit,
> > like you wrote.
> 
> Yes, for the case of size is 0, we can just simply set __test_bit_size 1. But for size > 0, I think set __test_bit_size to size >> XC_PAGE_SHIFT is incorrect. If size is not multiple of XC_PAGE_SIZE, then the part of (size % XC_PAGE_SIZE) also needs to test 1 bit. For example size is XC_PAGE_SIZE + 1, then it needs to test 2 bits, but size >> XC_PAGE_SHIFT is only 1. 
> 

I was assuming that the size is always page aligned.
Looking through the code actually I think that it's better not to rely
on this assumption.

Looking back at your original patch:



> We fixed this issue by below patch which computed the correct size for test_bits(). qemu_get_ram_ptr() and qemu_safe_ram_ptr() will call xen_map_cache() with size is 0 if the requested address is in the RAM. Then xen_map_cache() will pass the size 0 to test_bits() for checking if the corresponding pfn was mapped in cache. But test_bits() will always return 1 when size is 0 without any bit testing. Actually, for this case, test_bits should check one bit. So this patch introduced a __test_bit_size which is greater than 0 and a multiple of XC_PAGE_SIZE, then test_bits can work correctly with __test_bit_size >> XC_PAGE_SHIFT as its size.
> 
> Signed-off-by: Zhenguo Wang <wangzhenguo@huawei.com>
> Signed-off-by: Weidong Han <hanweidong@huawei.com>
> 
> diff --git a/xen-mapcache.c b/xen-mapcache.c
> index 31c06dc..bd4a97f 100644
> --- a/xen-mapcache.c
> +++ b/xen-mapcache.c
> @@ -202,6 +202,7 @@ uint8_t *xen_map_cache(hwaddr phys_addr, hwaddr size,
>      hwaddr address_index;
>      hwaddr address_offset;
>      hwaddr __size = size;
> +    hwaddr __test_bit_size = size;
>      bool translated = false;
> 
>  tryagain:
> @@ -210,7 +211,23 @@ tryagain:
> 
>      trace_xen_map_cache(phys_addr);
> 
> -    if (address_index == mapcache->last_address_index && !lock && !__size) {
> +    entry = &mapcache->entry[address_index % mapcache->nr_buckets];

there is no need to move this line up here, see below


> +    /* __test_bit_size is always a multiple of XC_PAGE_SIZE */
> +    if (size) {
> +        __test_bit_size = size + (phys_addr & (XC_PAGE_SIZE - 1));
> +
> +        if (__test_bit_size % XC_PAGE_SIZE) {
> +            __test_bit_size += XC_PAGE_SIZE - (__test_bit_size % XC_PAGE_SIZE);
> +        }
> +    } else {
> +        __test_bit_size = XC_PAGE_SIZE;
> +    }

this is OK


> +    if (address_index == mapcache->last_address_index && !lock && !__size &&
> +        test_bits(address_offset >> XC_PAGE_SHIFT,
> +                  __test_bit_size >> XC_PAGE_SHIFT,
> +                  entry->valid_mapping)) {
>          trace_xen_map_cache_return(mapcache->last_address_vaddr + address_offset);
>          return mapcache->last_address_vaddr + address_offset;
>      }

Unless I am missing something this change is unnecessary: if the mapping
is not valid than mapcache->last_address_index is set to -1.


> @@ -225,11 +242,10 @@ tryagain:
>          __size = MCACHE_BUCKET_SIZE;
>      }
> 
> -    entry = &mapcache->entry[address_index % mapcache->nr_buckets];
> -

just leave entry where it is


>      while (entry && entry->lock && entry->vaddr_base &&
>              (entry->paddr_index != address_index || entry->size != __size ||
> -             !test_bits(address_offset >> XC_PAGE_SHIFT, size >> XC_PAGE_SHIFT,
> +             !test_bits(address_offset >> XC_PAGE_SHIFT,
> +                 __test_bit_size >> XC_PAGE_SHIFT,
>                   entry->valid_mapping))) {
>          pentry = entry;
>          entry = entry->next;
> @@ -241,13 +257,15 @@ tryagain:
>      } else if (!entry->lock) {
>          if (!entry->vaddr_base || entry->paddr_index != address_index ||
>                  entry->size != __size ||
> -                !test_bits(address_offset >> XC_PAGE_SHIFT, size >> XC_PAGE_SHIFT,
> +                !test_bits(address_offset >> XC_PAGE_SHIFT,
> +                    __test_bit_size >> XC_PAGE_SHIFT,
>                      entry->valid_mapping)) {
>              xen_remap_bucket(entry, __size, address_index);
>          }
>      }
> 
> -    if(!test_bits(address_offset >> XC_PAGE_SHIFT, size >> XC_PAGE_SHIFT,
> +    if(!test_bits(address_offset >> XC_PAGE_SHIFT,
> +                __test_bit_size >> XC_PAGE_SHIFT,
>                  entry->valid_mapping)) {
>          mapcache->last_address_index = -1;
>          if (!translated && mapcache->phys_offset_to_gaddr) {

This is fine

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel