From: Mike Snitzer <snitzer@redhat.com>
To: Vivek Goyal <vgoyal@redhat.com>
Cc: Jens Axboe <axboe@kernel.dk>,
kexec@lists.infradead.org, linux-kernel@vger.kernel.org,
linux-mm@kvack.org, David Rientjes <rientjes@google.com>,
Dave Young <dyoung@redhat.com>
Subject: [PATCH] block: initialize request_queue's numa node during allocation
Date: Tue, 22 Nov 2011 16:19:58 -0500 [thread overview]
Message-ID: <20111122211954.GA17120@redhat.com> (raw)
In-Reply-To: <20111122152739.GA5663@redhat.com>
On Tue, Nov 22 2011 at 10:27am -0500,
Vivek Goyal <vgoyal@redhat.com> wrote:
> On Tue, Nov 22, 2011 at 02:00:24AM -0800, David Rientjes wrote:
...
> >
> > blk_throtl_init() is trying to allocate on a specific node and it appears
> > like its zonelists were never built successfully. I'd guess it's trying
> > to allocate on node 0 since it's not onlined above, probably because this
> > is the crashkernel. Your SRAT maps two different nodes but it's only
> > onlining node 1 and not node 0.
> >
> > The problem is that blk_alloc_queue_node() allocs the requeue_queue with
> > __GFP_ZERO, which zeros it and never initialized the node field so it
> > remains zero. blk_throtl_init() then calls kzalloc_node() on node 0 which
> > doesn't have initialized zonelists.
> >
> > Maybe try this?
> >
> > diff --git a/block/blk-core.c b/block/blk-core.c
> > index ea70e6c..99c1881 100644
> > --- a/block/blk-core.c
> > +++ b/block/blk-core.c
> > @@ -467,6 +467,7 @@ struct request_queue *blk_alloc_queue_node(gfp_t gfp_mask, int node_id)
> > q->backing_dev_info.state = 0;
> > q->backing_dev_info.capabilities = BDI_CAP_MAP_COPY;
> > q->backing_dev_info.name = "block";
> > + q->node = node_id;
> >
>
> Storing q->node info at queue allocation time makes sense to me. In fact
> it might make sense to clean it up from blk_init_allocated_queue_node
> and assume that passed queue has queue->node set at the allocation time.
>
> CCing Mike Snitzer who introduced blk_init_allocated_queue_node(). Mike
> what do you think. I am not sure it makes sense to pass in nodeid, both
> at queue allocation and queue initialization time. To me, it should make
> more sense to allocate the queue at one node and that becomes the default
> node for reset of the initialization.
Yeah, that makes sense to me too:
From: Mike Snitzer <snitzer@redhat.com>
Subject: block: initialize request_queue's numa node during allocation
Set request_queue's node in blk_alloc_queue_node() rather than
blk_init_allocated_queue_node(). This avoids blk_throtl_init() using
q->node before it is initialized.
Rename blk_init_allocated_queue_node() to blk_init_allocated_queue().
Signed-off-by: Mike Snitzer <snitzer@redhat.com>
---
block/blk-core.c | 14 +++-----------
include/linux/blkdev.h | 3 ---
2 files changed, 3 insertions(+), 14 deletions(-)
diff --git a/block/blk-core.c b/block/blk-core.c
index ea70e6c..20d69f6 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -467,6 +467,7 @@ struct request_queue *blk_alloc_queue_node(gfp_t gfp_mask, int node_id)
q->backing_dev_info.state = 0;
q->backing_dev_info.capabilities = BDI_CAP_MAP_COPY;
q->backing_dev_info.name = "block";
+ q->node = node_id;
err = bdi_init(&q->backing_dev_info);
if (err) {
@@ -551,7 +552,7 @@ blk_init_queue_node(request_fn_proc *rfn, spinlock_t *lock, int node_id)
if (!uninit_q)
return NULL;
- q = blk_init_allocated_queue_node(uninit_q, rfn, lock, node_id);
+ q = blk_init_allocated_queue(uninit_q, rfn, lock);
if (!q)
blk_cleanup_queue(uninit_q);
@@ -563,18 +564,9 @@ struct request_queue *
blk_init_allocated_queue(struct request_queue *q, request_fn_proc *rfn,
spinlock_t *lock)
{
- return blk_init_allocated_queue_node(q, rfn, lock, -1);
-}
-EXPORT_SYMBOL(blk_init_allocated_queue);
-
-struct request_queue *
-blk_init_allocated_queue_node(struct request_queue *q, request_fn_proc *rfn,
- spinlock_t *lock, int node_id)
-{
if (!q)
return NULL;
- q->node = node_id;
if (blk_init_free_list(q))
return NULL;
@@ -604,7 +596,7 @@ blk_init_allocated_queue_node(struct request_queue *q, request_fn_proc *rfn,
return NULL;
}
-EXPORT_SYMBOL(blk_init_allocated_queue_node);
+EXPORT_SYMBOL(blk_init_allocated_queue);
int blk_get_queue(struct request_queue *q)
{
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index c7a6d3b..94acd81 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -805,9 +805,6 @@ extern void blk_unprep_request(struct request *);
*/
extern struct request_queue *blk_init_queue_node(request_fn_proc *rfn,
spinlock_t *lock, int node_id);
-extern struct request_queue *blk_init_allocated_queue_node(struct request_queue *,
- request_fn_proc *,
- spinlock_t *, int node_id);
extern struct request_queue *blk_init_queue(request_fn_proc *, spinlock_t *);
extern struct request_queue *blk_init_allocated_queue(struct request_queue *,
request_fn_proc *, spinlock_t *);
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
WARNING: multiple messages have this Message-ID (diff)
From: Mike Snitzer <snitzer@redhat.com>
To: Vivek Goyal <vgoyal@redhat.com>
Cc: David Rientjes <rientjes@google.com>,
Dave Young <dyoung@redhat.com>, Jens Axboe <axboe@kernel.dk>,
linux-kernel@vger.kernel.org, linux-mm@kvack.org,
kexec@lists.infradead.org
Subject: [PATCH] block: initialize request_queue's numa node during allocation
Date: Tue, 22 Nov 2011 16:19:58 -0500 [thread overview]
Message-ID: <20111122211954.GA17120@redhat.com> (raw)
In-Reply-To: <20111122152739.GA5663@redhat.com>
On Tue, Nov 22 2011 at 10:27am -0500,
Vivek Goyal <vgoyal@redhat.com> wrote:
> On Tue, Nov 22, 2011 at 02:00:24AM -0800, David Rientjes wrote:
...
> >
> > blk_throtl_init() is trying to allocate on a specific node and it appears
> > like its zonelists were never built successfully. I'd guess it's trying
> > to allocate on node 0 since it's not onlined above, probably because this
> > is the crashkernel. Your SRAT maps two different nodes but it's only
> > onlining node 1 and not node 0.
> >
> > The problem is that blk_alloc_queue_node() allocs the requeue_queue with
> > __GFP_ZERO, which zeros it and never initialized the node field so it
> > remains zero. blk_throtl_init() then calls kzalloc_node() on node 0 which
> > doesn't have initialized zonelists.
> >
> > Maybe try this?
> >
> > diff --git a/block/blk-core.c b/block/blk-core.c
> > index ea70e6c..99c1881 100644
> > --- a/block/blk-core.c
> > +++ b/block/blk-core.c
> > @@ -467,6 +467,7 @@ struct request_queue *blk_alloc_queue_node(gfp_t gfp_mask, int node_id)
> > q->backing_dev_info.state = 0;
> > q->backing_dev_info.capabilities = BDI_CAP_MAP_COPY;
> > q->backing_dev_info.name = "block";
> > + q->node = node_id;
> >
>
> Storing q->node info at queue allocation time makes sense to me. In fact
> it might make sense to clean it up from blk_init_allocated_queue_node
> and assume that passed queue has queue->node set at the allocation time.
>
> CCing Mike Snitzer who introduced blk_init_allocated_queue_node(). Mike
> what do you think. I am not sure it makes sense to pass in nodeid, both
> at queue allocation and queue initialization time. To me, it should make
> more sense to allocate the queue at one node and that becomes the default
> node for reset of the initialization.
Yeah, that makes sense to me too:
From: Mike Snitzer <snitzer@redhat.com>
Subject: block: initialize request_queue's numa node during allocation
Set request_queue's node in blk_alloc_queue_node() rather than
blk_init_allocated_queue_node(). This avoids blk_throtl_init() using
q->node before it is initialized.
Rename blk_init_allocated_queue_node() to blk_init_allocated_queue().
Signed-off-by: Mike Snitzer <snitzer@redhat.com>
---
block/blk-core.c | 14 +++-----------
include/linux/blkdev.h | 3 ---
2 files changed, 3 insertions(+), 14 deletions(-)
diff --git a/block/blk-core.c b/block/blk-core.c
index ea70e6c..20d69f6 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -467,6 +467,7 @@ struct request_queue *blk_alloc_queue_node(gfp_t gfp_mask, int node_id)
q->backing_dev_info.state = 0;
q->backing_dev_info.capabilities = BDI_CAP_MAP_COPY;
q->backing_dev_info.name = "block";
+ q->node = node_id;
err = bdi_init(&q->backing_dev_info);
if (err) {
@@ -551,7 +552,7 @@ blk_init_queue_node(request_fn_proc *rfn, spinlock_t *lock, int node_id)
if (!uninit_q)
return NULL;
- q = blk_init_allocated_queue_node(uninit_q, rfn, lock, node_id);
+ q = blk_init_allocated_queue(uninit_q, rfn, lock);
if (!q)
blk_cleanup_queue(uninit_q);
@@ -563,18 +564,9 @@ struct request_queue *
blk_init_allocated_queue(struct request_queue *q, request_fn_proc *rfn,
spinlock_t *lock)
{
- return blk_init_allocated_queue_node(q, rfn, lock, -1);
-}
-EXPORT_SYMBOL(blk_init_allocated_queue);
-
-struct request_queue *
-blk_init_allocated_queue_node(struct request_queue *q, request_fn_proc *rfn,
- spinlock_t *lock, int node_id)
-{
if (!q)
return NULL;
- q->node = node_id;
if (blk_init_free_list(q))
return NULL;
@@ -604,7 +596,7 @@ blk_init_allocated_queue_node(struct request_queue *q, request_fn_proc *rfn,
return NULL;
}
-EXPORT_SYMBOL(blk_init_allocated_queue_node);
+EXPORT_SYMBOL(blk_init_allocated_queue);
int blk_get_queue(struct request_queue *q)
{
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index c7a6d3b..94acd81 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -805,9 +805,6 @@ extern void blk_unprep_request(struct request *);
*/
extern struct request_queue *blk_init_queue_node(request_fn_proc *rfn,
spinlock_t *lock, int node_id);
-extern struct request_queue *blk_init_allocated_queue_node(struct request_queue *,
- request_fn_proc *,
- spinlock_t *, int node_id);
extern struct request_queue *blk_init_queue(request_fn_proc *, spinlock_t *);
extern struct request_queue *blk_init_allocated_queue(struct request_queue *,
request_fn_proc *, spinlock_t *);
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
WARNING: multiple messages have this Message-ID (diff)
From: Mike Snitzer <snitzer@redhat.com>
To: Vivek Goyal <vgoyal@redhat.com>
Cc: David Rientjes <rientjes@google.com>,
Dave Young <dyoung@redhat.com>, Jens Axboe <axboe@kernel.dk>,
linux-kernel@vger.kernel.org, linux-mm@kvack.org,
kexec@lists.infradead.org
Subject: [PATCH] block: initialize request_queue's numa node during allocation
Date: Tue, 22 Nov 2011 16:19:58 -0500 [thread overview]
Message-ID: <20111122211954.GA17120@redhat.com> (raw)
In-Reply-To: <20111122152739.GA5663@redhat.com>
On Tue, Nov 22 2011 at 10:27am -0500,
Vivek Goyal <vgoyal@redhat.com> wrote:
> On Tue, Nov 22, 2011 at 02:00:24AM -0800, David Rientjes wrote:
...
> >
> > blk_throtl_init() is trying to allocate on a specific node and it appears
> > like its zonelists were never built successfully. I'd guess it's trying
> > to allocate on node 0 since it's not onlined above, probably because this
> > is the crashkernel. Your SRAT maps two different nodes but it's only
> > onlining node 1 and not node 0.
> >
> > The problem is that blk_alloc_queue_node() allocs the requeue_queue with
> > __GFP_ZERO, which zeros it and never initialized the node field so it
> > remains zero. blk_throtl_init() then calls kzalloc_node() on node 0 which
> > doesn't have initialized zonelists.
> >
> > Maybe try this?
> >
> > diff --git a/block/blk-core.c b/block/blk-core.c
> > index ea70e6c..99c1881 100644
> > --- a/block/blk-core.c
> > +++ b/block/blk-core.c
> > @@ -467,6 +467,7 @@ struct request_queue *blk_alloc_queue_node(gfp_t gfp_mask, int node_id)
> > q->backing_dev_info.state = 0;
> > q->backing_dev_info.capabilities = BDI_CAP_MAP_COPY;
> > q->backing_dev_info.name = "block";
> > + q->node = node_id;
> >
>
> Storing q->node info at queue allocation time makes sense to me. In fact
> it might make sense to clean it up from blk_init_allocated_queue_node
> and assume that passed queue has queue->node set at the allocation time.
>
> CCing Mike Snitzer who introduced blk_init_allocated_queue_node(). Mike
> what do you think. I am not sure it makes sense to pass in nodeid, both
> at queue allocation and queue initialization time. To me, it should make
> more sense to allocate the queue at one node and that becomes the default
> node for reset of the initialization.
Yeah, that makes sense to me too:
From: Mike Snitzer <snitzer@redhat.com>
Subject: block: initialize request_queue's numa node during allocation
Set request_queue's node in blk_alloc_queue_node() rather than
blk_init_allocated_queue_node(). This avoids blk_throtl_init() using
q->node before it is initialized.
Rename blk_init_allocated_queue_node() to blk_init_allocated_queue().
Signed-off-by: Mike Snitzer <snitzer@redhat.com>
---
block/blk-core.c | 14 +++-----------
include/linux/blkdev.h | 3 ---
2 files changed, 3 insertions(+), 14 deletions(-)
diff --git a/block/blk-core.c b/block/blk-core.c
index ea70e6c..20d69f6 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -467,6 +467,7 @@ struct request_queue *blk_alloc_queue_node(gfp_t gfp_mask, int node_id)
q->backing_dev_info.state = 0;
q->backing_dev_info.capabilities = BDI_CAP_MAP_COPY;
q->backing_dev_info.name = "block";
+ q->node = node_id;
err = bdi_init(&q->backing_dev_info);
if (err) {
@@ -551,7 +552,7 @@ blk_init_queue_node(request_fn_proc *rfn, spinlock_t *lock, int node_id)
if (!uninit_q)
return NULL;
- q = blk_init_allocated_queue_node(uninit_q, rfn, lock, node_id);
+ q = blk_init_allocated_queue(uninit_q, rfn, lock);
if (!q)
blk_cleanup_queue(uninit_q);
@@ -563,18 +564,9 @@ struct request_queue *
blk_init_allocated_queue(struct request_queue *q, request_fn_proc *rfn,
spinlock_t *lock)
{
- return blk_init_allocated_queue_node(q, rfn, lock, -1);
-}
-EXPORT_SYMBOL(blk_init_allocated_queue);
-
-struct request_queue *
-blk_init_allocated_queue_node(struct request_queue *q, request_fn_proc *rfn,
- spinlock_t *lock, int node_id)
-{
if (!q)
return NULL;
- q->node = node_id;
if (blk_init_free_list(q))
return NULL;
@@ -604,7 +596,7 @@ blk_init_allocated_queue_node(struct request_queue *q, request_fn_proc *rfn,
return NULL;
}
-EXPORT_SYMBOL(blk_init_allocated_queue_node);
+EXPORT_SYMBOL(blk_init_allocated_queue);
int blk_get_queue(struct request_queue *q)
{
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index c7a6d3b..94acd81 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -805,9 +805,6 @@ extern void blk_unprep_request(struct request *);
*/
extern struct request_queue *blk_init_queue_node(request_fn_proc *rfn,
spinlock_t *lock, int node_id);
-extern struct request_queue *blk_init_allocated_queue_node(struct request_queue *,
- request_fn_proc *,
- spinlock_t *, int node_id);
extern struct request_queue *blk_init_queue(request_fn_proc *, spinlock_t *);
extern struct request_queue *blk_init_allocated_queue(struct request_queue *,
request_fn_proc *, spinlock_t *);
next prev parent reply other threads:[~2011-11-22 21:20 UTC|newest]
Thread overview: 42+ messages / expand[flat|nested] mbox.gz Atom feed top
2011-11-22 8:25 BUG: zonelist->_zonerefs == 0x1c08 Dave Young
2011-11-22 8:25 ` Dave Young
2011-11-22 8:25 ` Dave Young
2011-11-22 8:29 ` Dave Young
2011-11-22 8:29 ` Dave Young
2011-11-22 8:29 ` Dave Young
2011-11-22 10:00 ` David Rientjes
2011-11-22 10:00 ` David Rientjes
2011-11-22 10:00 ` David Rientjes
2011-11-22 15:27 ` Vivek Goyal
2011-11-22 15:27 ` Vivek Goyal
2011-11-22 15:27 ` Vivek Goyal
2011-11-22 21:19 ` Mike Snitzer [this message]
2011-11-22 21:19 ` [PATCH] block: initialize request_queue's numa node during allocation Mike Snitzer
2011-11-22 21:19 ` Mike Snitzer
2011-11-22 21:32 ` Vivek Goyal
2011-11-22 21:32 ` Vivek Goyal
2011-11-22 21:32 ` Vivek Goyal
2011-11-22 21:45 ` David Rientjes
2011-11-22 21:45 ` David Rientjes
2011-11-22 21:45 ` David Rientjes
2011-11-22 22:02 ` Mike Snitzer
2011-11-22 22:02 ` Mike Snitzer
2011-11-22 22:02 ` Mike Snitzer
2011-11-23 1:14 ` [patch v2 for-3.2] " David Rientjes
2011-11-23 1:14 ` David Rientjes
2011-11-23 1:14 ` David Rientjes
2011-11-23 1:39 ` Dave Young
2011-11-23 1:39 ` Dave Young
2011-11-23 1:39 ` Dave Young
2011-11-23 9:59 ` Jens Axboe
2011-11-23 9:59 ` Jens Axboe
2011-11-23 9:59 ` Jens Axboe
2011-11-22 22:13 ` Mike Snitzer
2011-11-22 22:13 ` Mike Snitzer
2011-11-22 22:13 ` Mike Snitzer
2011-11-23 1:23 ` BUG: zonelist->_zonerefs == 0x1c08 Dave Young
2011-11-23 1:23 ` Dave Young
2011-11-23 1:23 ` Dave Young
2011-11-23 2:05 ` Dave Young
2011-11-23 2:05 ` Dave Young
2011-11-23 2:05 ` Dave Young
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20111122211954.GA17120@redhat.com \
--to=snitzer@redhat.com \
--cc=axboe@kernel.dk \
--cc=dyoung@redhat.com \
--cc=kexec@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=rientjes@google.com \
--cc=vgoyal@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.