linux-kernel - [PATCH] block: initialize request_queue's numa node during allocation

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20111122211954.GA17120@redhat.com>
Date:	Tue, 22 Nov 2011 16:19:58 -0500
From:	Mike Snitzer <snitzer@...hat.com>
To:	Vivek Goyal <vgoyal@...hat.com>
Cc:	David Rientjes <rientjes@...gle.com>,
	Dave Young <dyoung@...hat.com>, Jens Axboe <axboe@...nel.dk>,
	linux-kernel@...r.kernel.org, linux-mm@...ck.org,
	kexec@...ts.infradead.org
Subject: [PATCH] block: initialize request_queue's numa node during allocation

On Tue, Nov 22 2011 at 10:27am -0500,
Vivek Goyal <vgoyal@...hat.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@...hat.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@...hat.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 from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/