| Message ID | 20230530023518.16965-1-powen.kao@mediatek.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v2,1/1] scsi: ufs: core: Make UFS_MCQ_NUM_DEV_CMD_QUEUES a module parameter | expand |
Il 30/05/23 04:35, Po-Wen Kao ha scritto: > A dedicated queue for dev commands is not mandatory, hence let > UFS_MCQ_NUM_DEV_CMD_QUEUES become module parameter `dev_cmd_queues` > to allow sharing first hw queue for dev commands. > > When `dev_cmd_queues` is set to 0, the hwq 0 will be shared for I/O > requests and dev commands. In the same hwq, commands are processed based > on submission order hence might take longer to dispatch dev command > under heavy traffic. For the host with dedicated hwq for dev commands > can benefit in such scenario. > > Signed-off-by: Po-Wen Kao <powen.kao@mediatek.com> I imagine that MediaTek's UFS IP does not support dev_cmd_queues=1, does it? In that case, this should not be a UFS module parameter, but a setting that you provide from ufs-mediatek instead. Regards, Angelo > --- > drivers/ufs/core/ufs-mcq.c | 35 +++++++++++++++++++++++++++------- > drivers/ufs/core/ufshcd-priv.h | 2 +- > drivers/ufs/core/ufshcd.c | 2 +- > 3 files changed, 30 insertions(+), 9 deletions(-) > > diff --git a/drivers/ufs/core/ufs-mcq.c b/drivers/ufs/core/ufs-mcq.c > index 51b3c6ae781d..4ef48c84e62f 100644 > --- a/drivers/ufs/core/ufs-mcq.c > +++ b/drivers/ufs/core/ufs-mcq.c > @@ -16,7 +16,8 @@ > #define MAX_QUEUE_SUP GENMASK(7, 0) > #define UFS_MCQ_MIN_RW_QUEUES 2 > #define UFS_MCQ_MIN_READ_QUEUES 0 > -#define UFS_MCQ_NUM_DEV_CMD_QUEUES 1 > +#define UFS_MCQ_MAX_DEV_CMD_QUEUES 1 > +#define UFS_MCQ_MIN_DEV_CMD_QUEUES 0 > #define UFS_MCQ_MIN_POLL_QUEUES 0 > #define QUEUE_EN_OFFSET 31 > #define QUEUE_ID_OFFSET 16 > @@ -75,6 +76,23 @@ module_param_cb(poll_queues, &poll_queue_count_ops, &poll_queues, 0644); > MODULE_PARM_DESC(poll_queues, > "Number of poll queues used for r/w. Default value is 1"); > > +static int dev_cmd_queue_count_set(const char *val, const struct kernel_param *kp) > +{ > + return param_set_uint_minmax(val, kp, > + UFS_MCQ_MIN_DEV_CMD_QUEUES, > + UFS_MCQ_MAX_DEV_CMD_QUEUES); > +} > + > +static const struct kernel_param_ops dev_cmd_queue_count_ops = { > + .set = dev_cmd_queue_count_set, > + .get = param_get_uint, > +}; > + > +unsigned int dev_cmd_queues = 1; > +module_param_cb(dev_cmd_queues, &dev_cmd_queue_count_ops, &dev_cmd_queues, 0644); > +MODULE_PARM_DESC(dev_cmd_queues, > + "Number of queues used for dev command. Default value is 1"); > + > /** > * ufshcd_mcq_config_mac - Set the #Max Activ Cmds. > * @hba: per adapter instance > @@ -109,7 +127,7 @@ struct ufs_hw_queue *ufshcd_mcq_req_to_hwq(struct ufs_hba *hba, > u32 hwq = blk_mq_unique_tag_to_hwq(utag); > > /* uhq[0] is used to serve device commands */ > - return &hba->uhq[hwq + UFSHCD_MCQ_IO_QUEUE_OFFSET]; > + return &hba->uhq[hwq + dev_cmd_queues]; > } > > /** > @@ -153,7 +171,7 @@ static int ufshcd_mcq_config_nr_queues(struct ufs_hba *hba) > /* maxq is 0 based value */ > hba_maxq = FIELD_GET(MAX_QUEUE_SUP, hba->mcq_capabilities) + 1; > > - tot_queues = UFS_MCQ_NUM_DEV_CMD_QUEUES + read_queues + poll_queues + > + tot_queues = dev_cmd_queues + read_queues + poll_queues + > rw_queues; > > if (hba_maxq < tot_queues) { > @@ -162,7 +180,7 @@ static int ufshcd_mcq_config_nr_queues(struct ufs_hba *hba) > return -EOPNOTSUPP; > } > > - rem = hba_maxq - UFS_MCQ_NUM_DEV_CMD_QUEUES; > + rem = hba_maxq - dev_cmd_queues; > > if (rw_queues) { > hba->nr_queues[HCTX_TYPE_DEFAULT] = rw_queues; > @@ -188,7 +206,7 @@ static int ufshcd_mcq_config_nr_queues(struct ufs_hba *hba) > for (i = 0; i < HCTX_MAX_TYPES; i++) > host->nr_hw_queues += hba->nr_queues[i]; > > - hba->nr_hw_queues = host->nr_hw_queues + UFS_MCQ_NUM_DEV_CMD_QUEUES; > + hba->nr_hw_queues = host->nr_hw_queues + dev_cmd_queues; > return 0; > } > > @@ -424,8 +442,11 @@ int ufshcd_mcq_init(struct ufs_hba *hba) > > /* The very first HW queue serves device commands */ > hba->dev_cmd_queue = &hba->uhq[0]; > - /* Give dev_cmd_queue the minimal number of entries */ > - hba->dev_cmd_queue->max_entries = MAX_DEV_CMD_ENTRIES; > + if (dev_cmd_queues) { > + /* Give dedicated dev_cmd_queue the minimal number of entries */ > + hba->dev_cmd_queue->max_entries = MAX_DEV_CMD_ENTRIES; > + } > + > > host->host_tagset = 1; > return 0; > diff --git a/drivers/ufs/core/ufshcd-priv.h b/drivers/ufs/core/ufshcd-priv.h > index d53b93c21a0c..b490d645f12c 100644 > --- a/drivers/ufs/core/ufshcd-priv.h > +++ b/drivers/ufs/core/ufshcd-priv.h > @@ -78,7 +78,6 @@ struct ufs_hw_queue *ufshcd_mcq_req_to_hwq(struct ufs_hba *hba, > unsigned long ufshcd_mcq_poll_cqe_lock(struct ufs_hba *hba, > struct ufs_hw_queue *hwq); > > -#define UFSHCD_MCQ_IO_QUEUE_OFFSET 1 > #define SD_ASCII_STD true > #define SD_RAW false > int ufshcd_read_string_desc(struct ufs_hba *hba, u8 desc_index, > @@ -287,6 +286,7 @@ static inline int ufshcd_mcq_vops_config_esi(struct ufs_hba *hba) > return -EOPNOTSUPP; > } > > +extern unsigned int dev_cmd_queues; > extern const struct ufs_pm_lvl_states ufs_pm_lvl_states[]; > > /** > diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c > index 4ec8dacb447c..6361e21ca1c5 100644 > --- a/drivers/ufs/core/ufshcd.c > +++ b/drivers/ufs/core/ufshcd.c > @@ -5493,7 +5493,7 @@ static int ufshcd_poll(struct Scsi_Host *shost, unsigned int queue_num) > struct ufs_hw_queue *hwq; > > if (is_mcq_enabled(hba)) { > - hwq = &hba->uhq[queue_num + UFSHCD_MCQ_IO_QUEUE_OFFSET]; > + hwq = &hba->uhq[queue_num + dev_cmd_queues]; > > return ufshcd_mcq_poll_cqe_lock(hba, hwq); > }
On Tue, 2023-05-30 at 09:24 +0200, AngeloGioacchino Del Regno wrote: > > I imagine that MediaTek's UFS IP does not support dev_cmd_queues=1, > does it? > > In that case, this should not be a UFS module parameter, but a > setting that > you provide from ufs-mediatek instead. > > Regards, > Angelo Hi Angelo Our IP do work with dev_cmd_queues=1, but sacrifice one hwq specifically for dev commands leaving (N-1) hwq to (N) CPUs for submission doesn't make much sense. Actually I would prefer making it a dts property than module parameter, however, to match convention like `poll_queues` module parameters is chosen. Po-Wen
On 5/29/23 19:35, Po-Wen Kao wrote: > A dedicated queue for dev commands is not mandatory, hence let > UFS_MCQ_NUM_DEV_CMD_QUEUES become module parameter `dev_cmd_queues` > to allow sharing first hw queue for dev commands. > > When `dev_cmd_queues` is set to 0, the hwq 0 will be shared for I/O > requests and dev commands. In the same hwq, commands are processed based > on submission order hence might take longer to dispatch dev command > under heavy traffic. For the host with dedicated hwq for dev commands > can benefit in such scenario. The UFS core has already too many kernel module parameters. I don't think that we need more kernel module parameters. Instead of adding a new kernel module parameter, I propose to never reserve a hardware queue for device commands. Reserving a hardware queue for device commands seems wasteful to me. Thanks, Bart.
diff --git a/drivers/ufs/core/ufs-mcq.c b/drivers/ufs/core/ufs-mcq.c index 51b3c6ae781d..4ef48c84e62f 100644 --- a/drivers/ufs/core/ufs-mcq.c +++ b/drivers/ufs/core/ufs-mcq.c @@ -16,7 +16,8 @@ #define MAX_QUEUE_SUP GENMASK(7, 0) #define UFS_MCQ_MIN_RW_QUEUES 2 #define UFS_MCQ_MIN_READ_QUEUES 0 -#define UFS_MCQ_NUM_DEV_CMD_QUEUES 1 +#define UFS_MCQ_MAX_DEV_CMD_QUEUES 1 +#define UFS_MCQ_MIN_DEV_CMD_QUEUES 0 #define UFS_MCQ_MIN_POLL_QUEUES 0 #define QUEUE_EN_OFFSET 31 #define QUEUE_ID_OFFSET 16 @@ -75,6 +76,23 @@ module_param_cb(poll_queues, &poll_queue_count_ops, &poll_queues, 0644); MODULE_PARM_DESC(poll_queues, "Number of poll queues used for r/w. Default value is 1"); +static int dev_cmd_queue_count_set(const char *val, const struct kernel_param *kp) +{ + return param_set_uint_minmax(val, kp, + UFS_MCQ_MIN_DEV_CMD_QUEUES, + UFS_MCQ_MAX_DEV_CMD_QUEUES); +} + +static const struct kernel_param_ops dev_cmd_queue_count_ops = { + .set = dev_cmd_queue_count_set, + .get = param_get_uint, +}; + +unsigned int dev_cmd_queues = 1; +module_param_cb(dev_cmd_queues, &dev_cmd_queue_count_ops, &dev_cmd_queues, 0644); +MODULE_PARM_DESC(dev_cmd_queues, + "Number of queues used for dev command. Default value is 1"); + /** * ufshcd_mcq_config_mac - Set the #Max Activ Cmds. * @hba: per adapter instance @@ -109,7 +127,7 @@ struct ufs_hw_queue *ufshcd_mcq_req_to_hwq(struct ufs_hba *hba, u32 hwq = blk_mq_unique_tag_to_hwq(utag); /* uhq[0] is used to serve device commands */ - return &hba->uhq[hwq + UFSHCD_MCQ_IO_QUEUE_OFFSET]; + return &hba->uhq[hwq + dev_cmd_queues]; } /** @@ -153,7 +171,7 @@ static int ufshcd_mcq_config_nr_queues(struct ufs_hba *hba) /* maxq is 0 based value */ hba_maxq = FIELD_GET(MAX_QUEUE_SUP, hba->mcq_capabilities) + 1; - tot_queues = UFS_MCQ_NUM_DEV_CMD_QUEUES + read_queues + poll_queues + + tot_queues = dev_cmd_queues + read_queues + poll_queues + rw_queues; if (hba_maxq < tot_queues) { @@ -162,7 +180,7 @@ static int ufshcd_mcq_config_nr_queues(struct ufs_hba *hba) return -EOPNOTSUPP; } - rem = hba_maxq - UFS_MCQ_NUM_DEV_CMD_QUEUES; + rem = hba_maxq - dev_cmd_queues; if (rw_queues) { hba->nr_queues[HCTX_TYPE_DEFAULT] = rw_queues; @@ -188,7 +206,7 @@ static int ufshcd_mcq_config_nr_queues(struct ufs_hba *hba) for (i = 0; i < HCTX_MAX_TYPES; i++) host->nr_hw_queues += hba->nr_queues[i]; - hba->nr_hw_queues = host->nr_hw_queues + UFS_MCQ_NUM_DEV_CMD_QUEUES; + hba->nr_hw_queues = host->nr_hw_queues + dev_cmd_queues; return 0; } @@ -424,8 +442,11 @@ int ufshcd_mcq_init(struct ufs_hba *hba) /* The very first HW queue serves device commands */ hba->dev_cmd_queue = &hba->uhq[0]; - /* Give dev_cmd_queue the minimal number of entries */ - hba->dev_cmd_queue->max_entries = MAX_DEV_CMD_ENTRIES; + if (dev_cmd_queues) { + /* Give dedicated dev_cmd_queue the minimal number of entries */ + hba->dev_cmd_queue->max_entries = MAX_DEV_CMD_ENTRIES; + } + host->host_tagset = 1; return 0; diff --git a/drivers/ufs/core/ufshcd-priv.h b/drivers/ufs/core/ufshcd-priv.h index d53b93c21a0c..b490d645f12c 100644 --- a/drivers/ufs/core/ufshcd-priv.h +++ b/drivers/ufs/core/ufshcd-priv.h @@ -78,7 +78,6 @@ struct ufs_hw_queue *ufshcd_mcq_req_to_hwq(struct ufs_hba *hba, unsigned long ufshcd_mcq_poll_cqe_lock(struct ufs_hba *hba, struct ufs_hw_queue *hwq); -#define UFSHCD_MCQ_IO_QUEUE_OFFSET 1 #define SD_ASCII_STD true #define SD_RAW false int ufshcd_read_string_desc(struct ufs_hba *hba, u8 desc_index, @@ -287,6 +286,7 @@ static inline int ufshcd_mcq_vops_config_esi(struct ufs_hba *hba) return -EOPNOTSUPP; } +extern unsigned int dev_cmd_queues; extern const struct ufs_pm_lvl_states ufs_pm_lvl_states[]; /** diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c index 4ec8dacb447c..6361e21ca1c5 100644 --- a/drivers/ufs/core/ufshcd.c +++ b/drivers/ufs/core/ufshcd.c @@ -5493,7 +5493,7 @@ static int ufshcd_poll(struct Scsi_Host *shost, unsigned int queue_num) struct ufs_hw_queue *hwq; if (is_mcq_enabled(hba)) { - hwq = &hba->uhq[queue_num + UFSHCD_MCQ_IO_QUEUE_OFFSET]; + hwq = &hba->uhq[queue_num + dev_cmd_queues]; return ufshcd_mcq_poll_cqe_lock(hba, hwq); }
A dedicated queue for dev commands is not mandatory, hence let UFS_MCQ_NUM_DEV_CMD_QUEUES become module parameter `dev_cmd_queues` to allow sharing first hw queue for dev commands. When `dev_cmd_queues` is set to 0, the hwq 0 will be shared for I/O requests and dev commands. In the same hwq, commands are processed based on submission order hence might take longer to dispatch dev command under heavy traffic. For the host with dedicated hwq for dev commands can benefit in such scenario. Signed-off-by: Po-Wen Kao <powen.kao@mediatek.com> --- drivers/ufs/core/ufs-mcq.c | 35 +++++++++++++++++++++++++++------- drivers/ufs/core/ufshcd-priv.h | 2 +- drivers/ufs/core/ufshcd.c | 2 +- 3 files changed, 30 insertions(+), 9 deletions(-)