Re: [PATCH 02/13] target: split target_submit_cmd_map_sgls

Re: [PATCH 02/13] target: split target_submit_cmd_map_sgls

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 4737 bytes --]

Hi Mike,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on mkp-scsi/for-next]
[also build test WARNING on vhost/linux-next v5.11-rc7 next-20210125]
[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/target-move-t_task_cdb-initialization/20210209-213926
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git for-next
config: h8300-randconfig-s031-20210209 (attached as .config)
compiler: h8300-linux-gcc (GCC) 9.3.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # apt-get install sparse
        # sparse version: v0.6.3-215-g0fb77bb6-dirty
        # https://github.com/0day-ci/linux/commit/3382952197b63f11c166ff293f819ce6ac4d52ae
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Mike-Christie/target-move-t_task_cdb-initialization/20210209-213926
        git checkout 3382952197b63f11c166ff293f819ce6ac4d52ae
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=h8300 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>


"sparse warnings: (new ones prefixed by >>)"
>> drivers/target/target_core_transport.c:1695:12: sparse: sparse: incorrect type in assignment (different base types) @@     expected int rc @@     got restricted sense_reason_t @@
   drivers/target/target_core_transport.c:1695:12: sparse:     expected int rc
   drivers/target/target_core_transport.c:1695:12: sparse:     got restricted sense_reason_t
   drivers/target/target_core_transport.c:1699:12: sparse: sparse: incorrect type in assignment (different base types) @@     expected int rc @@     got restricted sense_reason_t @@
   drivers/target/target_core_transport.c:1699:12: sparse:     expected int rc
   drivers/target/target_core_transport.c:1699:12: sparse:     got restricted sense_reason_t
>> drivers/target/target_core_transport.c:1736:51: sparse: sparse: incorrect type in argument 2 (different base types) @@     expected restricted sense_reason_t [usertype] @@     got int rc @@
   drivers/target/target_core_transport.c:1736:51: sparse:     expected restricted sense_reason_t [usertype]
   drivers/target/target_core_transport.c:1736:51: sparse:     got int rc

vim +1695 drivers/target/target_core_transport.c

  1678	
  1679	/**
  1680	 * target_submit - perform final initialization and submit cmd to LIO core
  1681	 * @se_cmd: command descriptor to submit
  1682	 * @cdb: pointer to SCSI CDB
  1683	 *
  1684	 * target_submit_prep must have been called on the cmd, and this must be
  1685	 * called from process context.
  1686	 */
  1687	static void target_submit(struct se_cmd *se_cmd, unsigned char *cdb)
  1688	{
  1689		struct scatterlist *sgl = se_cmd->t_data_sg;
  1690		unsigned char *buf = NULL;
  1691		int rc;
  1692	
  1693		might_sleep();
  1694	
> 1695		rc = target_cmd_init_cdb(se_cmd, cdb);
  1696		if (rc)
  1697			goto fail;
  1698	
  1699		rc = target_cmd_parse_cdb(se_cmd);
  1700		if (rc != 0)
  1701			goto fail;
  1702	
  1703		if (se_cmd->t_data_nents != 0) {
  1704			BUG_ON(!sgl);
  1705			/*
  1706			 * A work-around for tcm_loop as some userspace code via
  1707			 * scsi-generic do not memset their associated read buffers,
  1708			 * so go ahead and do that here for type non-data CDBs.  Also
  1709			 * note that this is currently guaranteed to be a single SGL
  1710			 * for this case by target core in target_setup_cmd_from_cdb()
  1711			 * -> transport_generic_cmd_sequencer().
  1712			 */
  1713			if (!(se_cmd->se_cmd_flags & SCF_SCSI_DATA_CDB) &&
  1714			     se_cmd->data_direction == DMA_FROM_DEVICE) {
  1715				if (sgl)
  1716					buf = kmap(sg_page(sgl)) + sgl->offset;
  1717	
  1718				if (buf) {
  1719					memset(buf, 0, sgl->length);
  1720					kunmap(sg_page(sgl));
  1721				}
  1722			}
  1723	
  1724		}
  1725	
  1726		/*
  1727		 * Check if we need to delay processing because of ALUA
  1728		 * Active/NonOptimized primary access state..
  1729		 */
  1730		core_alua_check_nonop_delay(se_cmd);
  1731	
  1732		transport_handle_cdb_direct(se_cmd);
  1733		return;
  1734	
  1735	fail:
> 1736		transport_generic_request_failure(se_cmd, rc);
  1737	}
  1738	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 23129 bytes --]

