Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753936AbaA0QTw (ORCPT ); Mon, 27 Jan 2014 11:19:52 -0500 Received: from smtp02.citrix.com ([66.165.176.63]:25239 "EHLO SMTP02.CITRIX.COM" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753366AbaA0QTv (ORCPT ); Mon, 27 Jan 2014 11:19:51 -0500 X-IronPort-AV: E=Sophos;i="4.95,729,1384300800"; d="scan'208";a="94860365" Message-ID: <52E6871D.2050107@citrix.com> Date: Mon, 27 Jan 2014 17:19:41 +0100 From: =?ISO-8859-1?Q?Roger_Pau_Monn=E9?= User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:24.0) Gecko/20100101 Thunderbird/24.2.0 MIME-Version: 1.0 To: Konrad Rzeszutek Wilk CC: , , David Vrabel , Boris Ostrovsky , Matt Rushton , Matt Wilson , Ian Campbell Subject: Re: [PATCH] xen-blkback: fix memory leaks References: <1390817621-12031-1-git-send-email-roger.pau@citrix.com> <20140127160914.GA23059@phenom.dumpdata.com> In-Reply-To: <20140127160914.GA23059@phenom.dumpdata.com> X-Enigmail-Version: 1.6 Content-Type: text/plain; charset="ISO-8859-1" Content-Transfer-Encoding: 8bit X-DLP: MIA2 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 27/01/14 17:09, Konrad Rzeszutek Wilk wrote: > On Mon, Jan 27, 2014 at 11:13:41AM +0100, Roger Pau Monne wrote: >> I've at least identified two possible memory leaks in blkback, both >> related to the shutdown path of a VBD: >> >> - We don't wait for any pending purge work to finish before cleaning >> the list of free_pages. The purge work will call put_free_pages and >> thus we might end up with pages being added to the free_pages list >> after we have emptied it. >> - We don't wait for pending requests to end before cleaning persistent >> grants and the list of free_pages. Again this can add pages to the >> free_pages lists or persistent grants to the persistent_gnts >> red-black tree. >> >> Also, add some checks in xen_blkif_free to make sure we are cleaning >> everything. >> >> Signed-off-by: Roger Pau Monn? >> Cc: Konrad Rzeszutek Wilk >> Cc: David Vrabel >> Cc: Boris Ostrovsky >> Cc: Matt Rushton >> Cc: Matt Wilson >> Cc: Ian Campbell >> --- >> This should be applied after the patch: >> >> xen-blkback: fix memory leak when persistent grants are used > > Could you respin the series with the issues below fixed and > have said patch as part of the series. That way not only does > it have your SoB on it but it makes it easier to apply the patch > for lazy^H^H^Hbusy maintainers and makes it clear that you had > tested both of them. > > Also, please CC Jens Axboe on these patches. Ack, will do once the comments below are sorted out. >> >> >From Matt Rushton & Matt Wilson and backported to stable. >> >> I've been able to create and destroy ~4000 guests while doing heavy IO >> operations with this patch on a 512M Dom0 without problems. >> --- >> drivers/block/xen-blkback/blkback.c | 29 +++++++++++++++++++---------- >> drivers/block/xen-blkback/xenbus.c | 9 +++++++++ >> 2 files changed, 28 insertions(+), 10 deletions(-) >> >> diff --git a/drivers/block/xen-blkback/blkback.c b/drivers/block/xen-blkback/blkback.c >> index 30ef7b3..19925b7 100644 >> --- a/drivers/block/xen-blkback/blkback.c >> +++ b/drivers/block/xen-blkback/blkback.c >> @@ -169,6 +169,7 @@ static int dispatch_rw_block_io(struct xen_blkif *blkif, >> struct pending_req *pending_req); >> static void make_response(struct xen_blkif *blkif, u64 id, >> unsigned short op, int st); >> +static void xen_blk_drain_io(struct xen_blkif *blkif, bool force); >> >> #define foreach_grant_safe(pos, n, rbtree, node) \ >> for ((pos) = container_of(rb_first((rbtree)), typeof(*(pos)), node), \ >> @@ -625,6 +626,12 @@ purge_gnt_list: >> print_stats(blkif); >> } >> >> + /* Drain pending IO */ >> + xen_blk_drain_io(blkif, true); >> + >> + /* Drain pending purge work */ >> + flush_work(&blkif->persistent_purge_work); >> + >> /* Free all persistent grant pages */ >> if (!RB_EMPTY_ROOT(&blkif->persistent_gnts)) >> free_persistent_gnts(blkif, &blkif->persistent_gnts, >> @@ -930,7 +937,7 @@ static int dispatch_other_io(struct xen_blkif *blkif, >> return -EIO; >> } >> >> -static void xen_blk_drain_io(struct xen_blkif *blkif) >> +static void xen_blk_drain_io(struct xen_blkif *blkif, bool force) >> { >> atomic_set(&blkif->drain, 1); >> do { >> @@ -943,7 +950,7 @@ static void xen_blk_drain_io(struct xen_blkif *blkif) >> >> if (!atomic_read(&blkif->drain)) >> break; >> - } while (!kthread_should_stop()); >> + } while (!kthread_should_stop() || force); >> atomic_set(&blkif->drain, 0); >> } >> >> @@ -976,17 +983,19 @@ static void __end_block_io_op(struct pending_req *pending_req, int error) >> * the proper response on the ring. >> */ >> if (atomic_dec_and_test(&pending_req->pendcnt)) { >> - xen_blkbk_unmap(pending_req->blkif, >> + struct xen_blkif *blkif = pending_req->blkif; >> + >> + xen_blkbk_unmap(blkif, >> pending_req->segments, >> pending_req->nr_pages); >> - make_response(pending_req->blkif, pending_req->id, >> + make_response(blkif, pending_req->id, >> pending_req->operation, pending_req->status); >> - xen_blkif_put(pending_req->blkif); >> - if (atomic_read(&pending_req->blkif->refcnt) <= 2) { >> - if (atomic_read(&pending_req->blkif->drain)) >> - complete(&pending_req->blkif->drain_complete); >> + free_req(blkif, pending_req); >> + xen_blkif_put(blkif); >> + if (atomic_read(&blkif->refcnt) <= 2) { >> + if (atomic_read(&blkif->drain)) >> + complete(&blkif->drain_complete); >> } >> - free_req(pending_req->blkif, pending_req); >> } >> } >> >> @@ -1224,7 +1233,7 @@ static int dispatch_rw_block_io(struct xen_blkif *blkif, >> * issue the WRITE_FLUSH. >> */ >> if (drain) >> - xen_blk_drain_io(pending_req->blkif); >> + xen_blk_drain_io(pending_req->blkif, false); >> >> /* >> * If we have failed at this point, we need to undo the M2P override, >> diff --git a/drivers/block/xen-blkback/xenbus.c b/drivers/block/xen-blkback/xenbus.c >> index c2014a0..3c10281 100644 >> --- a/drivers/block/xen-blkback/xenbus.c >> +++ b/drivers/block/xen-blkback/xenbus.c >> @@ -125,6 +125,7 @@ static struct xen_blkif *xen_blkif_alloc(domid_t domid) >> blkif->persistent_gnts.rb_node = NULL; >> spin_lock_init(&blkif->free_pages_lock); >> INIT_LIST_HEAD(&blkif->free_pages); >> + INIT_LIST_HEAD(&blkif->persistent_purge_list); > > Hm, See comment below. >> blkif->free_pages_num = 0; >> atomic_set(&blkif->persistent_gnt_in_use, 0); >> >> @@ -259,6 +260,14 @@ static void xen_blkif_free(struct xen_blkif *blkif) >> if (!atomic_dec_and_test(&blkif->refcnt)) >> BUG(); >> >> + /* Make sure everything is drained before shutting down */ >> + BUG_ON(blkif->persistent_gnt_c != 0); >> + BUG_ON(atomic_read(&blkif->persistent_gnt_in_use) != 0); >> + BUG_ON(blkif->free_pages_num != 0); >> + BUG_ON(!list_empty(&blkif->persistent_purge_list)); > > You don't seem to put anything on this list? Or even declare this? > Was there another patch in the series? No, the list is already used in current code, but it is initialized only before usage, now I need to make sure it's initialized even if not used, or: BUG_ON(!list_empty(&blkif->persistent_purge_list)); Is going to fail. I will resend this and replace the other (now useless) initialization with a BUG_ON(!list_empty... > >> + BUG_ON(!list_empty(&blkif->free_pages)); >> + BUG_ON(!RB_EMPTY_ROOT(&blkif->persistent_gnts)); >> + >> /* Check that there is no request in use */ >> list_for_each_entry_safe(req, n, &blkif->pending_free, free_list) { >> list_del(&req->free_list); >> -- >> 1.7.7.5 (Apple Git-26) >> -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/