* [PATCH] drivers: xen-blkback: delay pending_req allocation to connect_ring
@ 2015-05-26 0:06 Bob Liu
2015-06-01 8:36 ` Roger Pau Monné
0 siblings, 1 reply; 3+ messages in thread
From: Bob Liu @ 2015-05-26 0:06 UTC (permalink / raw)
To: xen-devel
Cc: david.vrabel, justing, konrad.wilk, roger.pau, paul.durrant,
julien.grall, boris.ostrovsky, linux-kernel, Bob Liu
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.
Signed-off-by: Bob Liu <bob.liu@oracle.com>
---
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
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;
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;
+ }
+
+ for (i = 0; i < nr_grefs * XEN_BLKIF_REQS; i++) {
+ req = kzalloc(sizeof(*req), GFP_KERNEL);
+ if (!req)
+ goto fail;
+ list_add_tail(&req->free_list, &be->blkif->pending_free);
+ for (j = 0; j < nr_segs; j++) {
+ req->segments[j] = kzalloc(sizeof(*req->segments[0]),
+ GFP_KERNEL);
+ if (!req->segments[j])
+ goto fail;
+ }
+ for (j = 0; j < nr_indiret_pages; j++) {
+ req->indirect_pages[j] = kzalloc(sizeof(*req->indirect_pages[0]),
+ GFP_KERNEL);
+ if (!req->indirect_pages[j])
+ goto fail;
+ }
+ }
+
/* Map the shared frame, irq etc. */
err = xen_blkif_map(be->blkif, ring_ref, nr_grefs, evtchn);
if (err) {
@@ -962,6 +948,23 @@ static int connect_ring(struct backend_info *be)
}
return 0;
+
+fail:
+ list_for_each_entry_safe(req, n, &be->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);
+ }
+ return -ENOMEM;
}
static const struct xenbus_device_id xen_blkbk_ids[] = {
--
1.8.3.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] drivers: xen-blkback: delay pending_req allocation to connect_ring
2015-05-26 0:06 [PATCH] drivers: xen-blkback: delay pending_req allocation to connect_ring Bob Liu
@ 2015-06-01 8:36 ` Roger Pau Monné
2015-06-01 9:00 ` Bob Liu
0 siblings, 1 reply; 3+ messages in thread
From: Roger Pau Monné @ 2015-06-01 8:36 UTC (permalink / raw)
To: Bob Liu, xen-devel
Cc: david.vrabel, justing, konrad.wilk, paul.durrant, julien.grall,
boris.ostrovsky, linux-kernel
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 <bob.liu@oracle.com>
> ---
> 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.
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] drivers: xen-blkback: delay pending_req allocation to connect_ring
2015-06-01 8:36 ` Roger Pau Monné
@ 2015-06-01 9:00 ` Bob Liu
0 siblings, 0 replies; 3+ messages in thread
From: Bob Liu @ 2015-06-01 9:00 UTC (permalink / raw)
To: Roger Pau Monné
Cc: xen-devel, david.vrabel, justing, konrad.wilk, paul.durrant,
julien.grall, boris.ostrovsky, linux-kernel
On 06/01/2015 04:36 PM, Roger Pau Monné wrote:
> 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?
>
I think it's unnecessary to send an new iteration if there isn't any new comments
about multi-page ring series.
This patch is meaningful only after multi-page ring series, else there is no
difference(no memory can be saved).
So I think it's fine for this patch coming after multi-page ring series.
>> Signed-off-by: Bob Liu <bob.liu@oracle.com>
>> ---
>> 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;
>
Sorry, I can't remember why I made the mistake here.
Will be fixed.
Thanks,
-Bob
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2015-06-01 9:01 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-05-26 0:06 [PATCH] drivers: xen-blkback: delay pending_req allocation to connect_ring Bob Liu
2015-06-01 8:36 ` Roger Pau Monné
2015-06-01 9:00 ` Bob Liu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).