[8/8] IB/srp: Add multichannel support

Commit Message

Bart Van Assche Oct. 3, 2014, 1:01 p.m. UTC

On 10/02/14 18:55, Jens Axboe wrote:
> Sure, that's fine as well, but the function needs a more descriptive
> name. I try to think of it like I have never looked at the code and need
> to write a driver, it's a lot easier if the functions are named
> appropriately. Seeing blk_mq_rq_tag() and even with reading the function
> comment, I'm really none the wiser and would assume I need to use this
> function to get the tag.
> 
> So we can do the single function, but lets call it
> blk_mq_unique_rq_tag(). That's special enough that people will know this
> is something that doesn't just return the request tag. Then add an extra
> sentence to the comment you already have on when this is needed.
> 
> And lets roll those bitshift values and masks into a define or enum so
> it's collected in one place.

How about the patch below ? In that patch all comments should have been
addressed that Christoph and you have formulated so far.

Thanks,

Bart.

[PATCH] blk-mq: Add blk_mq_unique_tag()

The queuecommand() callback functions in SCSI low-level drivers
need to know which hardware context has been selected by the
block layer. Since this information is not available in the
request structure, and since passing the hctx pointer directly to
the queuecommand callback function would require modification of
all SCSI LLDs, add a function to the block layer that allows to
query the hardware context index.

Signed-off-by: Bart Van Assche <[e-mail]>
---
 block/blk-mq-tag.c     | 27 +++++++++++++++++++++++++++
 block/blk-mq.c         |  2 ++
 include/linux/blk-mq.h | 23 +++++++++++++++++++++++
 3 files changed, 52 insertions(+)

Comments

Jens Axboe Oct. 3, 2014, 2:24 p.m. UTC | #1

On 2014-10-03 07:01, Bart Van Assche wrote:
> On 10/02/14 18:55, Jens Axboe wrote:
>> Sure, that's fine as well, but the function needs a more descriptive
>> name. I try to think of it like I have never looked at the code and need
>> to write a driver, it's a lot easier if the functions are named
>> appropriately. Seeing blk_mq_rq_tag() and even with reading the function
>> comment, I'm really none the wiser and would assume I need to use this
>> function to get the tag.
>>
>> So we can do the single function, but lets call it
>> blk_mq_unique_rq_tag(). That's special enough that people will know this
>> is something that doesn't just return the request tag. Then add an extra
>> sentence to the comment you already have on when this is needed.
>>
>> And lets roll those bitshift values and masks into a define or enum so
>> it's collected in one place.
>
> How about the patch below ? In that patch all comments should have been
> addressed that Christoph and you have formulated so far.

Looks good to me now. Get rid of the extra TAG in the 
BLK_MQ_UNIQUE_TAG_TAG_BITS/MASK naming though, then you can add my 
acked-by if Christoph wants to take this through the scsi tree.

Patch

diff --git a/block/blk-mq-tag.c b/block/blk-mq-tag.c
index c1b9242..b5088f0 100644
--- a/block/blk-mq-tag.c
+++ b/block/blk-mq-tag.c
@@ -595,6 +595,33 @@  int blk_mq_tag_update_depth(struct blk_mq_tags *tags, unsigned int tdepth)
 	return 0;
 }
 
+/**
+ * blk_mq_unique_tag() - return a tag that is unique queue-wide
+ * @rq: request for which to compute a unique tag
+ *
+ * The tag field in struct request is unique per hardware queue but not over
+ * all hardware queues. Hence this function that returns a tag with the
+ * hardware context index in the upper bits and the per hardware queue tag in
+ * the lower bits.
+ *
+ * Note: When called for a request that queued on a non-multiqueue request
+ * queue, the hardware context index is set to zero.
+ */
+u32 blk_mq_unique_tag(struct request *rq)
+{
+	struct request_queue *q = rq->q;
+	struct blk_mq_hw_ctx *hctx;
+	int hwq = 0;
+
+	if (q->mq_ops) {
+		hctx = q->mq_ops->map_queue(q, rq->mq_ctx->cpu);
+		hwq = hctx->queue_num;
+	}
+
+	return blk_mq_build_unique_tag(hwq, rq->tag);
+}
+EXPORT_SYMBOL(blk_mq_unique_tag);
+
 ssize_t blk_mq_tag_sysfs_show(struct blk_mq_tags *tags, char *page)
 {
 	char *orig_page = page;
diff --git a/block/blk-mq.c b/block/blk-mq.c
index df8e1e0..8098aac 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -2018,6 +2018,8 @@  static int blk_mq_alloc_rq_maps(struct blk_mq_tag_set *set)
  */
 int blk_mq_alloc_tag_set(struct blk_mq_tag_set *set)
 {
+	BUILD_BUG_ON(BLK_MQ_MAX_DEPTH > 1 << BLK_MQ_UNIQUE_TAG_TAG_BITS);
+
 	if (!set->nr_hw_queues)
 		return -EINVAL;
 	if (!set->queue_depth)
diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
index eac4f31..b53d0c2 100644
--- a/include/linux/blk-mq.h
+++ b/include/linux/blk-mq.h
@@ -167,6 +167,29 @@  struct request *blk_mq_alloc_request(struct request_queue *q, int rw,
 		gfp_t gfp, bool reserved);
 struct request *blk_mq_tag_to_rq(struct blk_mq_tags *tags, unsigned int tag);
 
+enum {
+	BLK_MQ_UNIQUE_TAG_TAG_BITS = 16,
+	BLK_MQ_UNIQUE_TAG_TAG_MASK = (1 << BLK_MQ_UNIQUE_TAG_TAG_BITS) - 1,
+};
+
+u32 blk_mq_unique_tag(struct request *rq);
+
+static inline u32 blk_mq_build_unique_tag(int hwq, int tag)
+{
+	return (hwq << BLK_MQ_UNIQUE_TAG_TAG_BITS) |
+		(tag & BLK_MQ_UNIQUE_TAG_TAG_MASK);
+}
+
+static inline u16 blk_mq_unique_tag_to_hwq(u32 unique_tag)
+{
+	return unique_tag >> BLK_MQ_UNIQUE_TAG_TAG_BITS;
+}
+
+static inline u16 blk_mq_unique_tag_to_tag(u32 unique_tag)
+{
+	return unique_tag & BLK_MQ_UNIQUE_TAG_TAG_MASK;
+}
+
 struct blk_mq_hw_ctx *blk_mq_map_queue(struct request_queue *, const int ctx_index);
 struct blk_mq_hw_ctx *blk_mq_alloc_single_hw_queue(struct blk_mq_tag_set *, unsigned int, int);