| 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