Re: [PATCH 8/8] blk-mq: support per-distpatch_queue flush machinery
On Thu, Sep 11, 2014 at 3:02 AM, Christoph Hellwig wrote: > On Wed, Sep 10, 2014 at 09:40:11AM +0800, Ming Lei wrote: >> I am wondering we can do that because lifetime is totally different >> between flush requests and tag_set requests which are initialized >> before request queue is created. > > We shouldn't do it in the tag sets, but where we allocate and free > each hctx: blk_mq_init_queue and blk_mq_free_hw_queues. That should work, but both flush queue's allocation and .init_request() have to move to the function because hctx->numa_node is basically ready in blk_mq_init_queue(). Then blk_init_flush() only need to allocate the data for legacy case. Thanks, -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 8/8] blk-mq: support per-distpatch_queue flush machinery
On Wed, Sep 10, 2014 at 09:40:11AM +0800, Ming Lei wrote: > I am wondering we can do that because lifetime is totally different > between flush requests and tag_set requests which are initialized > before request queue is created. We shouldn't do it in the tag sets, but where we allocate and free each hctx: blk_mq_init_queue and blk_mq_free_hw_queues. -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 8/8] blk-mq: support per-distpatch_queue flush machinery
On Wed, Sep 10, 2014 at 2:48 AM, Christoph Hellwig wrote: >> + if (hctx) { >> + int cmd_sz = q->tag_set->cmd_size; >> + int node = hctx->numa_node; >> + >> + fq = kzalloc_node(sizeof(*fq), GFP_KERNEL, node); >> + if (!fq) >> + goto failed; >> + >> + rq_sz = round_up(rq_sz + cmd_sz, cache_line_size()); >> + fq->flush_rq = kzalloc_node(rq_sz, GFP_KERNEL, node); >> + if (!fq->flush_rq) >> + goto rq_failed; >> + >> + spin_lock_init(&fq->mq_flush_lock); >> + } else { >> + fq = kzalloc(sizeof(*fq), GFP_KERNEL); >> + if (!fq) >> + goto failed; >> + >> + fq->flush_rq = kzalloc(rq_sz, GFP_KERNEL); >> + if (!fq->flush_rq) >> + goto rq_failed; >> + } > > Seems like this would be a lot cleaner by passing the cmd_size and > node_id explicitly. The added benefit would be that we could also > pass the node for the blk_init_queue_node() case. OK. > >> +static void __blk_mq_exit_flush(struct request_queue *q, >> + unsigned free_end, unsigned int exit_end) >> +{ >> + struct blk_mq_hw_ctx *hctx; >> + unsigned int k; >> + struct blk_flush_queue *fq; >> + struct blk_mq_tag_set *set = q->tag_set; >> + unsigned start_idx = set->queue_depth; >> + >> + queue_for_each_hw_ctx(q, hctx, k) { >> + if (k >= free_end) >> + break; >> + >> + fq = hctx->fq; >> + if (k < exit_end && set->ops->exit_request) >> + set->ops->exit_request(set->driver_data, >> + fq->flush_rq, k, >> + start_idx + k); >> + >> + blk_free_flush_queue(fq); >> + } > > Can we merge the mq init/exit case into some existing for each hctx > loop in blk-mq? I am wondering we can do that because lifetime is totally different between flush requests and tag_set requests which are initialized before request queue is created. Thanks, -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 8/8] blk-mq: support per-distpatch_queue flush machinery
> + if (hctx) { > + int cmd_sz = q->tag_set->cmd_size; > + int node = hctx->numa_node; > + > + fq = kzalloc_node(sizeof(*fq), GFP_KERNEL, node); > + if (!fq) > + goto failed; > + > + rq_sz = round_up(rq_sz + cmd_sz, cache_line_size()); > + fq->flush_rq = kzalloc_node(rq_sz, GFP_KERNEL, node); > + if (!fq->flush_rq) > + goto rq_failed; > + > + spin_lock_init(&fq->mq_flush_lock); > + } else { > + fq = kzalloc(sizeof(*fq), GFP_KERNEL); > + if (!fq) > + goto failed; > + > + fq->flush_rq = kzalloc(rq_sz, GFP_KERNEL); > + if (!fq->flush_rq) > + goto rq_failed; > + } Seems like this would be a lot cleaner by passing the cmd_size and node_id explicitly. The added benefit would be that we could also pass the node for the blk_init_queue_node() case. > +static void __blk_mq_exit_flush(struct request_queue *q, > + unsigned free_end, unsigned int exit_end) > +{ > + struct blk_mq_hw_ctx *hctx; > + unsigned int k; > + struct blk_flush_queue *fq; > + struct blk_mq_tag_set *set = q->tag_set; > + unsigned start_idx = set->queue_depth; > + > + queue_for_each_hw_ctx(q, hctx, k) { > + if (k >= free_end) > + break; > + > + fq = hctx->fq; > + if (k < exit_end && set->ops->exit_request) > + set->ops->exit_request(set->driver_data, > + fq->flush_rq, k, > + start_idx + k); > + > + blk_free_flush_queue(fq); > + } Can we merge the mq init/exit case into some existing for each hctx loop in blk-mq? -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html