From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43835) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xp08Z-0002XK-BG for qemu-devel@nongnu.org; Thu, 13 Nov 2014 14:33:05 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Xp08Q-0003o6-CF for qemu-devel@nongnu.org; Thu, 13 Nov 2014 14:32:59 -0500 Received: from userp1040.oracle.com ([156.151.31.81]:27047) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xp08Q-0003nx-4a for qemu-devel@nongnu.org; Thu, 13 Nov 2014 14:32:50 -0500 Date: Thu, 13 Nov 2014 14:32:32 -0500 From: Konrad Rzeszutek Wilk Message-ID: <20141113193232.GB12502@laptop.dumpdata.com> References: <1415900529-10629-1-git-send-email-roger.pau@citrix.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <1415900529-10629-1-git-send-email-roger.pau@citrix.com> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v3 for-xen-4.5] xen_disk: fix unmapping of persistent grants List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Roger Pau Monne Cc: Kevin Wolf , Stefano Stabellini , George Dunlap , qemu-devel@nongnu.org, Stefan Hajnoczi , xen-devel@lists.xenproject.org On Thu, Nov 13, 2014 at 06:42:09PM +0100, Roger Pau Monne wrote: > This patch fixes two issues with persistent grants and the disk PV back= end > (Qdisk): >=20 > - Keep track of memory regions where persistent grants have been mappe= d > since we need to unmap them as a whole. It is not possible to unmap = a > single grant if it has been batch-mapped. A new check has also been = added > to make sure persistent grants are only used if the whole mapped reg= ion > can be persistently mapped in the batch_maps case. > - Unmap persistent grants before switching to the closed state, so the > frontend can also free them. >=20 > Signed-off-by: Roger Pau Monn=E9 > Reported-by: George Dunlap > Cc: Stefano Stabellini > Cc: Kevin Wolf > Cc: Stefan Hajnoczi > Cc: George Dunlap > Cc: Konrad Rzeszutek Wilk > --- > Xen release exception: this is obviously an important bug-fix that shou= ld be > included in 4.5. Without this fix, trying to do a block-detach of a Qdi= sk > from a running guest can lead to guest crash depending on the blkfront > implementation. Release-Acked-by: Konrad Rzeszutek Wilk on the spirit of having it in Xen 4.5 - but I am going to let Stefano decide if he would like to take this version or another one (if he spots something else that needs fixing). Thank you! > --- > Changea since v2: > - Expand the commit message to mention the newly added check. > - Don't use persistent_regions in the !batch_maps case, we can use the > previous implementation which works fine in that case. >=20 > Changes since v1: > - Instead of disabling batch mappings when using persistent grants, ke= ep > track of the persistently mapped regions and unmap them on disconnec= tion. > This prevents unmapping single persistent grants, but the current > implementation doesn't require it. >=20 > This new version is slightly faster than the previous one, the followin= g > test results are in IOPS from 20 runs of a block-attach, fio run, > block-detach test loop: >=20 > x batch > + no_batch > +----------------------------------------------------------------------= + > | + + x x + + + x xx x = | > |+ + x x+ *+++ * +x* +++x* x xx x *= | > | |_____________A_____M______|= | > | |________________A_____M___________| = | > +----------------------------------------------------------------------= + > N Min Max Median Avg St= ddev > x 20 52941 63822 62396 61180.15 2673.= 6497 > + 20 49967 63849 60322 59116.9 3456.= 3455 > Difference at 95.0% confidence > -2063.25 +/- 1977.66 > -3.37242% +/- 3.23252% > (Student's t, pooled s =3D 3089.88) > --- > hw/block/xen_disk.c | 72 +++++++++++++++++++++++++++++++++++++++++++++= +++----- > 1 file changed, 66 insertions(+), 6 deletions(-) >=20 > diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c > index 231e9a7..21842a0 100644 > --- a/hw/block/xen_disk.c > +++ b/hw/block/xen_disk.c > @@ -59,6 +59,13 @@ struct PersistentGrant { > =20 > typedef struct PersistentGrant PersistentGrant; > =20 > +struct PersistentRegion { > + void *addr; > + int num; > +}; > + > +typedef struct PersistentRegion PersistentRegion; > + > struct ioreq { > blkif_request_t req; > int16_t status; > @@ -118,6 +125,7 @@ struct XenBlkDev { > gboolean feature_discard; > gboolean feature_persistent; > GTree *persistent_gnts; > + GSList *persistent_regions; > unsigned int persistent_gnt_count; > unsigned int max_grants; > =20 > @@ -177,6 +185,23 @@ static void destroy_grant(gpointer pgnt) > g_free(grant); > } > =20 > +static void remove_persistent_region(gpointer data, gpointer dev) > +{ > + PersistentRegion *region =3D data; > + struct XenBlkDev *blkdev =3D dev; > + XenGnttab gnt =3D blkdev->xendev.gnttabdev; > + > + if (xc_gnttab_munmap(gnt, region->addr, region->num) !=3D 0) { > + xen_be_printf(&blkdev->xendev, 0, > + "xc_gnttab_munmap region %p failed: %s\n", > + region->addr, strerror(errno)); > + } > + xen_be_printf(&blkdev->xendev, 3, > + "unmapped grant region %p with %d pages\n", > + region->addr, region->num); > + g_free(region); > +} > + > static struct ioreq *ioreq_start(struct XenBlkDev *blkdev) > { > struct ioreq *ioreq =3D NULL; > @@ -343,6 +368,7 @@ static int ioreq_map(struct ioreq *ioreq) > void *page[BLKIF_MAX_SEGMENTS_PER_REQUEST]; > int i, j, new_maps =3D 0; > PersistentGrant *grant; > + PersistentRegion *region; > /* domids and refs variables will contain the information necessar= y > * to map the grants that are needed to fulfill this request. > * > @@ -421,7 +447,22 @@ static int ioreq_map(struct ioreq *ioreq) > } > } > } > - if (ioreq->blkdev->feature_persistent) { > + if (ioreq->blkdev->feature_persistent && new_maps !=3D 0 && > + (!batch_maps || (ioreq->blkdev->persistent_gnt_count + new_map= s <=3D > + ioreq->blkdev->max_grants))) { > + /* > + * If we are using persistent grants and batch mappings only > + * add the new maps to the list of persistent grants if the wh= ole > + * area can be persistently mapped. > + */ > + if (batch_maps) { > + region =3D g_malloc0(sizeof(*region)); > + region->addr =3D ioreq->pages; > + region->num =3D new_maps; > + ioreq->blkdev->persistent_regions =3D g_slist_append( > + ioreq->blkdev->persistent_= regions, > + region); > + } > while ((ioreq->blkdev->persistent_gnt_count < ioreq->blkdev->m= ax_grants) > && new_maps) { > /* Go through the list of newly mapped grants and add as m= any > @@ -447,6 +488,7 @@ static int ioreq_map(struct ioreq *ioreq) > grant); > ioreq->blkdev->persistent_gnt_count++; > } > + assert(!batch_maps || new_maps =3D=3D 0); > } > for (i =3D 0; i < ioreq->v.niov; i++) { > ioreq->v.iov[i].iov_base +=3D (uintptr_t)page[i]; > @@ -971,7 +1013,10 @@ static int blk_connect(struct XenDevice *xendev) > blkdev->max_grants =3D max_requests * BLKIF_MAX_SEGMENTS_PER_R= EQUEST; > blkdev->persistent_gnts =3D g_tree_new_full((GCompareDataFunc)= int_cmp, > NULL, NULL, > + batch_maps ? > + (GDestroyNotify)g_free : > (GDestroyNotify)destroy_g= rant); > + blkdev->persistent_regions =3D NULL; > blkdev->persistent_gnt_count =3D 0; > } > =20 > @@ -1000,6 +1045,26 @@ static void blk_disconnect(struct XenDevice *xen= dev) > blkdev->cnt_map--; > blkdev->sring =3D NULL; > } > + > + /* > + * Unmap persistent grants before switching to the closed state > + * so the frontend can free them. > + * > + * In the !batch_maps case g_tree_destroy will take care of unmapp= ing > + * the grant, but in the batch_maps case we need to iterate over e= very > + * region in persistent_regions and unmap it. > + */ > + if (blkdev->feature_persistent) { > + g_tree_destroy(blkdev->persistent_gnts); > + assert(batch_maps || blkdev->persistent_gnt_count =3D=3D 0); > + if (batch_maps) { > + blkdev->persistent_gnt_count =3D 0; > + g_slist_foreach(blkdev->persistent_regions, > + (GFunc)remove_persistent_region, blkdev); > + g_slist_free(blkdev->persistent_regions); > + } > + blkdev->feature_persistent =3D false; > + } > } > =20 > static int blk_free(struct XenDevice *xendev) > @@ -1011,11 +1076,6 @@ static int blk_free(struct XenDevice *xendev) > blk_disconnect(xendev); > } > =20 > - /* Free persistent grants */ > - if (blkdev->feature_persistent) { > - g_tree_destroy(blkdev->persistent_gnts); > - } > - > while (!QLIST_EMPTY(&blkdev->freelist)) { > ioreq =3D QLIST_FIRST(&blkdev->freelist); > QLIST_REMOVE(ioreq, list); > --=20 > 1.9.3 (Apple Git-50) >=20