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.
Reviewed-by: Xiaoguang Wang <[email protected]>
Signed-off-by: Guixin Liu <[email protected]>
---
drivers/target/target_core_user.c | 71 ++++++++++++++++++++++++++++++++++-----
1 file changed, 62 insertions(+), 9 deletions(-)
diff --git a/drivers/target/target_core_user.c b/drivers/target/target_core_user.c
index 7b2a89a..0a546db 100644
--- a/drivers/target/target_core_user.c
+++ b/drivers/target/target_core_user.c
@@ -61,10 +61,10 @@
#define TCMU_TIME_OUT (30 * MSEC_PER_SEC)
/* For mailbox plus cmd ring, the size is fixed 8MB */
-#define MB_CMDR_SIZE (8 * 1024 * 1024)
+#define MB_CMDR_SIZE_DEF (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)
+#define CMDR_SIZE_DEF (MB_CMDR_SIZE_DEF - CMDR_OFF)
/*
* For data area, the default block size is PAGE_SIZE and
@@ -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->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->cmdr_size + CMDR_OFF);
if (!mb) {
ret = -ENOMEM;
goto err_vzalloc;
@@ -2198,10 +2199,9 @@ 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->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;
+ udev->mmap_pages = (data_size + udev->cmdr_size + CMDR_OFF) >> PAGE_SHIFT;
udev->data_blk_size = udev->data_pages_per_blk * PAGE_SIZE;
udev->dbi_thresh = 0; /* Default in Idle state */
@@ -2221,7 +2221,7 @@ static int tcmu_configure_device(struct se_device *dev)
info->mem[0].name = "tcm-user command & data buffer";
info->mem[0].addr = (phys_addr_t)(uintptr_t)udev->mb_addr;
- info->mem[0].size = data_size + MB_CMDR_SIZE;
+ info->mem[0].size = data_size + udev->cmdr_size + CMDR_OFF;
info->mem[0].memtype = UIO_MEM_NONE;
info->irqcontrol = tcmu_irqcontrol;
@@ -2401,7 +2401,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 +2412,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 +2510,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(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->cmdr_size = (val << 20) - CMDR_OFF;
+ if (val > (MB_CMDR_SIZE_DEF >> 20)) {
+ pr_err("%d is too large. Adjusting cmd_ring_size_mb to global limit of %u\n",
+ val, (MB_CMDR_SIZE_DEF >> 20));
+ 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 +2599,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(udev, &args[0]);
+ break;
default:
break;
}
@@ -2584,7 +2623,9 @@ static ssize_t tcmu_show_configfs_dev_params(struct se_device *dev, char *b)
udev->dev_config[0] ? udev->dev_config : "NULL");
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, "DataPagesPerBlk: %u", udev->data_pages_per_blk);
+ bl += sprintf(b + bl, "CmdRingSizeMB: %u\n",
+ (udev->cmdr_size + CMDR_OFF) >> 20);
return bl;
}
@@ -2693,6 +2734,17 @@ static ssize_t tcmu_data_pages_per_blk_show(struct config_item *item,
}
CONFIGFS_ATTR_RO(tcmu_, data_pages_per_blk);
+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->cmdr_size + CMDR_OFF) >> 20);
+}
+CONFIGFS_ATTR_RO(tcmu_, cmd_ring_size_mb);
+
static ssize_t tcmu_dev_config_show(struct config_item *item, char *page)
{
struct se_dev_attrib *da = container_of(to_config_group(item),
@@ -3064,6 +3116,7 @@ static ssize_t tcmu_free_kept_buf_store(struct config_item *item, const char *pa
&tcmu_attr_qfull_time_out,
&tcmu_attr_max_data_area_mb,
&tcmu_attr_data_pages_per_blk,
+ &tcmu_attr_cmd_ring_size_mb,
&tcmu_attr_dev_config,
&tcmu_attr_dev_size,
&tcmu_attr_emulate_write_cache,
--
1.8.3.1
Hi Guixin,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on mkp-scsi/for-next]
[also build test WARNING on v5.17-rc4 next-20220215]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Guixin-Liu/scsi-target-tcmu-Make-cmd_ring_size-changeable-via-configfs/20220215-160505
base: https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git for-next
config: x86_64-allyesconfig (https://download.01.org/0day-ci/archive/20220215/[email protected]/config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build):
# https://github.com/0day-ci/linux/commit/7f77700542b8196c546ef10656dda7a107d8d1ad
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Guixin-Liu/scsi-target-tcmu-Make-cmd_ring_size-changeable-via-configfs/20220215-160505
git checkout 7f77700542b8196c546ef10656dda7a107d8d1ad
# save the config file to linux build tree
mkdir build_dir
make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/target/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All warnings (new ones prefixed by >>):
drivers/target/target_core_user.c: In function 'tcmu_show_configfs_dev_params':
>> drivers/target/target_core_user.c:2627:41: warning: format '%u' expects argument of type 'unsigned int', but argument 3 has type 'long unsigned int' [-Wformat=]
2627 | bl += sprintf(b + bl, "CmdRingSizeMB: %u\n",
| ~^
| |
| unsigned int
| %lu
2628 | (udev->cmdr_size + CMDR_OFF) >> 20);
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| |
| long unsigned int
drivers/target/target_core_user.c: In function 'tcmu_cmd_ring_size_mb_show':
drivers/target/target_core_user.c:2743:37: warning: format '%u' expects argument of type 'unsigned int', but argument 4 has type 'long unsigned int' [-Wformat=]
2743 | return snprintf(page, PAGE_SIZE, "%u\n",
| ~^
| |
| unsigned int
| %lu
2744 | (udev->cmdr_size + CMDR_OFF) >> 20);
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| |
| long unsigned int
vim +2627 drivers/target/target_core_user.c
2616
2617 static ssize_t tcmu_show_configfs_dev_params(struct se_device *dev, char *b)
2618 {
2619 struct tcmu_dev *udev = TCMU_DEV(dev);
2620 ssize_t bl = 0;
2621
2622 bl = sprintf(b + bl, "Config: %s ",
2623 udev->dev_config[0] ? udev->dev_config : "NULL");
2624 bl += sprintf(b + bl, "Size: %llu ", udev->dev_size);
2625 bl += sprintf(b + bl, "MaxDataAreaMB: %u ", udev->data_area_mb);
2626 bl += sprintf(b + bl, "DataPagesPerBlk: %u", udev->data_pages_per_blk);
> 2627 bl += sprintf(b + bl, "CmdRingSizeMB: %u\n",
2628 (udev->cmdr_size + CMDR_OFF) >> 20);
2629
2630 return bl;
2631 }
2632
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
Hi Guixin,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on mkp-scsi/for-next]
[also build test WARNING on v5.17-rc4 next-20220215]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Guixin-Liu/scsi-target-tcmu-Make-cmd_ring_size-changeable-via-configfs/20220215-160505
base: https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git for-next
config: x86_64-randconfig-a005-20220214 (https://download.01.org/0day-ci/archive/20220215/[email protected]/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 37f422f4ac31c8b8041c6b62065263314282dab6)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/7f77700542b8196c546ef10656dda7a107d8d1ad
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Guixin-Liu/scsi-target-tcmu-Make-cmd_ring_size-changeable-via-configfs/20220215-160505
git checkout 7f77700542b8196c546ef10656dda7a107d8d1ad
# save the config file to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/target/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All warnings (new ones prefixed by >>):
>> drivers/target/target_core_user.c:2628:9: warning: format specifies type 'unsigned int' but the argument has type 'unsigned long' [-Wformat]
(udev->cmdr_size + CMDR_OFF) >> 20);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/target/target_core_user.c:2744:4: warning: format specifies type 'unsigned int' but the argument has type 'unsigned long' [-Wformat]
(udev->cmdr_size + CMDR_OFF) >> 20);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
2 warnings generated.
vim +2628 drivers/target/target_core_user.c
2616
2617 static ssize_t tcmu_show_configfs_dev_params(struct se_device *dev, char *b)
2618 {
2619 struct tcmu_dev *udev = TCMU_DEV(dev);
2620 ssize_t bl = 0;
2621
2622 bl = sprintf(b + bl, "Config: %s ",
2623 udev->dev_config[0] ? udev->dev_config : "NULL");
2624 bl += sprintf(b + bl, "Size: %llu ", udev->dev_size);
2625 bl += sprintf(b + bl, "MaxDataAreaMB: %u ", udev->data_area_mb);
2626 bl += sprintf(b + bl, "DataPagesPerBlk: %u", udev->data_pages_per_blk);
2627 bl += sprintf(b + bl, "CmdRingSizeMB: %u\n",
> 2628 (udev->cmdr_size + CMDR_OFF) >> 20);
2629
2630 return bl;
2631 }
2632
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
Hi Liu,
since CMDR_OFF is defined as "sizeof(struct tcmu_mailbox)", we could
fix the warning by using %z. OTOH, the fields cmdr_off and cmdr_size
in struct tcmu_mailbox are defined as u32, as well as cmdr_size in
struct tcmu_dev.
So I think we should add a cast to u32 in the definition of CMDR_OFF.
That should allow us to use %u on all architectures. Additionally
it avoids expansion to long during calculations where CMDR_OFF is
involved.
Btw: in my comments I asked you to remove the "\n" in the sprintf for
DataPagesPerBlk. But of course we need a space instead to allow
proper parsing of the output.
Bodo
On 15.02.22 14:05, kernel test robot wrote:
> Hi Guixin,
>
> Thank you for the patch! Perhaps something to improve:
>
> [auto build test WARNING on mkp-scsi/for-next]
> [also build test WARNING on v5.17-rc4 next-20220215]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch]
>
> url: https://github.com/0day-ci/linux/commits/Guixin-Liu/scsi-target-tcmu-Make-cmd_ring_size-changeable-via-configfs/20220215-160505
> base: https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git for-next
> config: x86_64-allyesconfig (https://download.01.org/0day-ci/archive/20220215/[email protected]/config)
> compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
> reproduce (this is a W=1 build):
> # https://github.com/0day-ci/linux/commit/7f77700542b8196c546ef10656dda7a107d8d1ad
> git remote add linux-review https://github.com/0day-ci/linux
> git fetch --no-tags linux-review Guixin-Liu/scsi-target-tcmu-Make-cmd_ring_size-changeable-via-configfs/20220215-160505
> git checkout 7f77700542b8196c546ef10656dda7a107d8d1ad
> # save the config file to linux build tree
> mkdir build_dir
> make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/target/
>
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <[email protected]>
>
> All warnings (new ones prefixed by >>):
>
> drivers/target/target_core_user.c: In function 'tcmu_show_configfs_dev_params':
>>> drivers/target/target_core_user.c:2627:41: warning: format '%u' expects argument of type 'unsigned int', but argument 3 has type 'long unsigned int' [-Wformat=]
> 2627 | bl += sprintf(b + bl, "CmdRingSizeMB: %u\n",
> | ~^
> | |
> | unsigned int
> | %lu
> 2628 | (udev->cmdr_size + CMDR_OFF) >> 20);
> | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> | |
> | long unsigned int
> drivers/target/target_core_user.c: In function 'tcmu_cmd_ring_size_mb_show':
> drivers/target/target_core_user.c:2743:37: warning: format '%u' expects argument of type 'unsigned int', but argument 4 has type 'long unsigned int' [-Wformat=]
> 2743 | return snprintf(page, PAGE_SIZE, "%u\n",
> | ~^
> | |
> | unsigned int
> | %lu
> 2744 | (udev->cmdr_size + CMDR_OFF) >> 20);
> | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> | |
> | long unsigned int
>
>
> vim +2627 drivers/target/target_core_user.c
>
> 2616
> 2617 static ssize_t tcmu_show_configfs_dev_params(struct se_device *dev, char *b)
> 2618 {
> 2619 struct tcmu_dev *udev = TCMU_DEV(dev);
> 2620 ssize_t bl = 0;
> 2621
> 2622 bl = sprintf(b + bl, "Config: %s ",
> 2623 udev->dev_config[0] ? udev->dev_config : "NULL");
> 2624 bl += sprintf(b + bl, "Size: %llu ", udev->dev_size);
> 2625 bl += sprintf(b + bl, "MaxDataAreaMB: %u ", udev->data_area_mb);
> 2626 bl += sprintf(b + bl, "DataPagesPerBlk: %u", udev->data_pages_per_blk);
>> 2627 bl += sprintf(b + bl, "CmdRingSizeMB: %u\n",
> 2628 (udev->cmdr_size + CMDR_OFF) >> 20);
> 2629
> 2630 return bl;
> 2631 }
> 2632
>
> ---
> 0-DAY CI Kernel Test Service, Intel Corporation
> https://lists.01.org/hyperkitty/list/[email protected]