megaraid_sas: Enable shared host tag map
Subsystems (3)
PATCH 1/1 megaraid_sas: Enable shared host tag map
The megaraid SAS driver uses a shared host tag map internally, so we should be telling the block layer about it.
--- drivers/scsi/megaraid/megaraid_sas_base.c | 6 ++++++ 1 file changed, 6 insertions(+)
drivers/scsi/megaraid/megaraid_sas_base.c +6 -0
@@ -2757,6 +2757,7 @@ static struct scsi_host_template megasas_template = { .use_clustering = ENABLE_CLUSTERING, .change_queue_depth = scsi_change_queue_depth, .no_write_same = 1, + .use_blk_tags = 1, };/**
@@ -4996,6 +4997,11 @@ static int megasas_io_attach(struct megasas_instance *instance) host->hostt->eh_bus_reset_handler =NULL ; } +if (scsi_init_shared_tag_map (host, host->can_queue)) { +printk (KERN_INFO "megasas: failed to init shared tag map\n" ); +return -ENODEV ; + } +/* * Notify the mid-layer about the new controller */
Signature · 1 line
1.8.5.2
Quoted 2 lines · Hannes Reinecke
The megaraid SAS driver uses a shared host tag map internally, so we should be telling the block layer about it.
But it doesn't make use of request->tag yet. This would only be useful if you got rid of the internal tag allocator.
Quoted 7 lines · Christoph Hellwig
Quoted 2 lines · Hannes Reinecke
The megaraid SAS driver uses a shared host tag map internally, so we should be telling the block layer about it.
But it doesn't make use of
request->tagyet. This would only be useful if you got rid of the internal tag allocator.
It is useful as is, as we'll be getting prefixed logging output :-)
But yeah, it would be good to get rid of the internal tag allocator.
Which I didn't do yet as the driver is using a larger tag map than that one announced to the block layer.
This is to facilitate internal command submission, which should always work independent on any tag starvation issues from the upper layers.
So when moving to the generic tag allocation code I'd need:
- an block-layer helper for allocating a tag without a request (basically separating the existing blk-tag functionality, easy)
- introducing 'emergency pools' for the tag map, allowing to allocate a tag even under I/O pressure.
What I did was to:
- Split off blk_reserve_tag() functionality from blk_start_tag() (and similar with blk_end_tag)
- registered the overall size of the shared tag map
- called blk_resize_tags() to shrink it to ->can_queue
- Implemented a 'force' attribute to blk_reserve_tag() to allow it to dip into the excess size.
But then I didn't really like this mis-use of the blk_resize_tag() operation; I'd rather have it marked explicitly.
However, I'm sure megasas isn't the only driver requiring such a feature (libata eg would benefit from this, too).
Ideas?
Cheers,
Hannes
Signature · 8 lines
Dr. Hannes Reinecke zSeries & Storage [email protected] +49 911 74053 688 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: J. Hawn, J. Guild, F. Imendörffer, HRB 21284 (AG Nürnberg) -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 13 lines
-----Original Message-----
From: Hannes Reinecke [mailto:[email protected]]
Sent: Monday, November 24, 2014 9:21 PM
To: Christoph Hellwig
Cc: James Bottomley; Sumit Saxena; Kashyap Desai; linux- [email protected]
Subject: Re: [PATCH]megaraid_sas: Enable shared host tag mapQuoted 4 lines · Christoph Hellwig
Quoted 2 lines · Hannes Reinecke
The megaraid SAS driver uses a shared host tag map internally, so we should be telling the block layer about it.
Quoted 8 lines
Quoted 3 lines
But it doesn't make use of
request->tagyet. This would only be useful if you got rid of the internal tag allocator.It is useful as is, as we'll be getting prefixed logging output :-)
But yeah, it would be good to get rid of the internal tag allocator.
Which I didn't do yet as the driver is using a larger tag map than that
one
Quoted 2 lines
announced to the block layer.
This is to facilitate internal command submission, which should always
work
Quoted 8 lines
independent on any tag starvation issues from the upper layers.
So when moving to the generic tag allocation code I'd need:
- an block-layer helper for allocating a tag without a request (basically separating the existing blk-tag functionality, easy)
- introducing 'emergency pools' for the tag map, allowing to allocate a tag even under I/O pressure.
Quoted 7 lines
What I did was to:
- Split offblk_reserve_tag()functionality fromblk_start_tag()(and similar withblk_end_tag)
- registered the overall size of the shared tag map
- calledblk_resize_tags()to shrink it to ->can_queue
- Implemented a 'force' attribute toblk_reserve_tag()to allow it to dip into the excess size.
Hannes,
I just received your reply... My response crossed with yours. I have attached preview patch for your reference which I refer in my earlier response.Megaraid_sas driver will expose only actual IO command queue depth to the SML and it will manage all internal used commands without taking any help from SML.
Driver will now expose max_scsi_cmds to the upper layer which will be lower than actually FW can support max command.
E.a
+ instance->max_scsi_cmds = instance->max_fw_cmds -
+ (MEGASAS_FUSION_INTERNAL_CMDS +
+ MEGASAS_FUSION_IOCTL_CMDS);
We have to do this to make use of existing OS distribution which really does not support blk_reserve_tag().
~ Kashyap
Quoted 2 lines
But then I didn't really like this mis-use of the
blk_resize_tag()
operation; I'd
Quoted 7 lines
rather have it marked explicitly.
However, I'm sure megasas isn't the only driver requiring such a feature (libata eg would benefit from this, too).
Ideas?
Quoted 8 lines
Cheers,
Hannes
--
Dr. Hannes Reinecke zSeries & Storage
[email protected] +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 21284 (AG Nürnberg)
distro/base//megaraid_sas.h +7 -11
@@ -33,7 +33,7 @@/* * MegaRAID SAS Driver meta data */ -#define MEGASAS_VERSION "06.807.01.00" +#define MEGASAS_VERSION "06.807.02.00"#define MEGASAS_RELDATE "Nov 13, 2014"#define MEGASAS_EXT_VERSION "Nov 13, 17:00:00 PDT 2014"#define FALSE 0
@@ -156,6 +156,11 @@#define MFI_FRAME_DIR_BOTH 0x0018#define MFI_FRAME_IEEE 0x0020 +/* Driver internal */ +#define DRV_DCMD_POLLED_MODE 0x1 +#define BLK_TAG_DEBUG 0 +#define BLK_TAG_REFCOUNT 0 +/* * Definition for cmd_status */
@@ -239,12 +244,6 @@ MEGASAS_MAX_DEV_PER_CHANNEL) -typedef enum _MR_MFI_MPT_PTHR_FLAGS { -MFI_MPT_DETACHED =0 , -MFI_LIST_ADDED =1 , -MFI_MPT_ATTACHED =2 , -}MR_MFI_MPT_PTHR_FLAGS ; - typedef enum _MR_SCSI_CMD_TYPE { READ_WRITE_LDIO = 0, NON_READ_WRITE_LDIO = 1,
@@ -2222,13 +2221,10 @@ struct megasas_cmd {u8 cmd_status;u8 abort_aen;u8 retry_for_fw_reset; +u8 flags;struct list_head list;struct scsi_cmnd *scmd; - -void *mpt_pthr_cmd_blocked; -atomic_t mfi_mpt_pthr; -u8 is_wait_event;struct megasas_instance *instance;union {
distro/base//megaraid_sas_base.c +197 -264
726 lines not on this page · raw message
distro/base//megaraid_sas_fusion.c +137 -121
410 lines not on this page · raw message
distro/base//megaraid_sas_fusion.h +1 -4
@@ -854,8 +854,8 @@ struct megasas_cmd_fusion {*/ u32 sync_cmd_idx;u32 index; -u8 flags;u8 pd_r1_lb;/*PD after R1 load balancing*/ +atomic_t refcount; }; typedef struct _LD_LOAD_BALANCE_INFO
@@ -890,9 +890,6 @@ typedef struct LOG_BLOCK_SPAN_INFO {struct fusion_context {struct megasas_cmd_fusion **cmd_list; -struct list_head cmd_pool; - -spinlock_t cmd_pool_lock;dma_addr_t req_frames_desc_phys;u8 *req_frames_desc;
Quoted 1 line · Hannes Reinecke
It is useful as is, as we'll be getting prefixed logging output :-)
Use the blk-mq code path if you care :)
Quoted 5 lines
Which I didn't do yet as the driver is using a larger tag map than that one announced to the block layer.
This is to facilitate internal command submission, which should always work independent on any tag starvation issues from the upper layers.
This is an "issue" for a lot of drivers. blk-mq provides a reserved_tags pool for that, which reserves a number of tags for internal use, those must be allocated using blk_mq_alloc_request with the reserved argument set to true.
The lockless hpsa patches expose this to SCSI, which I'm generally fine with, but we need to find a way to transparently make this work for the old code path, too. This might be as simple as embedding a second blk_queue_tag structure into the Scsi_Host, adding a constant prefix to the tag and providing some wrappes in scsi that allow allocating a struct request (or rather scsi_cmnd) for internal use.
Quoted 16 lines · Christoph Hellwig
Quoted 1 line · Hannes Reinecke
It is useful as is, as we'll be getting prefixed logging output :-)
Use the blk-mq code path if you care :)
Quoted 5 lines
Which I didn't do yet as the driver is using a larger tag map than that one announced to the block layer.
This is to facilitate internal command submission, which should always work independent on any tag starvation issues from the upper layers.This is an "issue" for a lot of drivers. blk-mq provides a
reserved_tagspool for that, which reserves a number of tags for internal use, those must be allocated usingblk_mq_alloc_requestwith the reserved argument set to true.
Quoted 7 lines
The lockless hpsa patches expose this to SCSI, which I'm generally fine with, but we need to find a way to transparently make this work for the old code path, too. This might be as simple as embedding a second
blk_queue_tagstructure into theScsi_Host, adding a constant prefix to the tag and providing some wrappes in scsi that allow allocating a struct request (or ratherscsi_cmnd) for internal use.
I'd rather have a single map to get request/tags from; otherwise we'd be arbitrarily starving internal requests even though the 'main' tag map is empty.
My plan was more to mark a certain range of tags as 'reserved', and add another helper/argument to allow to dip into the reserved pool, too.
A tentative patch is attached.
Idea is to call blk_queue_init_tags() with the actual tag size and then blk_resize_tags() to limit the number of tags for the request queue.
The driver can then use 'blk_allocate_tag' with the appropriate max depth to get tags from the range [max_depth:real_max_depth].
Cheers,
Hannes
Signature · 4 lines
Dr. Hannes Reinecke zSeries & Storage [email protected] +49 911 74053 688 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: J. Hawn, J. Guild, F. Imendörffer, HRB 21284 (AG Nürnberg)
From e872bb0c4c2b1f72982f4d31925d3b2f65317ffd Mon Sep 17 00:00:00 2001
From: Hannes Reinecke <[email protected]>
Date: Tue, 25 Nov 2014 15:43:07 +0100
Subject: [PATCH] blk-tag: separate out blk_(allocate|release)_tag
Separate out helper functions blk_allocate_tag() and blk_release_tag().
--- block/blk-tag.c | 74 ++++++++++++++++++++++++++++++++++++++++++--------------- 1 file changed, 55 insertions(+), 19 deletions(-)
block/blk-tag.c +55 -19
@@ -244,6 +244,24 @@ int blk_queue_resize_tags(struct request_queue *q, int new_depth) }EXPORT_SYMBOL (blk_queue_resize_tags); +void blk_release_tag (struct blk_queue_tag *bqt,int tag) +{ +if (unlikely (!bqt)) +return ; + +if (unlikely (!test_bit (tag, bqt->tag_map))) { +printk (KERN_ERR "%s: attempt to clear non-busy tag (%d)\n" , + __func__, tag); +return ; + } +/* +* The tag_map bit acts as a lock for tag_index[bit], so we need +* unlock memory barrier semantics. +*/ +clear_bit_unlock (tag, bqt->tag_map); +} +EXPORT_SYMBOL (blk_release_tag); +/** * blk_queue_end_tag - end tag operations for a request * @q: the request queue for the device
@@ -275,18 +293,43 @@ void blk_queue_end_tag(struct request_queue *q, struct request *rq) bqt->tag_index[tag] =NULL ; -if (unlikely (!test_bit (tag, bqt->tag_map))) { -printk (KERN_ERR "%s: attempt to clear non-busy tag (%d)\n" , - __func__, tag); -return ; - } +blk_release_tag (bqt, tag); +} +EXPORT_SYMBOL (blk_queue_end_tag); + +/** +* blk_reserve_tag - lock and return the next free tag +* @bqt: tag map +* @max_depth: max tag depth to use +* +* Description: +* Lock and return the next free tag. +* The tag needs to be freed up after usage with +* blk_release_tag() +* +**/ +int blk_reserve_tag (struct blk_queue_tag *bqt,int max_depth) +{ +int tag =-1 ; + +if (!bqt) +return tag; +if (max_depth >= bqt->real_max_depth) +return -1 ; + +do { + tag =find_first_zero_bit (bqt->tag_map, max_depth); +if (tag >= max_depth) +return tag; + + }while (test_and_set_bit_lock (tag, bqt->tag_map));/* -* The tag_map bit acts as a lock for tag_index[bit], so we need -* unlock memory barrier semantics. +* We need lock ordering semantics given by test_and_set_bit_lock. +* See blk_queue_end_tag for details. */ -clear_bit_unlock (tag, bqt->tag_map); +return tag; } -EXPORT_SYMBOL (blk_queue_end_tag); +EXPORT_SYMBOL (blk_reserve_tag);/** * blk_queue_start_tag - find a free tag and assign it
@@ -343,16 +386,9 @@ int blk_queue_start_tag(struct request_queue *q, struct request *rq)return 1 ; } -do { - tag =find_first_zero_bit (bqt->tag_map, max_depth); -if (tag >= max_depth) -return 1 ; - - }while (test_and_set_bit_lock (tag, bqt->tag_map)); -/* -* We need lock ordering semantics given by test_and_set_bit_lock. -* See blk_queue_end_tag for details. -*/ + tag =blk_reserve_tag (bqt, max_depth); +if (tag ==-1 ) +return 1 ; rq->cmd_flags |=REQ_QUEUED ; rq->tag = tag;
Signature · 1 line
1.8.5.2
Quoted 3 lines · Hannes Reinecke
I'd rather have a single map to get request/tags from; otherwise we'd be arbitrarily starving internal requests even though the 'main' tag map is empty.
At least in blk-mq the assumption is that a driver needs very few internal tags, and it might need access to them in "emergency" situations like resets or aborts.
Quoted 3 lines
My plan was more to mark a certain range of tags as 'reserved', and add another helper/argument to allow to dip into the reserved pool, too.
We can add this as optional behavior, but I think the existing blk-mq behavior is a good default.
Quoted 6 lines
A tentative patch is attached.
Idea is to callblk_queue_init_tags()with the actual tag size and thenblk_resize_tags()to limit the number of tags for the request queue.
The driver can then use 'blk_allocate_tag' with the appropriate max depth to get tags from the range [max_depth:real_max_depth].
I'd much prefer the blk-mq approach with two maps. Either way please make sure whatever you come up is compatible with blk-mq.
Quoted 11 lines
-----Original Message-----
From: Christoph Hellwig [mailto:[email protected]]
Sent: Tuesday, November 25, 2014 8:02 PM
To: Hannes Reinecke
Cc: James Bottomley; Sumit Saxena; Kashyap Desai; linux- [email protected]; Webb Scales; Don Brace; Jens Axboe
Subject: Re: [PATCH]megaraid_sas: Enable shared host tag mapQuoted 1 line · Hannes Reinecke
It is useful as is, as we'll be getting prefixed logging output :-)
Quoted 8 lines
Use the blk-mq code path if you care :)
Quoted 4 lines
Which I didn't do yet as the driver is using a larger tag map than that one announced to the block layer.
This is to facilitate internal command submission, which should always work independent on any tag starvation issues from the upper layers.This is an "issue" for a lot of drivers. blk-mq provides a
reserved_tags pool
Quoted 1 line
for that, which reserves a number of tags for internal use, those must
be
Quoted 4 lines
allocated using
blk_mq_alloc_requestwith the reserved argument set to true.The lockless hpsa patches expose this to SCSI, which I'm generally fine
with,
Quoted 1 line
but we need to find a way to transparently make this work for the old
code
Quoted 2 lines
path, too. This might be as simple as embedding a second
blk_queue_tagstructure into theScsi_Host, adding a constant prefix to the tag and
providing
Quoted 2 lines
some wrappes in scsi that allow allocating a struct request (or rather
scsi_cmnd) for internal use.
Just trying to understand your above comment. I see host template has new field called
"reserved_tags" which is normally used by mid layer to reserved block tag pool for driver's internal use.
What if driver is OK to manage internal pool without any blk tag dependency and do not expose actual max can queue of what FW can support. For example, in megaraid_sas driver FW expose max_fw_cmd = 1024 and driver keep some reserve (let's say 24 commands for internal use) and it just expose 1000 command for blk tags. This will work for blk-mq and legacy blk driver. Let driver to manage whatever internal reserved it kept for and do not add any dependency with blk tag from above layer for those reserved pool.
` Kashyap
Quoted 4 lines · Kashyap Desai
Just trying to understand your above comment. I see host template has new field called
"reserved_tags" which is normally used by mid layer to reserved block tag pool for driver's internal use.
Quoted 8 lines
What if driver is OK to manage internal pool without any blk tag dependency and do not expose actual max can queue of what FW can support. For example, in
megaraid_sasdriver FW exposemax_fw_cmd= 1024 and driver keep some reserve (let's say 24 commands for internal use) and it just expose 1000 command for blk tags. This will work for blk-mq and legacy blk driver. Let driver to manage whatever internal reserved it kept for and do not add any dependency with blk tag from above layer for those reserved pool.
The driver can of coure always just manage its internal tags, but that means we need to duplicate a tag allocator in every driver. In short it works ok for now, but I'd rather solve the problem in a single place.
Quoted 5 lines
-----Original Message-----
From: Christoph Hellwig [mailto:[email protected]]
Sent: Monday, November 24, 2014 9:06 PM
To: Hannes Reinecke
Cc: James Bottomley; Sumit Saxena; Kashyap Desai; Christoph Hellwig;
linux-
Quoted 8 lines
[email protected]
Subject: Re: [PATCH]megaraid_sas: Enable shared host tag mapQuoted 2 lines · Hannes Reinecke
The megaraid SAS driver uses a shared host tag map internally, so we should be telling the block layer about it.
But it doesn't make use of
request->tagyet. This would only be useful
if you
Quoted 1 line
got rid of the internal tag allocator.
I have patch for code changes in this particular area and that is currently under internal test cycle.
Next patch series will start using shared blk tag for internal MPT frame allocation. It will get-rid-off MPT frame pool link list completely and also does lots of refactor w.r.t MFI/MPT frame corruption issue. Using shared blk tag gave good performance improvement as driver completely removed mpt frame spin lock used in megasas_get_cmd_fusion/ megasas_return_cmd_fusion.
~ Kashyap