From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:40582) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UMftX-0002Hp-8s for qemu-devel@nongnu.org; Mon, 01 Apr 2013 10:39:38 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UMftR-0001lT-7g for qemu-devel@nongnu.org; Mon, 01 Apr 2013 10:39:35 -0400 Received: from smtp.citrix.com ([66.165.176.89]:45247) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UMftQ-0001jL-UY for qemu-devel@nongnu.org; Mon, 01 Apr 2013 10:39:29 -0400 Date: Mon, 1 Apr 2013 15:39:02 +0100 From: Stefano Stabellini In-Reply-To: Message-ID: References: <5141A8B0.4050305@citrix.com> <20130314143403.GB5174@ocelot.phlegethon.org> <20130321121547.GB12338@ocelot.phlegethon.org> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="1342847746-937608982-1364826389=:5078" Content-ID: Subject: Re: [Qemu-devel] [Xen-devel] frequently ballooning results in qemu exit List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Hanweidong Cc: Stefano Stabellini , George Dunlap , Andrew Cooper , Yanqiangjun , "Tim (Xen.org)" , "qemu-devel@nongnu.org" , "xen-devel@lists.xen.org" , "Gonglei (Arei)" , Anthony Perard , Wangzhenguo --1342847746-937608982-1364826389=:5078 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: QUOTED-PRINTABLE Content-ID: On Sat, 30 Mar 2013, Hanweidong wrote: > > -----Original Message----- > > From: Stefano Stabellini [mailto:stefano.stabellini@eu.citrix.com] > > Sent: 2013=E5=B9=B43=E6=9C=8829=E6=97=A5 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 > >=20 > > 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 > > > Signed-off-by: Weidong Han > >=20 > > 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. >=20 > 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.=20 >=20 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 te= st_bits(). qemu_get_ram_ptr() and qemu_safe_ram_ptr() will call xen_map_cac= he() with size is 0 if the requested address is in the RAM. Then xen_map_ca= che() 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. >=20 > Signed-off-by: Zhenguo Wang > Signed-off-by: Weidong Han >=20 > 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 =3D size; > + hwaddr __test_bit_size =3D size; > bool translated =3D false; >=20 > tryagain: > @@ -210,7 +211,23 @@ tryagain: >=20 > trace_xen_map_cache(phys_addr); >=20 > - if (address_index =3D=3D mapcache->last_address_index && !lock && !_= _size) { > + entry =3D &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 =3D size + (phys_addr & (XC_PAGE_SIZE - 1)); > + > + if (__test_bit_size % XC_PAGE_SIZE) { > + __test_bit_size +=3D XC_PAGE_SIZE - (__test_bit_size % XC_PA= GE_SIZE); > + } > + } else { > + __test_bit_size =3D XC_PAGE_SIZE; > + } this is OK > + if (address_index =3D=3D 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 + addres= s_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 =3D MCACHE_BUCKET_SIZE; > } >=20 > - entry =3D &mapcache->entry[address_index % mapcache->nr_buckets]; > - just leave entry where it is > while (entry && entry->lock && entry->vaddr_base && > (entry->paddr_index !=3D address_index || entry->size !=3D _= _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 =3D entry; > entry =3D entry->next; > @@ -241,13 +257,15 @@ tryagain: > } else if (!entry->lock) { > if (!entry->vaddr_base || entry->paddr_index !=3D address_index = || > entry->size !=3D __size || > - !test_bits(address_offset >> XC_PAGE_SHIFT, size >> XC_P= AGE_SHIFT, > + !test_bits(address_offset >> XC_PAGE_SHIFT, > + __test_bit_size >> XC_PAGE_SHIFT, > entry->valid_mapping)) { > xen_remap_bucket(entry, __size, address_index); > } > } >=20 > - 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 =3D -1; > if (!translated && mapcache->phys_offset_to_gaddr) { This is fine --1342847746-937608982-1364826389=:5078--