[-- Attachment #3: Type: text/plain, Size: 183 bytes --]

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 01/13] target: move t_task_cdb initialization

[Christoph Hellwig]

On Tue, Feb 09, 2021 at 06:38:33AM -0600, Mike Christie wrote:
> The next patch splits target_submit_cmd_map_sgls so the initialization
> and submission part can be called at different times. If the init part
> fails we can reference the t_task_cdb early in some of the logging
> and tracing code. This moves it to transport_init_se_cmd so we don't
> hit NULL pointer crashes.
> 
> Signed-off-by: Mike Christie <michael.christie@oracle.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 02/13] target: split target_submit_cmd_map_sgls

[Christoph Hellwig]

Can you just kill off target_submit_cmd_map_sgls entirely and just
open code the two calls in the three callers?
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 05/13] tcm loop: use blk cmd allocator for se_cmds

[Christoph Hellwig]

On Tue, Feb 09, 2021 at 06:38:37AM -0600, Mike Christie wrote:
> This just has tcm loop use the block layer cmd allocator for se_cmds
> instead of using the tcm_loop_cmd_cache. In future patches when we
> can use the host tags for internal requests like TMFs we can completely
> kill the tcm_loop_cmd_cache.
> 
> Signed-off-by: Mike Christie <michael.christie@oracle.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 07/13] target: cleanup cmd flag bits

[Christoph Hellwig]

On Tue, Feb 09, 2021 at 06:38:39AM -0600, Mike Christie wrote:
> We have a couple holes in the cmd flags definitions. This cleans
> up the definitions to fix that and make it easier to read.
> 
> Signed-off-by: Mike Christie <michael.christie@oracle.com>
> Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 11/13] target: replace work per cmd in completion path

[Christoph Hellwig]

On Tue, Feb 09, 2021 at 06:38:43AM -0600, Mike Christie wrote:
> Doing a work per cmd can lead to lots of threads being created.
> This patch just replaces the completion work per cmd with a per cpu
> list. Combined with the first patches this allows tcm loop on top of
> initiators like iser to go from around 700K IOPs to 1000K and reduces
> the number of threads that get created when the system is under heavy
> load and hitting the initiator drivers tagging limits.

OTOH it does increase completion latency, which might be the preference
for some workloads.  Do we need a tunable here?

> +static void target_queue_cmd_work(struct se_cmd_queue *q, struct se_cmd *se_cmd,
> +				  int cpu, struct workqueue_struct *wq)
>  {
> -	struct se_cmd *cmd = container_of(work, struct se_cmd, work);
> +	llist_add(&se_cmd->se_cmd_list, &q->cmd_list);
> +	queue_work_on(cpu, wq, &q->work);
> +}

Do we need this helper at all?  Having it open coded in the two callers
would seem easier to follow to me.

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 12/13] target, vhost-scsi: don't switch cpus on completion

[Christoph Hellwig]

>  	struct se_device *se_dev = se_cmd->se_dev;
> -	int cpu = se_cmd->cpuid;
> +	int cpu;
> +
> +	if (se_cmd->se_cmd_flags & SCF_IGNORE_CPUID_COMPL)
> +		cpu = smp_processor_id();
> +	else
> +		cpu = se_cmd->cpuid;
>  
>  	target_queue_cmd_work(&se_dev->queues[cpu].cq, se_cmd, cpu,
>  			      target_completion_wq);

Can't we just use se_cmd->cpuid == WORK_CPU_UNBOUND as the no affinity
indicator, which would remove all branches here.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization