Re: [PATCH 8/8] IB/srp: Add multichannel support
On 10/7/2014 3:51 PM, Bart Van Assche wrote: On 09/23/14 18:32, Sagi Grimberg wrote: Since you don't seem to negotiate/declare multichannel with the target, did you test this code with some target implementations other than SCST that happen to be out there? (replying to an e-mail of two weeks ago) Hello Sagi, I have just verified that the multichannel code in this patch series works fine in combination with the upstream SRP target driver. Working as in single channel mode? or multichannel mode? Sagi. -- 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] IB/srp: Add multichannel support
On 10/13/14 10:17, Sagi Grimberg wrote: On 10/7/2014 3:51 PM, Bart Van Assche wrote: On 09/23/14 18:32, Sagi Grimberg wrote: Since you don't seem to negotiate/declare multichannel with the target, did you test this code with some target implementations other than SCST that happen to be out there? (replying to an e-mail of two weeks ago) I have just verified that the multichannel code in this patch series works fine in combination with the upstream SRP target driver. Working as in single channel mode? or multichannel mode? Hello Sagi, In my e-mail I was referring to multichannel mode. Bart. -- 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] IB/srp: Add multichannel support
On Mon, Oct 6, 2014 at 4:16 AM, Bart Van Assche bvanass...@acm.org wrote: On 10/02/14 19:30, Christoph Hellwig wrote: Also if we want to merge scsi LLDDs that can take advantage of multiqueue support it would probably be best if I take this via the SCSI tree. Sending these patches to you is fine with me, at least if Roland agrees. That's fine with me. Christoph/Bart should I just let you guys handle all the pending SRP patches, or is there anything I should pick up via the InfiniBand tree? Everything that's in flight looks reasonable to me, so I'm fine with however you guys want to merge it. - R. -- 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] IB/srp: Add multichannel support
On 09/23/14 18:32, Sagi Grimberg wrote: Since you don't seem to negotiate/declare multichannel with the target, did you test this code with some target implementations other than SCST that happen to be out there? (replying to an e-mail of two weeks ago) Hello Sagi, I have just verified that the multichannel code in this patch series works fine in combination with the upstream SRP target driver. Bart. -- 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] IB/srp: Add multichannel support
On 10/02/14 19:30, Christoph Hellwig wrote: Also if we want to merge scsi LLDDs that can take advantage of multiqueue support it would probably be best if I take this via the SCSI tree. Sending these patches to you is fine with me, at least if Roland agrees. Bart. -- 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] IB/srp: Add multichannel support
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 bvanass...@acm.org
---
block/blk-mq-tag.c | 27 +++
block/blk-mq.c | 2 ++
include/linux/blk-mq.h | 23 +++
3 files changed, 52 insertions(+)
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);
--
1.8.4.5
--
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] IB/srp: Add multichannel support
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. -- Jens Axboe -- 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] IB/srp: Add multichannel support
On 10/01/14 18:54, Jens Axboe wrote:
Lets get rid of the blk_mq_tag struct and just have it be an u32 or
something. We could potentially typedef it, but I'd prefer to just have
it be an unsigned 32-bit int.
Probably also need some init time safety checks that 16-bits is enough
to hold BLK_MQ_MAX_DEPTH. Just in case that is ever bumped, or the queue
prefixing changes.
And I think we need to name this better. Your comment correctly
describes that this generates a unique tag queue wide, but the name of
the function implies that we just return the request tag. Most drivers
wont use this. Perhaps add a queue flag that tells us that we should
generate these tags and have it setup ala:
u32 blk_mq_unique_rq_tag(struct request *rq)
{
struct request_queue *q = rq-q;
u32 tag = rq-tag ((1 16) - 1);
struct blk_mq_hw_ctx *hctx;
hctx = q-mq_ops-map_queue(q, rq-mq_ctx-cpu);
return tag | (hctx-queue_num 16);
}
u32 blk_mq_rq_tag(struct request *rq)
{
struct request_queue *q = rq-q;
if (q-mq_ops
test_bit(QUEUE_FLAG_UNIQUE_TAGS, q-queue_flags))
return blk_mq_unique_rq_tag(rq);
return rq-tag;
}
Would it be acceptable to let blk_mq_rq_tag() always return the
hardware context number and the per-hctx tag ? Block and SCSI LLD
drivers that do not need the hardware context number can still use
rq-tag. Drivers that need both can use blk_mq_rq_tag(). That way we do
not have to introduce a new queue flag. How about the patch below
(which is still missing a BLK_MQ_MAX_DEPTH check):
diff --git a/block/blk-mq-tag.c b/block/blk-mq-tag.c
index c1b9242..8cfbc7b 100644
--- a/block/blk-mq-tag.c
+++ b/block/blk-mq-tag.c
@@ -595,6 +595,30 @@ int blk_mq_tag_update_depth(struct blk_mq_tags *tags,
unsigned int tdepth)
return 0;
}
+/**
+ * blk_mq_rq_tag() - return a tag that is unique queue-wide
+ *
+ * The tag field in struct request is unique per hardware queue but not
+ * queue-wide. Hence this function.
+ *
+ * Note: When called for a non-multiqueue request queue, the hardware context
+ * index is set to zero.
+ */
+u32 blk_mq_rq_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_rq_tag(hwq, rq-tag);
+}
+EXPORT_SYMBOL(blk_mq_rq_tag);
+
ssize_t blk_mq_tag_sysfs_show(struct blk_mq_tags *tags, char *page)
{
char *orig_page = page;
diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
index eac4f31..c5be535 100644
--- a/include/linux/blk-mq.h
+++ b/include/linux/blk-mq.h
@@ -166,6 +166,19 @@ bool blk_mq_can_queue(struct blk_mq_hw_ctx *);
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);
+u32 blk_mq_rq_tag(struct request *rq);
+static inline u32 blk_mq_build_rq_tag(int hwq, int tag)
+{
+ return (hwq 16) | (tag ((1 16) - 1));
+}
+static inline u16 blk_mq_rq_tag_to_hwq(u32 rq_tag)
+{
+ return rq_tag 16;
+}
+static inline u16 blk_mq_rq_tag_to_tag(u32 rq_tag)
+{
+ return rq_tag ((1 16) - 1);
+}
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);
--
1.8.4.5
--
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] IB/srp: Add multichannel support
On 10/02/2014 10:45 AM, Bart Van Assche wrote:
On 10/01/14 18:54, Jens Axboe wrote:
Lets get rid of the blk_mq_tag struct and just have it be an u32 or
something. We could potentially typedef it, but I'd prefer to just have
it be an unsigned 32-bit int.
Probably also need some init time safety checks that 16-bits is enough
to hold BLK_MQ_MAX_DEPTH. Just in case that is ever bumped, or the queue
prefixing changes.
And I think we need to name this better. Your comment correctly
describes that this generates a unique tag queue wide, but the name of
the function implies that we just return the request tag. Most drivers
wont use this. Perhaps add a queue flag that tells us that we should
generate these tags and have it setup ala:
u32 blk_mq_unique_rq_tag(struct request *rq)
{
struct request_queue *q = rq-q;
u32 tag = rq-tag ((1 16) - 1);
struct blk_mq_hw_ctx *hctx;
hctx = q-mq_ops-map_queue(q, rq-mq_ctx-cpu);
return tag | (hctx-queue_num 16);
}
u32 blk_mq_rq_tag(struct request *rq)
{
struct request_queue *q = rq-q;
if (q-mq_ops
test_bit(QUEUE_FLAG_UNIQUE_TAGS, q-queue_flags))
return blk_mq_unique_rq_tag(rq);
return rq-tag;
}
Would it be acceptable to let blk_mq_rq_tag() always return the
hardware context number and the per-hctx tag ? Block and SCSI LLD
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.
--
Jens Axboe
--
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] IB/srp: Add multichannel support
On Thu, Oct 02, 2014 at 06:45:55PM +0200, Bart Van Assche wrote: Would it be acceptable to let blk_mq_rq_tag() always return the hardware context number and the per-hctx tag ? Block and SCSI LLD drivers that do not need the hardware context number can still use rq-tag. Drivers that need both can use blk_mq_rq_tag(). That way we do not have to introduce a new queue flag. How about the patch below (which is still missing a BLK_MQ_MAX_DEPTH check): I'd add the unique_ part to the name that Jens added, and fix up the comment to be valid kerneldoc, but otherwise this looks fine to me. Also if we want to merge scsi LLDDs that can take advantage of multiqueue support it would probably be best if I take this via the SCSI tree. -- 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] IB/srp: Add multichannel support
On 09/19/14 17:38, Jens Axboe wrote:
ctx was meant to be private, unfortunately it's leaked a bit into other
parts of block/. But it's still private within that, at least.
Lets not add more stuff to struct request, it's already way too large.
We could add an exported
struct blk_mq_hw_ctx *blk_mq_request_to_hctx(struct request *rq)
{
struct request_queue *q = rq-q;
return q-mq_ops-map_queue(q, rq-mq_ctx-cpu);
}
for this.
How about the patch below ? That patch makes it easy for SCSI LLDs to obtain
the hardware context index.
[PATCH] blk-mq: Add blk_get_mq_tag()
The queuecommand() callback functions in SCSI low-level drivers
must know which hardware context has been selected by the block
layer. 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 bvanass...@acm.org
---
block/blk-mq-tag.c | 24
include/linux/blk-mq.h | 16
2 files changed, 40 insertions(+)
diff --git a/block/blk-mq-tag.c b/block/blk-mq-tag.c
index c1b9242..5618759 100644
--- a/block/blk-mq-tag.c
+++ b/block/blk-mq-tag.c
@@ -595,6 +595,30 @@ int blk_mq_tag_update_depth(struct blk_mq_tags *tags,
unsigned int tdepth)
return 0;
}
+/**
+ * blk_get_mq_tag() - return a tag that is unique queue-wide
+ *
+ * The tag field in struct request is unique per hardware queue but not
+ * queue-wide. Hence this function. It is not only safe to use this function
+ * for multiqueue request queues but also for single-queue request queues.
+ * Note: rq-tag == -1 if tagging is not enabled for single-queue request
+ * queues.
+ */
+struct blk_mq_tag blk_get_mq_tag(struct request *rq)
+{
+ struct request_queue *q = rq-q;
+ struct blk_mq_tag tag = { rq-tag ((1 16) - 1) };
+ struct blk_mq_hw_ctx *hctx;
+
+ if (q-mq_ops) {
+ hctx = q-mq_ops-map_queue(q, rq-mq_ctx-cpu);
+ tag.tag |= hctx-queue_num 16;
+ }
+
+ return tag;
+}
+EXPORT_SYMBOL(blk_get_mq_tag);
+
ssize_t blk_mq_tag_sysfs_show(struct blk_mq_tags *tags, char *page)
{
char *orig_page = page;
diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
index eac4f31..eb419bc 100644
--- a/include/linux/blk-mq.h
+++ b/include/linux/blk-mq.h
@@ -88,6 +88,13 @@ struct blk_mq_tag_set {
struct list_headtag_list;
};
+/**
+ * struct blk_mq_tag - hardware queue index and per-queue tag
+ */
+struct blk_mq_tag {
+ u32 tag;
+};
+
typedef int (queue_rq_fn)(struct blk_mq_hw_ctx *, struct request *);
typedef struct blk_mq_hw_ctx *(map_queue_fn)(struct request_queue *, const
int);
typedef int (init_hctx_fn)(struct blk_mq_hw_ctx *, void *, unsigned int);
@@ -166,6 +173,15 @@ bool blk_mq_can_queue(struct blk_mq_hw_ctx *);
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);
+struct blk_mq_tag blk_get_mq_tag(struct request *rq);
+static inline u16 blk_mq_tag_to_hwq(struct blk_mq_tag t)
+{
+ return t.tag 16;
+}
+static inline u16 blk_mq_tag_to_tag(struct blk_mq_tag t)
+{
+ return t.tag ((1 16) - 1);
+}
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);
--
1.8.4.5
--
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] IB/srp: Add multichannel support
On 2014-10-01 10:08, Bart Van Assche wrote:
On 09/19/14 17:38, Jens Axboe wrote:
ctx was meant to be private, unfortunately it's leaked a bit into other
parts of block/. But it's still private within that, at least.
Lets not add more stuff to struct request, it's already way too large.
We could add an exported
struct blk_mq_hw_ctx *blk_mq_request_to_hctx(struct request *rq)
{
struct request_queue *q = rq-q;
return q-mq_ops-map_queue(q, rq-mq_ctx-cpu);
}
for this.
How about the patch below ? That patch makes it easy for SCSI LLDs to obtain
the hardware context index.
[PATCH] blk-mq: Add blk_get_mq_tag()
The queuecommand() callback functions in SCSI low-level drivers
must know which hardware context has been selected by the block
layer. 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 bvanass...@acm.org
---
block/blk-mq-tag.c | 24
include/linux/blk-mq.h | 16
2 files changed, 40 insertions(+)
diff --git a/block/blk-mq-tag.c b/block/blk-mq-tag.c
index c1b9242..5618759 100644
--- a/block/blk-mq-tag.c
+++ b/block/blk-mq-tag.c
@@ -595,6 +595,30 @@ int blk_mq_tag_update_depth(struct blk_mq_tags *tags,
unsigned int tdepth)
return 0;
}
+/**
+ * blk_get_mq_tag() - return a tag that is unique queue-wide
+ *
+ * The tag field in struct request is unique per hardware queue but not
+ * queue-wide. Hence this function. It is not only safe to use this function
+ * for multiqueue request queues but also for single-queue request queues.
+ * Note: rq-tag == -1 if tagging is not enabled for single-queue request
+ * queues.
+ */
+struct blk_mq_tag blk_get_mq_tag(struct request *rq)
+{
+ struct request_queue *q = rq-q;
+ struct blk_mq_tag tag = { rq-tag ((1 16) - 1) };
+ struct blk_mq_hw_ctx *hctx;
+
+ if (q-mq_ops) {
+ hctx = q-mq_ops-map_queue(q, rq-mq_ctx-cpu);
+ tag.tag |= hctx-queue_num 16;
+ }
+
+ return tag;
+}
+EXPORT_SYMBOL(blk_get_mq_tag);
Lets get rid of the blk_mq_tag struct and just have it be an u32 or
something. We could potentially typedef it, but I'd prefer to just have
it be an unsigned 32-bit int.
Probably also need some init time safety checks that 16-bits is enough
to hold BLK_MQ_MAX_DEPTH. Just in case that is ever bumped, or the queue
prefixing changes.
And I think we need to name this better. Your comment correctly
describes that this generates a unique tag queue wide, but the name of
the function implies that we just return the request tag. Most drivers
wont use this. Perhaps add a queue flag that tells us that we should
generate these tags and have it setup ala:
u32 blk_mq_unique_rq_tag(struct request *rq)
{
struct request_queue *q = rq-q;
u32 tag = rq-tag ((1 16) - 1);
struct blk_mq_hw_ctx *hctx;
hctx = q-mq_ops-map_queue(q, rq-mq_ctx-cpu);
return tag | (hctx-queue_num 16);
}
u32 blk_mq_rq_tag(struct request *rq)
{
struct request_queue *q = rq-q;
if (q-mq_ops
test_bit(QUEUE_FLAG_UNIQUE_TAGS, q-queue_flags))
return blk_mq_unique_rq_tag(rq);
return rq-tag;
}
Totally untested, just typed in email.
--
Jens Axboe
--
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] IB/srp: Add multichannel support
On 9/23/2014 10:02 PM, Bart Van Assche wrote: On 23/09/2014 10:32, Sagi Grimberg wrote: On 9/19/2014 4:00 PM, Bart Van Assche wrote: Improve performance by using multiple RDMA/RC channels per SCSI host for communicating with an SRP target. Hey Bart, Since you don't seem to negotiate/declare multichannel with the target, did you test this code with some target implementations other than SCST that happen to be out there? Overall, I think this patch would be easier to review if you also provide a list of logical changes (which obviously are introduced in this patch). Patch 7/8 can use some more information of target-channel relations as well. Hello Sagi, That's a good question. So far this patch series has only been tested against the SCST SRP target driver. However, as you probably noticed, if setting up a second or later RDMA channel fails SRP login is not failed but communication proceeds with the number of channels that have been established. This mechanism should retain backwards compatibility with SRP target systems that do not support multichannel communication. However, if the new code for SRP login turns out to be triggering bugs in existing SRP target implementations we can still add a blacklist for these implementations. I'm more concerned that a target will accept multichannel and then starts flipping since that wasn't tested in I don't know when, probably never... Since SRP_LOGIN_REQ/RESP has some free bits why not declare it and activate it when both sides *says* they support it? I'd be much calmer knowing we're on the safe side on this... I will provide a more detailed list of logical changes in the second version of this patch series. Thanks, Plus, I would like to run it on my performance setups. can you point me to the SCST repo? is multichannel supported in scst trunk? Sagi. -- 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] IB/srp: Add multichannel support
On 24/09/2014 6:22, Sagi Grimberg wrote: Since SRP_LOGIN_REQ/RESP has some free bits why not declare it and activate it when both sides *says* they support it? I'd be much calmer knowing we're on the safe side on this... Hello Sagi, Since more than ten years the SRP protocol is an official ANSI standard. Since multichannel support has been defined in that standard my preference is to follow what has been documented in that standard with regard to multichannel operation. Using one of the free bits in the SRP login request and response would involve a protocol modification. Hence the proposal to add a blacklist for non-conforming target implementations. Plus, I would like to run it on my performance setups. can you point me to the SCST repo? is multichannel supported in scst trunk? I think multichannel support was already present in the SCST SRP target driver before I started maintaining that driver. However, last April a few patches were checked in to improve multichannel support in the SCST SRP target driver. These patches have been included in the SCST 3.0 release. Download instructions for SCST (3.0 and trunk) can be found e.g. here: http://scst.sourceforge.net/downloads.html. Bart. -- 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] IB/srp: Add multichannel support
On 9/24/2014 4:13 PM, Bart Van Assche wrote: On 24/09/2014 6:22, Sagi Grimberg wrote: Since SRP_LOGIN_REQ/RESP has some free bits why not declare it and activate it when both sides *says* they support it? I'd be much calmer knowing we're on the safe side on this... Hello Sagi, Since more than ten years the SRP protocol is an official ANSI standard. Since multichannel support has been defined in that standard my preference is to follow what has been documented in that standard with regard to multichannel operation. Just re-visited the r16a, srp_login request req_flags include MULTI CHANNEL ACTION (Table 10) and srp login response rsp_flags include MULTI-CHANNEL RESULT (Table 12). Did you notice those? Didn't see any reference in the patch... Using one of the free bits in the SRP login request and response would involve a protocol modification. Hence the proposal to add a blacklist for non-conforming target implementations. So I'm not so sure we need to update SRP login sequence... Plus, I would like to run it on my performance setups. can you point me to the SCST repo? is multichannel supported in scst trunk? I think multichannel support was already present in the SCST SRP target driver before I started maintaining that driver. However, last April a few patches were checked in to improve multichannel support in the SCST SRP target driver. These patches have been included in the SCST 3.0 release. Download instructions for SCST (3.0 and trunk) can be found e.g. here: http://scst.sourceforge.net/downloads.html. Thanks, P.S. Would it be possible to break 8/8 into more patches in the next round? it would help make it more review-able? Thanks, Sagi. -- 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] IB/srp: Add multichannel support
On 9/24/2014 4:38 PM, Sagi Grimberg wrote: On 9/24/2014 4:13 PM, Bart Van Assche wrote: On 24/09/2014 6:22, Sagi Grimberg wrote: Since SRP_LOGIN_REQ/RESP has some free bits why not declare it and activate it when both sides *says* they support it? I'd be much calmer knowing we're on the safe side on this... Hello Sagi, Since more than ten years the SRP protocol is an official ANSI standard. Since multichannel support has been defined in that standard my preference is to follow what has been documented in that standard with regard to multichannel operation. Just re-visited the r16a, srp_login request req_flags include MULTI CHANNEL ACTION (Table 10) and srp login response rsp_flags include MULTI-CHANNEL RESULT (Table 12). Did you notice those? Didn't see any reference in the patch... Using one of the free bits in the SRP login request and response would involve a protocol modification. Hence the proposal to add a blacklist for non-conforming target implementations. So I'm not so sure we need to update SRP login sequence... Wait, yes you did reference those... OK, I'm on board now... Sagi. -- 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] IB/srp: Add multichannel support
On 9/19/2014 4:00 PM, Bart Van Assche wrote: Improve performance by using multiple RDMA/RC channels per SCSI host for communicating with an SRP target. Hey Bart, Since you don't seem to negotiate/declare multichannel with the target, did you test this code with some target implementations other than SCST that happen to be out there? Overall, I think this patch would be easier to review if you also provide a list of logical changes (which obviously are introduced in this patch). Patch 7/8 can use some more information of target-channel relations as well. Signed-off-by: Bart Van Assche bvanass...@acm.org --- Documentation/ABI/stable/sysfs-driver-ib_srp | 25 +- drivers/infiniband/ulp/srp/ib_srp.c | 337 --- drivers/infiniband/ulp/srp/ib_srp.h | 20 +- 3 files changed, 287 insertions(+), 95 deletions(-) diff --git a/Documentation/ABI/stable/sysfs-driver-ib_srp b/Documentation/ABI/stable/sysfs-driver-ib_srp index b9688de..d5a459e 100644 --- a/Documentation/ABI/stable/sysfs-driver-ib_srp +++ b/Documentation/ABI/stable/sysfs-driver-ib_srp @@ -55,12 +55,12 @@ Description:Interface for making ib_srp connect to a new target. only safe with partial memory descriptor list support enabled (allow_ext_sg=1). * comp_vector, a number in the range 0..n-1 specifying the - MSI-X completion vector. Some HCA's allocate multiple (n) - MSI-X vectors per HCA port. If the IRQ affinity masks of - these interrupts have been configured such that each MSI-X - interrupt is handled by a different CPU then the comp_vector - parameter can be used to spread the SRP completion workload - over multiple CPU's. + MSI-X completion vector of the first RDMA channel. Some + HCA's allocate multiple (n) MSI-X vectors per HCA port. If + the IRQ affinity masks of these interrupts have been + configured such that each MSI-X interrupt is handled by a + different CPU then the comp_vector parameter can be used to + spread the SRP completion workload over multiple CPU's. Why do you want the first channel vector placement? Why can't you start with obvious 0? * tl_retry_count, a number in the range 2..7 specifying the IB RC retry count. * queue_size, the maximum number of commands that the @@ -88,6 +88,13 @@ Description: Whether ib_srp is allowed to include a partial memory descriptor list in an SRP_CMD when communicating with an SRP target. +What: /sys/class/scsi_host/hostn/ch_count +Date: November 1, 2014 +KernelVersion: 3.18 +Contact: linux-r...@vger.kernel.org +Description: Number of RDMA channels used for communication with the SRP + target. + What: /sys/class/scsi_host/hostn/cmd_sg_entries Date: May 19, 2011 KernelVersion:2.6.39 @@ -95,6 +102,12 @@ Contact:linux-r...@vger.kernel.org Description: Maximum number of data buffer descriptors that may be sent to the target in a single SRP_CMD request. +What: /sys/class/scsi_host/hostn/comp_vector +Date: September 2, 2013 +KernelVersion: 3.11 +Contact: linux-r...@vger.kernel.org +Description: Completion vector used for the first RDMA channel. + What: /sys/class/scsi_host/hostn/dgid Date: June 17, 2006 KernelVersion:2.6.17 diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c index 9feeea1..58ca618 100644 --- a/drivers/infiniband/ulp/srp/ib_srp.c +++ b/drivers/infiniband/ulp/srp/ib_srp.c @@ -123,6 +123,16 @@ MODULE_PARM_DESC(dev_loss_tmo, if fast_io_fail_tmo has not been set. \off\ means that this functionality is disabled.); +static unsigned ch_count; +module_param(ch_count, uint, 0444); +MODULE_PARM_DESC(ch_count, +Number of RDMA channels to use for communication with an SRP + target. Using more than one channel improves performance + if the HCA supports multiple completion vectors. The + default value is the minimum of four times the number of + online CPU sockets and the number of completion vectors + supported by the HCA.); + Can you explain the default math? how did you end-up with 4*numa_nodes? wouldn't per-cpu be a better fit? Moreover, while using multiple channels you don't suffice for less requests of less FMRs/FRs. I'm a bit concerned here about scalability of multi-channel. Should we take care of cases where the user will want lots of channels to lots of targets and might run out of resources? static void srp_add_one(struct ib_device *device); static void srp_remove_one(struct
Re: [PATCH 8/8] IB/srp: Add multichannel support
On 23/09/2014 10:32, Sagi Grimberg wrote: On 9/19/2014 4:00 PM, Bart Van Assche wrote: Improve performance by using multiple RDMA/RC channels per SCSI host for communicating with an SRP target. Hey Bart, Since you don't seem to negotiate/declare multichannel with the target, did you test this code with some target implementations other than SCST that happen to be out there? Overall, I think this patch would be easier to review if you also provide a list of logical changes (which obviously are introduced in this patch). Patch 7/8 can use some more information of target-channel relations as well. Hello Sagi, That's a good question. So far this patch series has only been tested against the SCST SRP target driver. However, as you probably noticed, if setting up a second or later RDMA channel fails SRP login is not failed but communication proceeds with the number of channels that have been established. This mechanism should retain backwards compatibility with SRP target systems that do not support multichannel communication. However, if the new code for SRP login turns out to be triggering bugs in existing SRP target implementations we can still add a blacklist for these implementations. I will provide a more detailed list of logical changes in the second version of this patch series. Bart. Bart. -- 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] IB/srp: Add multichannel support
On Fri, Sep 19, 2014 at 9:00 PM, Bart Van Assche bvanass...@acm.org wrote:
Improve performance by using multiple RDMA/RC channels per SCSI host
for communicating with an SRP target.
Signed-off-by: Bart Van Assche bvanass...@acm.org
---
Documentation/ABI/stable/sysfs-driver-ib_srp | 25 +-
drivers/infiniband/ulp/srp/ib_srp.c | 337
---
drivers/infiniband/ulp/srp/ib_srp.h | 20 +-
3 files changed, 287 insertions(+), 95 deletions(-)
diff --git a/Documentation/ABI/stable/sysfs-driver-ib_srp
b/Documentation/ABI/stable/sysfs-driver-ib_srp
index b9688de..d5a459e 100644
--- a/Documentation/ABI/stable/sysfs-driver-ib_srp
+++ b/Documentation/ABI/stable/sysfs-driver-ib_srp
@@ -55,12 +55,12 @@ Description:Interface for making ib_srp connect
to a new target.
only safe with partial memory descriptor list support
enabled
(allow_ext_sg=1).
* comp_vector, a number in the range 0..n-1 specifying the
- MSI-X completion vector. Some HCA's allocate multiple (n)
- MSI-X vectors per HCA port. If the IRQ affinity masks of
- these interrupts have been configured such that each MSI-X
- interrupt is handled by a different CPU then the comp_vector
- parameter can be used to spread the SRP completion workload
- over multiple CPU's.
+ MSI-X completion vector of the first RDMA channel. Some
+ HCA's allocate multiple (n) MSI-X vectors per HCA port. If
+ the IRQ affinity masks of these interrupts have been
+ configured such that each MSI-X interrupt is handled by a
+ different CPU then the comp_vector parameter can be used to
+ spread the SRP completion workload over multiple CPU's.
* tl_retry_count, a number in the range 2..7 specifying the
IB RC retry count.
* queue_size, the maximum number of commands that the
@@ -88,6 +88,13 @@ Description: Whether ib_srp is allowed to include a
partial memory
descriptor list in an SRP_CMD when communicating with an SRP
target.
+What: /sys/class/scsi_host/hostn/ch_count
+Date: November 1, 2014
+KernelVersion: 3.18
+Contact: linux-r...@vger.kernel.org
+Description: Number of RDMA channels used for communication with the SRP
+ target.
+
What: /sys/class/scsi_host/hostn/cmd_sg_entries
Date: May 19, 2011
KernelVersion: 2.6.39
@@ -95,6 +102,12 @@ Contact:linux-r...@vger.kernel.org
Description: Maximum number of data buffer descriptors that may be sent to
the target in a single SRP_CMD request.
+What: /sys/class/scsi_host/hostn/comp_vector
+Date: September 2, 2013
+KernelVersion: 3.11
+Contact: linux-r...@vger.kernel.org
+Description: Completion vector used for the first RDMA channel.
+
What: /sys/class/scsi_host/hostn/dgid
Date: June 17, 2006
KernelVersion: 2.6.17
diff --git a/drivers/infiniband/ulp/srp/ib_srp.c
b/drivers/infiniband/ulp/srp/ib_srp.c
index 9feeea1..58ca618 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -123,6 +123,16 @@ MODULE_PARM_DESC(dev_loss_tmo,
if fast_io_fail_tmo has not been set. \off\ means that
this functionality is disabled.);
+static unsigned ch_count;
+module_param(ch_count, uint, 0444);
+MODULE_PARM_DESC(ch_count,
+Number of RDMA channels to use for communication with an
SRP
+ target. Using more than one channel improves performance
+ if the HCA supports multiple completion vectors. The
+ default value is the minimum of four times the number of
+ online CPU sockets and the number of completion vectors
+ supported by the HCA.);
+
static void srp_add_one(struct ib_device *device);
static void srp_remove_one(struct ib_device *device);
static void srp_recv_completion(struct ib_cq *cq, void *ch_ptr);
@@ -556,17 +566,32 @@ err:
* Note: this function may be called without srp_alloc_iu_bufs() having been
* invoked. Hence the ch-[rt]x_ring checks.
*/
-static void srp_free_ch_ib(struct srp_rdma_ch *ch)
+static void srp_free_ch_ib(struct srp_target_port *target,
+ struct srp_rdma_ch *ch)
{
- struct srp_target_port *target = ch-target;
struct srp_device *dev = target-srp_host-srp_dev;
int i;
+ if (!ch-target)
+ return;
+
+ /*
+* Avoid that the SCSI error handler tries to use this channel after
+* it has been freed. The SCSI error handler can namely continue
+* trying to perform recovery actions
Re: [PATCH 8/8] IB/srp: Add multichannel support
On 09/19/14 16:28, Ming Lei wrote:
On Fri, Sep 19, 2014 at 9:00 PM, Bart Van Assche bvanass...@acm.org wrote:
@@ -2643,7 +2754,8 @@ static struct scsi_host_template srp_template = {
.proc_name = DRV_NAME,
.slave_configure= srp_slave_configure,
.info = srp_target_info,
- .queuecommand = srp_queuecommand,
+ .queuecommand = srp_sq_queuecommand,
+ .mq_queuecommand= srp_mq_queuecommand,
Another choice is to obtain hctx from request directly, then mq can
reuse the .queuecommand interface too.
Hello Ming,
Is the hctx information already available in the request data structure
? I have found a mq_ctx member but no hctx member. Did I perhaps
overlook something ?
Thanks,
Bart.
--
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] IB/srp: Add multichannel support
On Fri, Sep 19, 2014 at 11:21 PM, Bart Van Assche bvanass...@acm.org wrote:
On 09/19/14 16:28, Ming Lei wrote:
On Fri, Sep 19, 2014 at 9:00 PM, Bart Van Assche bvanass...@acm.org
wrote:
@@ -2643,7 +2754,8 @@ static struct scsi_host_template srp_template = {
.proc_name = DRV_NAME,
.slave_configure= srp_slave_configure,
.info = srp_target_info,
- .queuecommand = srp_queuecommand,
+ .queuecommand = srp_sq_queuecommand,
+ .mq_queuecommand= srp_mq_queuecommand,
Another choice is to obtain hctx from request directly, then mq can
reuse the .queuecommand interface too.
Hello Ming,
Is the hctx information already available in the request data structure ? I
have found a mq_ctx member but no hctx member. Did I perhaps overlook
something ?
You are right, but the mq_ctx can be mapped to hctx like below way:
ctx = rq-mq_ctx;
hctx = q-mq_ops-map_queue(q, ctx-cpu);
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] IB/srp: Add multichannel support
On 09/19/14 17:27, Ming Lei wrote:
On Fri, Sep 19, 2014 at 11:21 PM, Bart Van Assche bvanass...@acm.org wrote:
On 09/19/14 16:28, Ming Lei wrote:
On Fri, Sep 19, 2014 at 9:00 PM, Bart Van Assche bvanass...@acm.org
wrote:
@@ -2643,7 +2754,8 @@ static struct scsi_host_template srp_template = {
.proc_name = DRV_NAME,
.slave_configure= srp_slave_configure,
.info = srp_target_info,
- .queuecommand = srp_queuecommand,
+ .queuecommand = srp_sq_queuecommand,
+ .mq_queuecommand= srp_mq_queuecommand,
Another choice is to obtain hctx from request directly, then mq can
reuse the .queuecommand interface too.
Hello Ming,
Is the hctx information already available in the request data structure ? I
have found a mq_ctx member but no hctx member. Did I perhaps overlook
something ?
You are right, but the mq_ctx can be mapped to hctx like below way:
ctx = rq-mq_ctx;
hctx = q-mq_ops-map_queue(q, ctx-cpu);
Hello Ming,
Had you already noticed that the blk_mq_ctx data structure is a private
data structure (declared in block/blk-mq.h) and hence not available to
SCSI LLDs ? However, what might be possible is to cache the hctx pointer
in the request structure, e.g. like in the (completely untested) patch
below.
[PATCH] blk-mq: Cache hardware context pointer in struct request
Cache the hardware context pointer in struct request such that block
drivers can determine which hardware queue has been selected by
reading rq-mq_hctx-queue_num. This information is needed to select
the appropriate hardware queue in e.g. SCSI LLDs.
---
block/blk-flush.c | 6 +-
block/blk-mq.c | 16 +---
include/linux/blkdev.h | 1 +
3 files changed, 11 insertions(+), 12 deletions(-)
diff --git a/block/blk-flush.c b/block/blk-flush.c
index 3cb5e9e..698812d 100644
--- a/block/blk-flush.c
+++ b/block/blk-flush.c
@@ -327,13 +327,9 @@ static void flush_data_end_io(struct request *rq, int
error)
static void mq_flush_data_end_io(struct request *rq, int error)
{
struct request_queue *q = rq-q;
- struct blk_mq_hw_ctx *hctx;
- struct blk_mq_ctx *ctx;
+ struct blk_mq_hw_ctx *hctx = rq-mq_hctx;
unsigned long flags;
- ctx = rq-mq_ctx;
- hctx = q-mq_ops-map_queue(q, ctx-cpu);
-
/*
* After populating an empty queue, kick it to avoid stall. Read
* the comment in flush_end_io().
diff --git a/block/blk-mq.c b/block/blk-mq.c
index 383ea0c..8e428fe 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -210,6 +210,7 @@ __blk_mq_alloc_request(struct blk_mq_alloc_data *data, int
rw)
}
rq-tag = tag;
+ rq-mq_hctx = data-hctx;
blk_mq_rq_ctx_init(data-q, data-ctx, rq, rw);
return rq;
}
@@ -267,12 +268,10 @@ static void __blk_mq_free_request(struct blk_mq_hw_ctx
*hctx,
void blk_mq_free_request(struct request *rq)
{
struct blk_mq_ctx *ctx = rq-mq_ctx;
- struct blk_mq_hw_ctx *hctx;
- struct request_queue *q = rq-q;
+ struct blk_mq_hw_ctx *hctx = rq-mq_hctx;
ctx-rq_completed[rq_is_sync(rq)]++;
- hctx = q-mq_ops-map_queue(q, ctx-cpu);
__blk_mq_free_request(hctx, ctx, rq);
}
@@ -287,10 +286,10 @@ void blk_mq_free_request(struct request *rq)
void blk_mq_clone_flush_request(struct request *flush_rq,
struct request *orig_rq)
{
- struct blk_mq_hw_ctx *hctx =
- orig_rq-q-mq_ops-map_queue(orig_rq-q, orig_rq-mq_ctx-cpu);
+ struct blk_mq_hw_ctx *hctx = orig_rq-mq_hctx;
flush_rq-mq_ctx = orig_rq-mq_ctx;
+ flush_rq-mq_hctx = hctx;
flush_rq-tag = orig_rq-tag;
memcpy(blk_mq_rq_to_pdu(flush_rq), blk_mq_rq_to_pdu(orig_rq),
hctx-cmd_size);
@@ -956,6 +955,7 @@ void blk_mq_insert_request(struct request *rq, bool
at_head, bool run_queue,
rq-mq_ctx = ctx = current_ctx;
hctx = q-mq_ops-map_queue(q, ctx-cpu);
+ rq-mq_hctx = hctx;
if (rq-cmd_flags (REQ_FLUSH | REQ_FUA)
!(rq-cmd_flags (REQ_FLUSH_SEQ))) {
@@ -1001,6 +1001,7 @@ static void blk_mq_insert_requests(struct request_queue
*q,
rq = list_first_entry(list, struct request, queuelist);
list_del_init(rq-queuelist);
rq-mq_ctx = ctx;
+ rq-mq_hctx = hctx;
__blk_mq_insert_request(hctx, rq, false);
}
spin_unlock(ctx-lock);
@@ -1475,6 +1476,8 @@ static int blk_mq_hctx_cpu_offline(struct blk_mq_hw_ctx
*hctx, int cpu)
return NOTIFY_OK;
ctx = blk_mq_get_ctx(q);
+ hctx = q-mq_ops-map_queue(q, ctx-cpu);
+
spin_lock(ctx-lock);
while (!list_empty(tmp)) {
@@ -1482,10 +1485,9 @@ static int blk_mq_hctx_cpu_offline(struct blk_mq_hw_ctx
*hctx,
Re: [PATCH 8/8] IB/srp: Add multichannel support
On 09/19/2014 09:35 AM, Bart Van Assche wrote:
On 09/19/14 17:27, Ming Lei wrote:
On Fri, Sep 19, 2014 at 11:21 PM, Bart Van Assche bvanass...@acm.org wrote:
On 09/19/14 16:28, Ming Lei wrote:
On Fri, Sep 19, 2014 at 9:00 PM, Bart Van Assche bvanass...@acm.org
wrote:
@@ -2643,7 +2754,8 @@ static struct scsi_host_template srp_template = {
.proc_name = DRV_NAME,
.slave_configure= srp_slave_configure,
.info = srp_target_info,
- .queuecommand = srp_queuecommand,
+ .queuecommand = srp_sq_queuecommand,
+ .mq_queuecommand= srp_mq_queuecommand,
Another choice is to obtain hctx from request directly, then mq can
reuse the .queuecommand interface too.
Hello Ming,
Is the hctx information already available in the request data structure ? I
have found a mq_ctx member but no hctx member. Did I perhaps overlook
something ?
You are right, but the mq_ctx can be mapped to hctx like below way:
ctx = rq-mq_ctx;
hctx = q-mq_ops-map_queue(q, ctx-cpu);
Hello Ming,
Had you already noticed that the blk_mq_ctx data structure is a private
data structure (declared in block/blk-mq.h) and hence not available to
SCSI LLDs ? However, what might be possible is to cache the hctx pointer
in the request structure, e.g. like in the (completely untested) patch
below.
ctx was meant to be private, unfortunately it's leaked a bit into other
parts of block/. But it's still private within that, at least.
Lets not add more stuff to struct request, it's already way too large.
We could add an exported
struct blk_mq_hw_ctx *blk_mq_request_to_hctx(struct request *rq)
{
struct request_queue *q = rq-q;
return q-mq_ops-map_queue(q, rq-mq_ctx-cpu);
}
for this.
--
Jens Axboe
--
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] IB/srp: Add multichannel support
On 9/19/2014 6:38 PM, Jens Axboe wrote:
On 09/19/2014 09:35 AM, Bart Van Assche wrote:
On 09/19/14 17:27, Ming Lei wrote:
On Fri, Sep 19, 2014 at 11:21 PM, Bart Van Assche bvanass...@acm.org
wrote:
On 09/19/14 16:28, Ming Lei wrote:
On Fri, Sep 19, 2014 at 9:00 PM, Bart Van Assche bvanass...@acm.org
wrote:
@@ -2643,7 +2754,8 @@ static struct scsi_host_template srp_template = {
.proc_name = DRV_NAME,
.slave_configure= srp_slave_configure,
.info = srp_target_info,
- .queuecommand = srp_queuecommand,
+ .queuecommand = srp_sq_queuecommand,
+ .mq_queuecommand= srp_mq_queuecommand,
Another choice is to obtain hctx from request directly, then mq can
reuse the .queuecommand interface too.
Hello Ming,
Is the hctx information already available in the request data structure ? I
have found a mq_ctx member but no hctx member. Did I perhaps overlook
something ?
You are right, but the mq_ctx can be mapped to hctx like below way:
ctx = rq-mq_ctx;
hctx = q-mq_ops-map_queue(q, ctx-cpu);
Hello Ming,
Had you already noticed that the blk_mq_ctx data structure is a private
data structure (declared in block/blk-mq.h) and hence not available to
SCSI LLDs ? However, what might be possible is to cache the hctx pointer
in the request structure, e.g. like in the (completely untested) patch
below.
ctx was meant to be private, unfortunately it's leaked a bit into other
parts of block/. But it's still private within that, at least.
Lets not add more stuff to struct request, it's already way too large.
We could add an exported
struct blk_mq_hw_ctx *blk_mq_request_to_hctx(struct request *rq)
{
struct request_queue *q = rq-q;
return q-mq_ops-map_queue(q, rq-mq_ctx-cpu);
}
for this.
Hey,
So I agree that we shouldn't overload struct request with another
pointer, but it also seems a bit unnecessary to map again just to get
the hctx. Since in the future we would like LLDs to use scsi-mq why not
modify existing .queuecommand to pass hctx (or even better
hctx-driver_data) and for now LLDs won't use it. Once they choose to,
it will be available to them.
Sagi.
--
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] IB/srp: Add multichannel support
On 09/19/2014 11:30 AM, Sagi Grimberg wrote:
On 9/19/2014 6:38 PM, Jens Axboe wrote:
On 09/19/2014 09:35 AM, Bart Van Assche wrote:
On 09/19/14 17:27, Ming Lei wrote:
On Fri, Sep 19, 2014 at 11:21 PM, Bart Van Assche bvanass...@acm.org
wrote:
On 09/19/14 16:28, Ming Lei wrote:
On Fri, Sep 19, 2014 at 9:00 PM, Bart Van Assche bvanass...@acm.org
wrote:
@@ -2643,7 +2754,8 @@ static struct scsi_host_template srp_template = {
.proc_name = DRV_NAME,
.slave_configure= srp_slave_configure,
.info = srp_target_info,
- .queuecommand = srp_queuecommand,
+ .queuecommand = srp_sq_queuecommand,
+ .mq_queuecommand= srp_mq_queuecommand,
Another choice is to obtain hctx from request directly, then mq can
reuse the .queuecommand interface too.
Hello Ming,
Is the hctx information already available in the request data structure ?
I
have found a mq_ctx member but no hctx member. Did I perhaps overlook
something ?
You are right, but the mq_ctx can be mapped to hctx like below way:
ctx = rq-mq_ctx;
hctx = q-mq_ops-map_queue(q, ctx-cpu);
Hello Ming,
Had you already noticed that the blk_mq_ctx data structure is a private
data structure (declared in block/blk-mq.h) and hence not available to
SCSI LLDs ? However, what might be possible is to cache the hctx pointer
in the request structure, e.g. like in the (completely untested) patch
below.
ctx was meant to be private, unfortunately it's leaked a bit into other
parts of block/. But it's still private within that, at least.
Lets not add more stuff to struct request, it's already way too large.
We could add an exported
struct blk_mq_hw_ctx *blk_mq_request_to_hctx(struct request *rq)
{
struct request_queue *q = rq-q;
return q-mq_ops-map_queue(q, rq-mq_ctx-cpu);
}
for this.
Hey,
So I agree that we shouldn't overload struct request with another
pointer, but it also seems a bit unnecessary to map again just to get
the hctx. Since in the future we would like LLDs to use scsi-mq why not
modify existing .queuecommand to pass hctx (or even better
hctx-driver_data) and for now LLDs won't use it. Once they choose to,
it will be available to them.
That'd be fine as well. The mapping is cheap, though, but it would make
sense to have an appropriate way to just pass it in like it happens for
-queue_rq() for native blk-mq drivers.
--
Jens Axboe
--
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] IB/srp: Add multichannel support
On Fri, Sep 19, 2014 at 11:33:15AM -0600, Jens Axboe wrote: That'd be fine as well. The mapping is cheap, though, but it would make sense to have an appropriate way to just pass it in like it happens for -queue_rq() for native blk-mq drivers. I think just passing the hw_ctx is fine. But I don't want drivers to have to implement both methods, so we should make sure the new method works for both the blk-mq and !blk-mq case. Note that once thing the new method should get is a bool last paramter similar to the one I added to the queue_rq method in the block tree tree. It might be worth it to simply do a global search and replace and pass a hw_ctx method to all instances, too. -- 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
