lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Message-ID: <0428d955-e9c7-b632-cb19-102e49cef387@gmail.com> Date: Sat, 12 Feb 2022 12:25:33 +0100 From: Bodo Stroesser <bostroesser@...il.com> To: Guixin Liu <kanie@...ux.alibaba.com>, martin.petersen@...cle.com Cc: xiaoguang.wang@...ux.alibaba.com, xlpang@...ux.alibaba.com, linux-scsi@...r.kernel.org, target-devel@...r.kernel.org, linux-kernel@...r.kernel.org Subject: Re: [PATCH] scsi: target: tcmu: Make cmd_ring_size changeable via configfs. 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@...ux.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, > }; >
Powered by blists - more mailing lists