Xen project Mailing List

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

On Sat, 30 Mar 2013, Hanweidong wrote: > > -----Original Message----- > > From: Stefano Stabellini [mailto:stefano.stabellini@xxxxxxxxxxxxx] > > Sent: 2013å3æ29æ 20:37 > > To: Hanweidong > > Cc: Tim (Xen.org); George Dunlap; Andrew Cooper; Yanqiangjun; qemu- > > devel@xxxxxxxxxx; xen-devel@xxxxxxxxxxxxx; 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@xxxxxxxxxx> > > > Signed-off-by: Weidong Han <hanweidong@xxxxxxxxxx> > > > > 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@xxxxxxxxxx> > Signed-off-by: Weidong Han <hanweidong@xxxxxxxxxx> > > 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

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

©2013 Xen Project, A Linux Foundation Collaborative Project. All Rights Reserved.
Linux Foundation is a registered trademark of The Linux Foundation.
Xen Project is a trademark of The Linux Foundation.