Re: [PATCH 3/8] vhost scsi: alloc cmds per vq instead of session
On Mon, Sep 21, 2020 at 01:23:03PM -0500, Mike Christie wrote:
> We currently are limited to 256 cmds per session. This leads to problems
> where if the user has increased virtqueue_size to more than 2 or
> cmd_per_lun to more than 256 vhost_scsi_get_tag can fail and the guest
> will get IO errors.
>
> This patch moves the cmd allocation to per vq so we can easily match
> whatever the user has specified for num_queues and
> virtqueue_size/cmd_per_lun. It also makes it easier to control how much
> memory we preallocate. For cases, where perf is not as important and
> we can use the current defaults (1 vq and 128 cmds per vq) memory use
> from preallocate cmds is cut in half. For cases, where we are willing
> to use more memory for higher perf, cmd mem use will now increase as
> the num queues and queue depth increases.
>
> Signed-off-by: Mike Christie
> ---
> drivers/vhost/scsi.c | 204
> ---
> 1 file changed, 127 insertions(+), 77 deletions(-)
>
> diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
> index b22adf0..13311b8 100644
> --- a/drivers/vhost/scsi.c
> +++ b/drivers/vhost/scsi.c
> @@ -52,7 +52,6 @@
> #define VHOST_SCSI_VERSION "v0.1"
> #define VHOST_SCSI_NAMELEN 256
> #define VHOST_SCSI_MAX_CDB_SIZE 32
> -#define VHOST_SCSI_DEFAULT_TAGS 256
> #define VHOST_SCSI_PREALLOC_SGLS 2048
> #define VHOST_SCSI_PREALLOC_UPAGES 2048
> #define VHOST_SCSI_PREALLOC_PROT_SGLS 2048
> @@ -189,6 +188,9 @@ struct vhost_scsi_virtqueue {
>* Writers must also take dev mutex and flush under it.
>*/
> int inflight_idx;
> + struct vhost_scsi_cmd *scsi_cmds;
> + struct sbitmap scsi_tags;
> + int max_cmds;
> };
>
> struct vhost_scsi {
> @@ -324,7 +326,9 @@ static void vhost_scsi_release_cmd(struct se_cmd *se_cmd)
> {
> struct vhost_scsi_cmd *tv_cmd = container_of(se_cmd,
> struct vhost_scsi_cmd, tvc_se_cmd);
> - struct se_session *se_sess = tv_cmd->tvc_nexus->tvn_se_sess;
> + struct vhost_scsi_virtqueue *svq = container_of(tv_cmd->tvc_vq,
> + struct vhost_scsi_virtqueue, vq);
> + struct vhost_scsi_inflight *inflight = tv_cmd->inflight;
> int i;
>
> if (tv_cmd->tvc_sgl_count) {
> @@ -336,8 +340,8 @@ static void vhost_scsi_release_cmd(struct se_cmd *se_cmd)
> put_page(sg_page(&tv_cmd->tvc_prot_sgl[i]));
> }
>
> - vhost_scsi_put_inflight(tv_cmd->inflight);
> - target_free_tag(se_sess, se_cmd);
> + sbitmap_clear_bit(&svq->scsi_tags, se_cmd->map_tag);
> + vhost_scsi_put_inflight(inflight);
> }
>
> static u32 vhost_scsi_sess_get_index(struct se_session *se_sess)
> @@ -566,13 +570,14 @@ static void vhost_scsi_complete_cmd_work(struct
> vhost_work *work)
> }
>
> static struct vhost_scsi_cmd *
> -vhost_scsi_get_tag(struct vhost_virtqueue *vq, struct vhost_scsi_tpg *tpg,
> +vhost_scsi_get_cmd(struct vhost_virtqueue *vq, struct vhost_scsi_tpg *tpg,
> unsigned char *cdb, u64 scsi_tag, u16 lun, u8 task_attr,
> u32 exp_data_len, int data_direction)
> {
> + struct vhost_scsi_virtqueue *svq = container_of(vq,
> + struct vhost_scsi_virtqueue, vq);
> struct vhost_scsi_cmd *cmd;
> struct vhost_scsi_nexus *tv_nexus;
> - struct se_session *se_sess;
> struct scatterlist *sg, *prot_sg;
> struct page **pages;
> int tag, cpu;
> @@ -582,15 +587,14 @@ static void vhost_scsi_complete_cmd_work(struct
> vhost_work *work)
> pr_err("Unable to locate active struct vhost_scsi_nexus\n");
> return ERR_PTR(-EIO);
> }
> - se_sess = tv_nexus->tvn_se_sess;
>
> - tag = sbitmap_queue_get(&se_sess->sess_tag_pool, &cpu);
> + tag = sbitmap_get(&svq->scsi_tags, 0, false);
> if (tag < 0) {
> pr_err("Unable to obtain tag for vhost_scsi_cmd\n");
> return ERR_PTR(-ENOMEM);
> }
After this change, cpu is uninitialized.
>
> - cmd = &((struct vhost_scsi_cmd *)se_sess->sess_cmd_map)[tag];
> + cmd = &svq->scsi_cmds[tag];
> sg = cmd->tvc_sgl;
> prot_sg = cmd->tvc_prot_sgl;
> pages = cmd->tvc_upages;
> @@ -1065,11 +1069,11 @@ static void vhost_scsi_submission_work(struct
> work_struct *work)
> scsi_command_size(cdb),
> VHOST_SCSI_MAX_CDB_SIZE);
> goto err;
> }
> - cmd = vhost_scsi_get_tag(vq, tpg, cdb, tag, lun, task_attr,
> + cmd = vhost_scsi_get_cmd(vq, tpg, cdb, tag, lun, task_attr,
>exp_data_len + prot_bytes,
>data_direction);
> if (IS_ERR(cmd)) {
> - vq_err(vq, "vhost_scsi_get_tag failed %ld\n",
> + vq_err(vq, "vhost_scsi_get_cmd failed %ld\n",
> PT
Re: [PATCH 3/8] vhost scsi: alloc cmds per vq instead of session
Hi Mike,
url:
https://github.com/0day-ci/linux/commits/Mike-Christie/vhost-scsi-fixes-and-cleanups/20200922-031251
base: https://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git linux-next
config: i386-randconfig-m021-20200923 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot
Reported-by: Dan Carpenter
smatch warnings:
drivers/vhost/scsi.c:606 vhost_scsi_get_cmd() error: uninitialized symbol 'cpu'.
#
https://github.com/0day-ci/linux/commit/aef0e1e9298ab68f2d7bdf1afb9a376641b993d5
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review
Mike-Christie/vhost-scsi-fixes-and-cleanups/20200922-031251
git checkout aef0e1e9298ab68f2d7bdf1afb9a376641b993d5
vim +/cpu +606 drivers/vhost/scsi.c
1a1ff8256af679c8 drivers/vhost/scsi.c Nicholas Bellinger 2015-01-31 572
static struct vhost_scsi_cmd *
aef0e1e9298ab68f drivers/vhost/scsi.c Mike Christie 2020-09-21 573
vhost_scsi_get_cmd(struct vhost_virtqueue *vq, struct vhost_scsi_tpg *tpg,
95e7c4341b8e28da drivers/vhost/scsi.c Nicholas Bellinger 2014-02-22 574
unsigned char *cdb, u64 scsi_tag, u16 lun, u8 task_attr,
95e7c4341b8e28da drivers/vhost/scsi.c Nicholas Bellinger 2014-02-22 575
u32 exp_data_len, int data_direction)
057cbf49a1f08297 drivers/vhost/tcm_vhost.c Nicholas Bellinger 2012-07-18 576 {
aef0e1e9298ab68f drivers/vhost/scsi.c Mike Christie 2020-09-21 577
struct vhost_scsi_virtqueue *svq = container_of(vq,
aef0e1e9298ab68f drivers/vhost/scsi.c Mike Christie 2020-09-21 578
struct vhost_scsi_virtqueue, vq);
1a1ff8256af679c8 drivers/vhost/scsi.c Nicholas Bellinger 2015-01-31 579
struct vhost_scsi_cmd *cmd;
1a1ff8256af679c8 drivers/vhost/scsi.c Nicholas Bellinger 2015-01-31 580
struct vhost_scsi_nexus *tv_nexus;
b1935f687bb93b20 drivers/vhost/scsi.c Nicholas Bellinger 2014-02-22 581
struct scatterlist *sg, *prot_sg;
3aee26b4ae91048c drivers/vhost/scsi.c Nicholas Bellinger 2013-06-21 582
struct page **pages;
10e9cbb6b531117b drivers/vhost/scsi.c Matthew Wilcox 2018-06-12 583
int tag, cpu;
057cbf49a1f08297 drivers/vhost/tcm_vhost.c Nicholas Bellinger 2012-07-18 584
9871831283e79575 drivers/vhost/scsi.c Asias He 2013-05-06 585
tv_nexus = tpg->tpg_nexus;
057cbf49a1f08297 drivers/vhost/tcm_vhost.c Nicholas Bellinger 2012-07-18 586
if (!tv_nexus) {
1a1ff8256af679c8 drivers/vhost/scsi.c Nicholas Bellinger 2015-01-31 587
pr_err("Unable to locate active struct vhost_scsi_nexus\n");
057cbf49a1f08297 drivers/vhost/tcm_vhost.c Nicholas Bellinger 2012-07-18 588
return ERR_PTR(-EIO);
057cbf49a1f08297 drivers/vhost/tcm_vhost.c Nicholas Bellinger 2012-07-18 589
}
057cbf49a1f08297 drivers/vhost/tcm_vhost.c Nicholas Bellinger 2012-07-18 590
aef0e1e9298ab68f drivers/vhost/scsi.c Mike Christie 2020-09-21 591
tag = sbitmap_get(&svq->scsi_tags, 0, false);
4a47d3a1ff10e564 drivers/vhost/scsi.c Nicholas Bellinger 2013-09-23 592
if (tag < 0) {
1a1ff8256af679c8 drivers/vhost/scsi.c Nicholas Bellinger 2015-01-31 593
pr_err("Unable to obtain tag for vhost_scsi_cmd\n");
4a47d3a1ff10e564 drivers/vhost/scsi.c Nicholas Bellinger 2013-09-23 594
return ERR_PTR(-ENOMEM);
4a47d3a1ff10e564 drivers/vhost/scsi.c Nicholas Bellinger 2013-09-23 595
}
4a47d3a1ff10e564 drivers/vhost/scsi.c Nicholas Bellinger 2013-09-23 596
aef0e1e9298ab68f drivers/vhost/scsi.c Mike Christie 2020-09-21 597
cmd = &svq->scsi_cmds[tag];
3aee26b4ae91048c drivers/vhost/scsi.c Nicholas Bellinger 2013-06-21 598
sg = cmd->tvc_sgl;
b1935f687bb93b20 drivers/vhost/scsi.c Nicholas Bellinger 2014-02-22 599
prot_sg = cmd->tvc_prot_sgl;
3aee26b4ae91048c drivers/vhost/scsi.c Nicholas Bellinger 2013-06-21 600
pages = cmd->tvc_upages;
473f0b15a4c97d39 drivers/vhost/scsi.c Markus Elfring 2017-05-20 601
memset(cmd, 0, sizeof(*cmd));
3aee26b4ae91048c drivers/vhost/scsi.c Nicholas Bellinger 2013-06-21 602
cmd->tvc_sgl = sg;
b1935f687bb93b20 drivers/vhost/scsi.c Nicholas Bellinger 2014-02-22 603
cmd->tvc_prot_sgl = prot_sg;
3aee26b4ae91048c drivers/vhost/scsi.c Nicholas Bellinger 2013-06-21 604
cmd->tvc_upages = pages;
4824d3bfb9097ac5 drivers/vhost/scsi.c Nicholas Bellinger 2013-06-07 605
cmd->tvc_se_cmd.map_tag = tag;
10e9cbb6b531117b drivers/vhost/scsi.c Matthew Wilcox 2018-06-12 @606
cmd->tvc_se_cmd.map_cpu = cpu;
"cpu" is never initialized.
95e7c4341b8e28da drivers/vhost/scsi.c Nicholas Bellinger 2014-02-22 607
cmd->tvc_tag = scsi_tag;
95e7c4341b8e28da drivers/vhost/scsi.c Nicholas Bellinger 2014-02-22 608
cmd->tvc_lun = lun;
95e7c4341b8e28da drivers/vhost/scsi.c
Re: [PATCH 3/8] vhost scsi: alloc cmds per vq instead of session
Hi Mike, Thank you for the patch! Perhaps something to improve: [auto build test WARNING on vhost/linux-next] [also build test WARNING on v5.9-rc6 next-20200921] [cannot apply to target/for-next] [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/Mike-Christie/vhost-scsi-fixes-and-cleanups/20200922-031251 base: https://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git linux-next config: s390-randconfig-r022-20200920 (attached as .config) compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project 4e8c028158b56d9c2142a62464e8e0686bde3584) 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 # install s390 cross compiling tool for clang build # apt-get install binutils-s390x-linux-gnu # save the attached .config to linux build tree COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=s390 If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot All warnings (new ones prefixed by >>): include/asm-generic/io.h:490:45: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] val = __le32_to_cpu(__raw_readl(PCI_IOBASE + addr)); ~~ ^ include/uapi/linux/byteorder/big_endian.h:34:59: note: expanded from macro '__le32_to_cpu' #define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x)) ^ include/uapi/linux/swab.h:119:21: note: expanded from macro '__swab32' ___constant_swab32(x) : \ ^ include/uapi/linux/swab.h:20:12: note: expanded from macro '___constant_swab32' (((__u32)(x) & (__u32)0xff00UL) << 8) |\ ^ In file included from drivers/vhost/scsi.c:45: In file included from include/uapi/linux/vhost.h:14: In file included from include/uapi/linux/vhost_types.h:16: In file included from include/linux/virtio_config.h:7: In file included from include/linux/virtio.h:7: In file included from include/linux/scatterlist.h:9: In file included from arch/s390/include/asm/io.h:72: include/asm-generic/io.h:490:45: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] val = __le32_to_cpu(__raw_readl(PCI_IOBASE + addr)); ~~ ^ include/uapi/linux/byteorder/big_endian.h:34:59: note: expanded from macro '__le32_to_cpu' #define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x)) ^ include/uapi/linux/swab.h:119:21: note: expanded from macro '__swab32' ___constant_swab32(x) : \ ^ include/uapi/linux/swab.h:21:12: note: expanded from macro '___constant_swab32' (((__u32)(x) & (__u32)0x00ffUL) >> 8) |\ ^ In file included from drivers/vhost/scsi.c:45: In file included from include/uapi/linux/vhost.h:14: In file included from include/uapi/linux/vhost_types.h:16: In file included from include/linux/virtio_config.h:7: In file included from include/linux/virtio.h:7: In file included from include/linux/scatterlist.h:9: In file included from arch/s390/include/asm/io.h:72: include/asm-generic/io.h:490:45: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] val = __le32_to_cpu(__raw_readl(PCI_IOBASE + addr)); ~~ ^ include/uapi/linux/byteorder/big_endian.h:34:59: note: expanded from macro '__le32_to_cpu' #define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x)) ^ include/uapi/linux/swab.h:119:21: note: expanded from macro '__swab32' ___constant_swab32(x) : \ ^ include/uapi/linux/swab.h:22:12: note: expanded from macro '___constant_swab32' (((__u32)(x) & (__u32)0xff00UL) >> 24))) ^ In file included from drivers/vhost/scsi.c:45: In file included from include/uapi/linux/vhost.h:14: In file included from include/uapi/linux/vhost_types.h:16: In file included from include/linux/virtio_config.h:7: In file included from include/linux/virtio.h:7: In file included from include/linux/scatterlist.h:9: In file included from arch/s390/include/asm/io.h:72: include/asm-generic/io.h:490:45: warning: performing pointer arithme
