On Wed, Aug 02, 2017 at 11:41:19AM -0700, Dave Jiang wrote:
> Adding blk-mq support to the pmem driver in addition to the direct bio
> support. This allows for hardware offloading via DMA engines. By default
> the bio method will be enabled. The blk-mq support can be turned on via
> module parameter queue_mode=1.
>
> Signed-off-by: Dave Jiang <dave.jiang@xxxxxxxxx>
One small nit with error handling. With that addressed you can add:
Reviewed-by: Ross Zwisler <ross.zwisler@xxxxxxxxxxxxxxx>
> @@ -303,17 +369,47 @@ static int pmem_attach_disk(struct device *dev,
> return -EBUSY;
> }
>
> - q = blk_alloc_queue_node(GFP_KERNEL, dev_to_node(dev));
> - if (!q)
> - return -ENOMEM;
> + if (queue_mode == PMEM_Q_MQ) {
> + pmem->tag_set.ops = &pmem_mq_ops;
> + pmem->tag_set.nr_hw_queues = nr_online_nodes;
> + pmem->tag_set.queue_depth = 64;
> + pmem->tag_set.numa_node = dev_to_node(dev);
> + pmem->tag_set.cmd_size = sizeof(struct pmem_cmd);
> + pmem->tag_set.flags = BLK_MQ_F_SHOULD_MERGE;
> + pmem->tag_set.driver_data = pmem;
> +
> + rc = blk_mq_alloc_tag_set(&pmem->tag_set);
> + if (rc < 0)
> + return rc;
> +
> + pmem->q = blk_mq_init_queue(&pmem->tag_set);
> + if (IS_ERR(pmem->q)) {
> + blk_mq_free_tag_set(&pmem->tag_set);
> + return -ENOMEM;
> + }
>
> - if (devm_add_action_or_reset(dev, pmem_release_queue, q))
> - return -ENOMEM;
> + if (devm_add_action_or_reset(dev, pmem_release_queue, pmem)) {
In the failure case for both this devm_add_action_or_reset() and the one a few
lines down in the PMEM_Q_BIO case, I think you should manually call
pmem_release_queue() instead of calling blk_mq_free_tag_set(). This will free
the tag set and it will clean up the queue you just set up with
blk_mq_init_queue() or blk_alloc_queue_node().
> + blk_mq_free_tag_set(&pmem->tag_set);
> + return -ENOMEM;
> + }
> + } else if (queue_mode == PMEM_Q_BIO) {
> + pmem->q = blk_alloc_queue_node(GFP_KERNEL, dev_to_node(dev));
> + if (!pmem->q)
> + return -ENOMEM;
> +
> + if (devm_add_action_or_reset(dev, pmem_release_queue, pmem))
The 2nd case. This one was like this in the previous code, but should be
fixed unless I'm missing something.
> + return -ENOMEM;
> +
> + blk_queue_make_request(pmem->q, pmem_make_request);
> + } else {
> + dev_warn(dev, "Invalid queue mode: %d\n", queue_mode);
> + return -EINVAL;
> + }
--
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
