| Message ID | 1644291974-73531-1-git-send-email-kanie@linux.alibaba.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | scsi: target: tcmu: Make cmd_ring_size changeable via configfs. | expand |
On 08.02.22 04:46, Guixin Liu wrote: > Make cmd_ring_size changeable similar to the way it is done for > max_data_area_mb, the reason is that our tcmu client will create > thousands of tcmu instances, and this will consume lots of mem with > default 8Mb cmd ring size for every backstore. > > One can change the value by typing: > echo "cmd_ring_size_mb=N" > control > The "N" is a integer between 1 to 8, if set 1, the cmd ring can hold > about 6k cmds(tcmu_cmd_entry about 176 byte) at least. > > The value is printed when doing: > cat info > In addition, a new readonly attribute 'cmd_ring_size_mb' returns the > value in read. Thank you for the patch. This was on my todo list also. > > Signed-off-by: Guixin Liu <kanie@linux.alibaba.com> > --- > drivers/target/target_core_user.c | 64 ++++++++++++++++++++++++++++++++++++--- > 1 file changed, 59 insertions(+), 5 deletions(-) > > diff --git a/drivers/target/target_core_user.c b/drivers/target/target_core_user.c > index 7b2a89a..826c1c0 100644 > --- a/drivers/target/target_core_user.c > +++ b/drivers/target/target_core_user.c > @@ -64,7 +64,6 @@ > #define MB_CMDR_SIZE (8 * 1024 * 1024) I'd like to change this to MB_CMDR_SIZE_DEF, since it no longer is the fixed size of mailbox plus cmd ring, but the default only. > /* Offset of cmd ring is size of mailbox */ > #define CMDR_OFF sizeof(struct tcmu_mailbox) > -#define CMDR_SIZE (MB_CMDR_SIZE - CMDR_OFF) Similarly this could become CMDR_SIZE_DEF > > /* > * For data area, the default block size is PAGE_SIZE and > @@ -133,6 +132,7 @@ struct tcmu_dev { > struct tcmu_mailbox *mb_addr; > void *cmdr; > u32 cmdr_size; > + u32 total_cmdr_size_byte; Do we really need a new field in tcmu_dev? I think we should avoid it since the field cmdr_size holds the same value, just lowered by CMDR_OFF. > u32 cmdr_last_cleaned; > /* Offset of data area from start of mb */ > /* Must add data_off and mb_addr to get the address */ > @@ -1617,6 +1617,7 @@ static struct se_device *tcmu_alloc_device(struct se_hba *hba, const char *name) > > udev->data_pages_per_blk = DATA_PAGES_PER_BLK_DEF; > udev->max_blocks = DATA_AREA_PAGES_DEF / udev->data_pages_per_blk; > + udev->total_cmdr_size_byte = MB_CMDR_SIZE; We could do udev->cmdr_size = CMDR_SIZE_DEF; > udev->data_area_mb = TCMU_PAGES_TO_MBS(DATA_AREA_PAGES_DEF); > > mutex_init(&udev->cmdr_lock); > @@ -2189,7 +2190,7 @@ static int tcmu_configure_device(struct se_device *dev) > goto err_bitmap_alloc; > } > > - mb = vzalloc(MB_CMDR_SIZE); > + mb = vzalloc(udev->total_cmdr_size_byte); > if (!mb) { > ret = -ENOMEM; > goto err_vzalloc; > @@ -2198,8 +2199,8 @@ static int tcmu_configure_device(struct se_device *dev) > /* mailbox fits in first part of CMDR space */ > udev->mb_addr = mb; > udev->cmdr = (void *)mb + CMDR_OFF; > - udev->cmdr_size = CMDR_SIZE; > - udev->data_off = MB_CMDR_SIZE; > + udev->cmdr_size = udev->total_cmdr_size_byte - CMDR_OFF; > + udev->data_off = udev->total_cmdr_size_byte; CMDR_SIZE would be already set here, so we only would need: udev->data_off = udev->cmdr_size + CMDR_OFF; > data_size = TCMU_MBS_TO_PAGES(udev->data_area_mb) << PAGE_SHIFT; > udev->mmap_pages = (data_size + MB_CMDR_SIZE) >> PAGE_SHIFT; I think we have to replace MB_CMDR_SIZE with udev->cmdr_size + CMDR_OFF here. Just a few lines below your patch also does not change the line info->mem[0].size = data_size + MB_CMDR_SIZE; where the same replacement is needed. > udev->data_blk_size = udev->data_pages_per_blk * PAGE_SIZE; > @@ -2401,7 +2402,7 @@ static void tcmu_reset_ring(struct tcmu_dev *udev, u8 err_level) > enum { > Opt_dev_config, Opt_dev_size, Opt_hw_block_size, Opt_hw_max_sectors, > Opt_nl_reply_supported, Opt_max_data_area_mb, Opt_data_pages_per_blk, > - Opt_err, > + Opt_cmd_ring_size_mb, Opt_err, > }; > > static match_table_t tokens = { > @@ -2412,6 +2413,7 @@ enum { > {Opt_nl_reply_supported, "nl_reply_supported=%d"}, > {Opt_max_data_area_mb, "max_data_area_mb=%d"}, > {Opt_data_pages_per_blk, "data_pages_per_blk=%d"}, > + {Opt_cmd_ring_size_mb, "cmd_ring_size_mb=%d"}, > {Opt_err, NULL} > }; > > @@ -2509,6 +2511,41 @@ static int tcmu_set_data_pages_per_blk(struct tcmu_dev *udev, substring_t *arg) > return ret; > } > > +static int tcmu_set_cmd_ring_size_param(struct tcmu_dev *udev, substring_t *arg) Please remove the "_param" suffix from function name, as the other similar set functions don't have it also. > +{ > + int val, ret; > + > + ret = match_int(arg, &val); > + if (ret < 0) { > + pr_err("match_int() failed for cmd_ring_size_mb=. Error %d.\n", > + ret); > + return ret; > + } > + > + if (val <= 0) { > + pr_err("Invalid cmd_ring_size_mb %d.\n", val); > + return -EINVAL; > + } > + > + mutex_lock(&udev->cmdr_lock); > + if (udev->data_bitmap) { > + pr_err("Cannot set cmd_ring_size_mb after it has been enabled.\n"); > + ret = -EINVAL; > + goto unlock; > + } > + > + udev->total_cmdr_size_byte = (val << 20); > + if (udev->total_cmdr_size_byte > MB_CMDR_SIZE) { Would be udev->cmdr_size = (val << 20) - CMDR_OFF; if (val > (MB_CMDR_SIZE_DEF >> 20)) { Please note: the check you coded is definitely wrong. E.g. if someone entered the value 4096, which is 0x1000, we would have ended up with udev->total_cmdr_size_byte set to 0 ... > + pr_err("%d is too large. Adjusting cmd_ring_size_mb to global limit of %u\n", > + val, (MB_CMDR_SIZE >> 20)); > + udev->total_cmdr_size_byte = MB_CMDR_SIZE; udev->cmdr_size = CMDR_SIZE_DEF; > + } > + > +unlock: > + mutex_unlock(&udev->cmdr_lock); > + return ret; > +} > + > static ssize_t tcmu_set_configfs_dev_params(struct se_device *dev, > const char *page, ssize_t count) > { > @@ -2563,6 +2600,9 @@ static ssize_t tcmu_set_configfs_dev_params(struct se_device *dev, > case Opt_data_pages_per_blk: > ret = tcmu_set_data_pages_per_blk(udev, &args[0]); > break; > + case Opt_cmd_ring_size_mb: > + ret = tcmu_set_cmd_ring_size_param(udev, &args[0]); > + break; > default: > break; > } > @@ -2585,6 +2625,8 @@ static ssize_t tcmu_show_configfs_dev_params(struct se_device *dev, char *b) > bl += sprintf(b + bl, "Size: %llu ", udev->dev_size); > bl += sprintf(b + bl, "MaxDataAreaMB: %u ", udev->data_area_mb); > bl += sprintf(b + bl, "DataPagesPerBlk: %u\n", udev->data_pages_per_blk); I think we should print all the config values on one line. So I'd like to remove the '\n' after DataPagesPerBlk. > + bl += sprintf(b + bl, "CmdRingSizeMB: %u\n", > + (udev->total_cmdr_size_byte >> 20)); > > return bl; > } > @@ -3059,6 +3101,17 @@ static ssize_t tcmu_free_kept_buf_store(struct config_item *item, const char *pa > } > CONFIGFS_ATTR_WO(tcmu_, free_kept_buf); > > +static ssize_t tcmu_cmd_ring_size_mb_show(struct config_item *item, char *page) > +{ > + struct se_dev_attrib *da = container_of(to_config_group(item), > + struct se_dev_attrib, da_group); > + struct tcmu_dev *udev = TCMU_DEV(da->da_dev); > + > + return snprintf(page, PAGE_SIZE, "%u\n", > + (udev->total_cmdr_size_byte >> 20)); (udev->cmdr_size + CMDR_OFF) >> 20); > +} > +CONFIGFS_ATTR_RO(tcmu_, cmd_ring_size_mb); > + The attributes block_dev, reset_ring and free_kept_buf belong to the separate action attribute group. So shouldn't we shift this new attribute up in the code, e.q. behind the line "CONFIGFS_ATTR_RO(tcmu_, data_pages_per_blk);"? > static struct configfs_attribute *tcmu_attrib_attrs[] = { > &tcmu_attr_cmd_time_out, > &tcmu_attr_qfull_time_out, > @@ -3069,6 +3122,7 @@ static ssize_t tcmu_free_kept_buf_store(struct config_item *item, const char *pa > &tcmu_attr_emulate_write_cache, > &tcmu_attr_tmr_notification, > &tcmu_attr_nl_reply_supported, > + &tcmu_attr_cmd_ring_size_mb, > NULL, > }; >
diff --git a/drivers/target/target_core_user.c b/drivers/target/target_core_user.c index 7b2a89a..826c1c0 100644 --- a/drivers/target/target_core_user.c +++ b/drivers/target/target_core_user.c @@ -64,7 +64,6 @@ #define MB_CMDR_SIZE (8 * 1024 * 1024) /* Offset of cmd ring is size of mailbox */ #define CMDR_OFF sizeof(struct tcmu_mailbox) -#define CMDR_SIZE (MB_CMDR_SIZE - CMDR_OFF) /* * For data area, the default block size is PAGE_SIZE and @@ -133,6 +132,7 @@ struct tcmu_dev { struct tcmu_mailbox *mb_addr; void *cmdr; u32 cmdr_size; + u32 total_cmdr_size_byte; u32 cmdr_last_cleaned; /* Offset of data area from start of mb */ /* Must add data_off and mb_addr to get the address */ @@ -1617,6 +1617,7 @@ static struct se_device *tcmu_alloc_device(struct se_hba *hba, const char *name) udev->data_pages_per_blk = DATA_PAGES_PER_BLK_DEF; udev->max_blocks = DATA_AREA_PAGES_DEF / udev->data_pages_per_blk; + udev->total_cmdr_size_byte = MB_CMDR_SIZE; udev->data_area_mb = TCMU_PAGES_TO_MBS(DATA_AREA_PAGES_DEF); mutex_init(&udev->cmdr_lock); @@ -2189,7 +2190,7 @@ static int tcmu_configure_device(struct se_device *dev) goto err_bitmap_alloc; } - mb = vzalloc(MB_CMDR_SIZE); + mb = vzalloc(udev->total_cmdr_size_byte); if (!mb) { ret = -ENOMEM; goto err_vzalloc; @@ -2198,8 +2199,8 @@ static int tcmu_configure_device(struct se_device *dev) /* mailbox fits in first part of CMDR space */ udev->mb_addr = mb; udev->cmdr = (void *)mb + CMDR_OFF; - udev->cmdr_size = CMDR_SIZE; - udev->data_off = MB_CMDR_SIZE; + udev->cmdr_size = udev->total_cmdr_size_byte - CMDR_OFF; + udev->data_off = udev->total_cmdr_size_byte; data_size = TCMU_MBS_TO_PAGES(udev->data_area_mb) << PAGE_SHIFT; udev->mmap_pages = (data_size + MB_CMDR_SIZE) >> PAGE_SHIFT; udev->data_blk_size = udev->data_pages_per_blk * PAGE_SIZE; @@ -2401,7 +2402,7 @@ static void tcmu_reset_ring(struct tcmu_dev *udev, u8 err_level) enum { Opt_dev_config, Opt_dev_size, Opt_hw_block_size, Opt_hw_max_sectors, Opt_nl_reply_supported, Opt_max_data_area_mb, Opt_data_pages_per_blk, - Opt_err, + Opt_cmd_ring_size_mb, Opt_err, }; static match_table_t tokens = { @@ -2412,6 +2413,7 @@ enum { {Opt_nl_reply_supported, "nl_reply_supported=%d"}, {Opt_max_data_area_mb, "max_data_area_mb=%d"}, {Opt_data_pages_per_blk, "data_pages_per_blk=%d"}, + {Opt_cmd_ring_size_mb, "cmd_ring_size_mb=%d"}, {Opt_err, NULL} }; @@ -2509,6 +2511,41 @@ static int tcmu_set_data_pages_per_blk(struct tcmu_dev *udev, substring_t *arg) return ret; } +static int tcmu_set_cmd_ring_size_param(struct tcmu_dev *udev, substring_t *arg) +{ + int val, ret; + + ret = match_int(arg, &val); + if (ret < 0) { + pr_err("match_int() failed for cmd_ring_size_mb=. Error %d.\n", + ret); + return ret; + } + + if (val <= 0) { + pr_err("Invalid cmd_ring_size_mb %d.\n", val); + return -EINVAL; + } + + mutex_lock(&udev->cmdr_lock); + if (udev->data_bitmap) { + pr_err("Cannot set cmd_ring_size_mb after it has been enabled.\n"); + ret = -EINVAL; + goto unlock; + } + + udev->total_cmdr_size_byte = (val << 20); + if (udev->total_cmdr_size_byte > MB_CMDR_SIZE) { + pr_err("%d is too large. Adjusting cmd_ring_size_mb to global limit of %u\n", + val, (MB_CMDR_SIZE >> 20)); + udev->total_cmdr_size_byte = MB_CMDR_SIZE; + } + +unlock: + mutex_unlock(&udev->cmdr_lock); + return ret; +} + static ssize_t tcmu_set_configfs_dev_params(struct se_device *dev, const char *page, ssize_t count) { @@ -2563,6 +2600,9 @@ static ssize_t tcmu_set_configfs_dev_params(struct se_device *dev, case Opt_data_pages_per_blk: ret = tcmu_set_data_pages_per_blk(udev, &args[0]); break; + case Opt_cmd_ring_size_mb: + ret = tcmu_set_cmd_ring_size_param(udev, &args[0]); + break; default: break; } @@ -2585,6 +2625,8 @@ static ssize_t tcmu_show_configfs_dev_params(struct se_device *dev, char *b) bl += sprintf(b + bl, "Size: %llu ", udev->dev_size); bl += sprintf(b + bl, "MaxDataAreaMB: %u ", udev->data_area_mb); bl += sprintf(b + bl, "DataPagesPerBlk: %u\n", udev->data_pages_per_blk); + bl += sprintf(b + bl, "CmdRingSizeMB: %u\n", + (udev->total_cmdr_size_byte >> 20)); return bl; } @@ -3059,6 +3101,17 @@ static ssize_t tcmu_free_kept_buf_store(struct config_item *item, const char *pa } CONFIGFS_ATTR_WO(tcmu_, free_kept_buf); +static ssize_t tcmu_cmd_ring_size_mb_show(struct config_item *item, char *page) +{ + struct se_dev_attrib *da = container_of(to_config_group(item), + struct se_dev_attrib, da_group); + struct tcmu_dev *udev = TCMU_DEV(da->da_dev); + + return snprintf(page, PAGE_SIZE, "%u\n", + (udev->total_cmdr_size_byte >> 20)); +} +CONFIGFS_ATTR_RO(tcmu_, cmd_ring_size_mb); + static struct configfs_attribute *tcmu_attrib_attrs[] = { &tcmu_attr_cmd_time_out, &tcmu_attr_qfull_time_out, @@ -3069,6 +3122,7 @@ static ssize_t tcmu_free_kept_buf_store(struct config_item *item, const char *pa &tcmu_attr_emulate_write_cache, &tcmu_attr_tmr_notification, &tcmu_attr_nl_reply_supported, + &tcmu_attr_cmd_ring_size_mb, NULL, };
Make cmd_ring_size changeable similar to the way it is done for max_data_area_mb, the reason is that our tcmu client will create thousands of tcmu instances, and this will consume lots of mem with default 8Mb cmd ring size for every backstore. One can change the value by typing: echo "cmd_ring_size_mb=N" > control The "N" is a integer between 1 to 8, if set 1, the cmd ring can hold about 6k cmds(tcmu_cmd_entry about 176 byte) at least. The value is printed when doing: cat info In addition, a new readonly attribute 'cmd_ring_size_mb' returns the value in read. Signed-off-by: Guixin Liu <kanie@linux.alibaba.com> --- drivers/target/target_core_user.c | 64 ++++++++++++++++++++++++++++++++++++--- 1 file changed, 59 insertions(+), 5 deletions(-)