Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752831AbbFAIgg (ORCPT ); Mon, 1 Jun 2015 04:36:36 -0400 Received: from smtp.citrix.com ([66.165.176.89]:16235 "EHLO SMTP.CITRIX.COM" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752121AbbFAIga (ORCPT ); Mon, 1 Jun 2015 04:36:30 -0400 X-IronPort-AV: E=Sophos;i="5.13,531,1427760000"; d="scan'208";a="267707253" Message-ID: <556C1984.2030705@citrix.com> Date: Mon, 1 Jun 2015 10:36:20 +0200 From: =?windows-1252?Q?Roger_Pau_Monn=E9?= User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:31.0) Gecko/20100101 Thunderbird/31.7.0 MIME-Version: 1.0 To: Bob Liu , CC: , , , , , , Subject: Re: [PATCH] drivers: xen-blkback: delay pending_req allocation to connect_ring References: <1432598764-21546-1-git-send-email-bob.liu@oracle.com> In-Reply-To: <1432598764-21546-1-git-send-email-bob.liu@oracle.com> Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit X-DLP: MIA1 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5740 Lines: 166 El 26/05/15 a les 2.06, Bob Liu ha escrit: > In connect_ring, we can know exactly how many pages are used for the shared > ring and also whether feature-persistent is enabled, delay pending_req > allocation here so that we won't waste too much memory. I would very much prefer for this to be a pre-patch for your multipage ring series. Do you think you can include it in the next iteration? > Signed-off-by: Bob Liu > --- > drivers/block/xen-blkback/common.h | 3 +- > drivers/block/xen-blkback/xenbus.c | 95 ++++++++++++++++++++------------------ > 2 files changed, 51 insertions(+), 47 deletions(-) > > diff --git a/drivers/block/xen-blkback/common.h b/drivers/block/xen-blkback/common.h > index 919a1ab..e1d605d 100644 > --- a/drivers/block/xen-blkback/common.h > +++ b/drivers/block/xen-blkback/common.h > @@ -249,7 +249,7 @@ struct backend_info; > #define PERSISTENT_GNT_WAS_ACTIVE 1 > > /* Number of requests that we can fit in a ring */ > -#define XEN_MAX_BLKIF_REQS (32 * XENBUS_MAX_RING_PAGES) > +#define XEN_BLKIF_REQS 32 This should be XEN_BLKIF_REQS_PER_PAGE (or a similar name of your choice that reflects that those are the number of requests per ring page). > > struct persistent_gnt { > struct page *page; > @@ -321,6 +321,7 @@ struct xen_blkif { > struct work_struct free_work; > /* Thread shutdown wait queue. */ > wait_queue_head_t shutdown_wq; > + unsigned int nr_ring_pages; > }; > > struct seg_buf { > diff --git a/drivers/block/xen-blkback/xenbus.c b/drivers/block/xen-blkback/xenbus.c > index bc33888..48336a3 100644 > --- a/drivers/block/xen-blkback/xenbus.c > +++ b/drivers/block/xen-blkback/xenbus.c > @@ -125,8 +125,6 @@ static void xen_update_blkif_status(struct xen_blkif *blkif) > static struct xen_blkif *xen_blkif_alloc(domid_t domid) > { > struct xen_blkif *blkif; > - struct pending_req *req, *n; > - int i, j; > > BUILD_BUG_ON(MAX_INDIRECT_PAGES > BLKIF_MAX_INDIRECT_PAGES_PER_REQUEST); > > @@ -153,50 +151,11 @@ static struct xen_blkif *xen_blkif_alloc(domid_t domid) > INIT_LIST_HEAD(&blkif->pending_free); > INIT_WORK(&blkif->free_work, xen_blkif_deferred_free); > > - for (i = 0; i < XEN_MAX_BLKIF_REQS; i++) { > - req = kzalloc(sizeof(*req), GFP_KERNEL); > - if (!req) > - goto fail; > - list_add_tail(&req->free_list, > - &blkif->pending_free); > - for (j = 0; j < MAX_INDIRECT_SEGMENTS; j++) { > - req->segments[j] = kzalloc(sizeof(*req->segments[0]), > - GFP_KERNEL); > - if (!req->segments[j]) > - goto fail; > - } > - for (j = 0; j < MAX_INDIRECT_PAGES; j++) { > - req->indirect_pages[j] = kzalloc(sizeof(*req->indirect_pages[0]), > - GFP_KERNEL); > - if (!req->indirect_pages[j]) > - goto fail; > - } > - } > spin_lock_init(&blkif->pending_free_lock); > init_waitqueue_head(&blkif->pending_free_wq); > init_waitqueue_head(&blkif->shutdown_wq); > > return blkif; > - > -fail: > - list_for_each_entry_safe(req, n, &blkif->pending_free, free_list) { > - list_del(&req->free_list); > - for (j = 0; j < MAX_INDIRECT_SEGMENTS; j++) { > - if (!req->segments[j]) > - break; > - kfree(req->segments[j]); > - } > - for (j = 0; j < MAX_INDIRECT_PAGES; j++) { > - if (!req->indirect_pages[j]) > - break; > - kfree(req->indirect_pages[j]); > - } > - kfree(req); > - } > - > - kmem_cache_free(xen_blkif_cachep, blkif); > - > - return ERR_PTR(-ENOMEM); > } > > static int xen_blkif_map(struct xen_blkif *blkif, grant_ref_t *gref, > @@ -313,7 +272,7 @@ static void xen_blkif_free(struct xen_blkif *blkif) > i++; > } > > - WARN_ON(i != XEN_MAX_BLKIF_REQS); > + WARN_ON(i != XEN_BLKIF_REQS * blkif->nr_ring_pages); > > kmem_cache_free(xen_blkif_cachep, blkif); > } > @@ -868,9 +827,10 @@ static int connect_ring(struct backend_info *be) > struct xenbus_device *dev = be->dev; > unsigned int ring_ref[XENBUS_MAX_RING_PAGES]; > unsigned int evtchn, nr_grefs, ring_page_order; > - unsigned int pers_grants; > + unsigned int pers_grants, i, j; > + struct pending_req *req, *n; > char protocol[64] = ""; > - int err; > + int err, nr_indiret_pages, nr_segs; ^ nr_indirect_pages (notice the missing 'c'). > > pr_debug("%s %s\n", __func__, dev->otherend); > > @@ -899,8 +859,6 @@ static int connect_ring(struct backend_info *be) > pr_info("%s:using single page: ring-ref %d\n", dev->otherend, > ring_ref[0]); > } else { > - unsigned int i; > - > if (ring_page_order > xen_blkif_max_ring_order) { > err = -EINVAL; > xenbus_dev_fatal(dev, err, "%s/request %d ring page order exceed max:%d", > @@ -949,11 +907,39 @@ static int connect_ring(struct backend_info *be) > > be->blkif->vbd.feature_gnt_persistent = pers_grants; > be->blkif->vbd.overflow_max_grants = 0; > + be->blkif->nr_ring_pages = nr_grefs; > > pr_info("ring-pages:%d, event-channel %d, protocol %d (%s) %s\n", > nr_grefs, evtchn, be->blkif->blk_protocol, protocol, > pers_grants ? "persistent grants" : ""); > > + if (!pers_grants) { > + nr_indiret_pages = 0; > + nr_segs = BLKIF_MAX_SEGMENTS_PER_REQUEST; > + } else { > + nr_indiret_pages = MAX_INDIRECT_PAGES; > + nr_segs = MAX_INDIRECT_SEGMENTS; > + } This is not right, persistent grants and indirect descriptors are features that can be used independently, so AFAICT this has to be: nr_indiret_pages = MAX_INDIRECT_PAGES; nr_segs = MAX_INDIRECT_SEGMENTS; Roger. -- 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/