Message-ID: <4C860B42.6080600@panasas.com>
Date:	Tue, 07 Sep 2010 12:52:02 +0300
From:	Boaz Harrosh <bharrosh@...asas.com>
To:	"Nicholas A. Bellinger" <nab@...ux-iscsi.org>
CC:	linux-scsi <linux-scsi@...r.kernel.org>,
	Jens Axboe <axboe@...nel.dk>,
	linux-kernel <linux-kernel@...r.kernel.org>,
	Christoph Hellwig <hch@....de>,
	FUJITA Tomonori <fujita.tomonori@....ntt.co.jp>,
	Mike Christie <michaelc@...wisc.edu>,
	Hannes Reinecke <hare@...e.de>,
	James Bottomley <James.Bottomley@...e.de>,
	Konrad Rzeszutek Wilk <konrad@...nok.org>,
	Richard Sharpe <realrichardsharpe@...il.com>
Subject: Re: [PATCH 2/6] tcm: Add support for struct	target_core_fabric_ops->task_sg_chaining

On 09/07/2010 12:46 AM, Nicholas A. Bellinger wrote:
> On Mon, 2010-09-06 at 14:37 -0700, Nicholas A. Bellinger wrote:
>> From: Nicholas Bellinger <nab@...ux-iscsi.org>
>>
>> This patch adds support for struct target_core_fabric_ops->task_sg_chaining=1,
>> which allows a single struct se_cmd descriptor to use include/linux/scatterlist.h
>> SGL chaining logic and link contigious struct se_task->task_sg[] arrays together
>> into a single walkable scatterlist chain for HW target mode drivers that require
>> this for pci_map_sg().
>>
>> This also includes a transport_do_task_sg_chain() helper which does the sg_chain()
>> calls, and other necessary struct scatterlist->page_link termination bit twiddling to
>> create a linked SGL from struct se_task->task_sg[] created in transport_calc_sg_num()
>>
>> Inside of transport_calc_sg_num(), this patch when running in TFI->task_sg_chaining=1
>> mode will alloc an extra trailing padding struct scatterlist entry to the primary
>> struct se_task->task_sg[] allocation.  The call to transport_do_task_sg_chain()
>> will then use these extra trailing SGL for the sg_chain() call.
>>
> 
> Hi Jens and Co,
> 
> When you have a moment, please have a look at this patch, namely the
> transport_do_task_sg_chain() logic which uses
> include/linux/scatterlist.h:sg_chain() to create a struct se_cmd wide
> SGL chain of the individual contigiously allocated struct
> se_task->task_sg[].  I am currently testing this logic with TCM HW
> fabric modules that expect to use pci_map_sg() and pci_unmap_sg() for
> mapping SCSI data payload into PCIe memory, but there are other folks
> who's modules will be depending on this logic as well.
> 
> Thanks!
> 

Hi Nicholas

I have not yet had the time to properly review any of your patches, Sorry.
I seem to be behind schedule on my duties and can't catch up ;-(
(BTW iscsi_tcp code was not yet posted in the last round, right?)

I do read the mails though and this series prompts a warning in my mind
so my $0.017 here.

In SCSI the sg_chaining=1 is no longer an option. It is/must supported by
all LLDs. There was a transition time where we had such flag so all drivers
did not need to be converted at once, and the effort was done by multiple
people. But once all LLDs where converted it was removed. Note that only few
initiators actually supply chained sg's but all drivers and supporting code
will behave properly if met. Also initiator need to comply with the last
sg marked with the end bit.

So I think all submitted modules must comply with sg-chaining from day
one. This does not mean that they need to actually allocate and send chains
only that all loops are done with the proper sg-chain macros and that
the sg arrays are sg_init_table() and/or sg_mark_end(). (Don't consider speed
the chained loops are just as fast as none-chained loops if done properly)

Also below in transport_do_task_sg_chain() from a short glans it looks like
you have a pre allocated list of sg-arrays. That you want to chain sg-chain
style. It might be easier/cleaner to do with the __sg_alloc_table helper.
Look at scsi_alloc_sgtable(). The __sg_alloc_table API receives a pointer
to an sg_alloc_fn function. That one in your case can just return the next one
in the list. However __sg_alloc_table has a deficiency in that it does not let
you pass a user-pointer to your sg_alloc_fn. So you might need to do something
about that.

Finally if you decide to keep transport_do_task_sg_chain() inline for other
reasons. Please see __sg_alloc_table() for proper chaining code. Specifically
the proper use of sg_init_table(), sg_chain() and sg_mark_end().

Good lock
Boaz

> --nab
> 
>> Signed-off-by: Nicholas A. Bellinger <nab@...ux-iscsi.org>
>> ---
>>  drivers/target/target_core_transport.c  |  127 +++++++++++++++++++++++++++++-
>>  include/target/target_core_base.h       |    4 +
>>  include/target/target_core_fabric_ops.h |    6 ++
>>  include/target/target_core_transport.h  |    1 +
>>  4 files changed, 133 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
>> index 5be102c..602b2d8 100644
>> --- a/drivers/target/target_core_transport.c
>> +++ b/drivers/target/target_core_transport.c
>> @@ -6736,7 +6736,8 @@ extern u32 transport_calc_sg_num(
>>  {
>>  	struct se_cmd *se_cmd = task->task_se_cmd;
>>  	struct se_mem *se_mem = in_se_mem;
>> -	u32 sg_length, task_size = task->task_size;
>> +	struct target_core_fabric_ops *tfo = CMD_TFO(se_cmd);
>> +	u32 sg_length, task_size = task->task_size, task_sg_num_padded;
>>  
>>  	while (task_size != 0) {
>>  		DEBUG_SC("se_mem->se_page(%p) se_mem->se_len(%u)"
>> @@ -6786,8 +6787,19 @@ next:
>>  
>>  		task->task_sg_num++;
>>  	}
>> +	/*
>> +	 * Check if the fabric module driver is requesting that all
>> +	 * struct se_task->task_sg[] be chained together..  If so,
>> +	 * then allocate an extra padding SG entry for linking and
>> +	 * marking the end of the chained SGL.
>> +	 */
>> +	if (tfo->task_sg_chaining) {
>> +		task_sg_num_padded = (task->task_sg_num + 1);
>> +		task->task_padded_sg = 1;
>> +	} else
>> +		task_sg_num_padded = task->task_sg_num;
>>  
>> -	task->task_sg = kzalloc(task->task_sg_num *
>> +	task->task_sg = kzalloc(task_sg_num_padded *
>>  			sizeof(struct scatterlist), GFP_KERNEL);
>>  	if (!(task->task_sg)) {
>>  		printk(KERN_ERR "Unable to allocate memory for"
>> @@ -6795,10 +6807,18 @@ next:
>>  		return 0;
>>  	}
>>  
>> -	sg_init_table(&task->task_sg[0], task->task_sg_num);
>> +	sg_init_table(&task->task_sg[0], task_sg_num_padded);
>> +	/*
>> +	 * For the chaining case, setup the proper end of SGL for the
>> +	 * initial submission struct task into struct se_subsystem_api.
>> +	 * This will be cleared later by transport_do_task_sg_chain()
>> +	 */
>> +	if (task->task_padded_sg)
>> +		sg_mark_end(&task->task_sg[task->task_sg_num - 1]);
>>  
>> -	DEBUG_SC("Successfully allocated task->task_sg_num(%u)\n",
>> -			task->task_sg_num);
>> +	DEBUG_SC("Successfully allocated task->task_sg_num(%u),"
>> +		" task_sg_num_padded(%u)\n", task->task_sg_num,
>> +		task_sg_num_padded);
>>  
>>  	return task->task_sg_num;
>>  }
>> @@ -7021,6 +7041,103 @@ next:
>>  	return 0;
>>  }
>>  
>> +/*
>> + * This function can be used by HW target mode drivers to create a linked
>> + * scatterlist from all contiguously allocated struct se_task->task_sg[].
>> + * This is intended to be called during the completion path by TCM Core
>> + * when struct target_core_fabric_ops->check_task_sg_chaining is enabled.
>> + */
>> +void transport_do_task_sg_chain(struct se_cmd *cmd)
>> +{
>> +	struct scatterlist *sg_head = NULL, *sg_link = NULL, *sg_first = NULL;
>> +	struct scatterlist *sg_head_cur = NULL, *sg_link_cur = NULL;
>> +	struct scatterlist *sg, *sg_end = NULL, *sg_end_cur = NULL;
>> +	struct se_task *task;
>> +	struct target_core_fabric_ops *tfo = CMD_TFO(cmd);
>> +	u32 task_sg_num = 0, sg_count = 0;
>> +	int i;
>> +
>> +	if (tfo->task_sg_chaining == 0) {
>> +		printk(KERN_ERR "task_sg_chaining is diabled for fabric module:"
>> +				" %s\n", tfo->get_fabric_name());
>> +		dump_stack();
>> +		return;
>> +	}
>> +	/*
>> +	 * Walk the struct se_task list and setup scatterlist chains
>> +	 * for each contiguosly allocated struct se_task->task_sg[].
>> +	 */
>> +	list_for_each_entry(task, &T_TASK(cmd)->t_task_list, t_list) {
>> +		if (!(task->task_sg) || !(task->task_padded_sg))
>> +			continue;
>> +
>> +		if (sg_head && sg_link) {
>> +			sg_head_cur = &task->task_sg[0];
>> +			sg_link_cur = &task->task_sg[task->task_sg_num];
>> +			/*
>> +			 * Either add chain or mark end of scatterlist
>> +			 */
>> +			if (!(list_is_last(&task->t_list,
>> +					&T_TASK(cmd)->t_task_list))) {
>> +				/*
>> +				 * Clear existing SGL termination bit set in
>> +				 * transport_calc_sg_num(), see sg_mark_end()
>> +				 */
>> +				sg_end_cur = &task->task_sg[task->task_sg_num - 1];
>> +				sg_end_cur->page_link &= ~0x02;
>> +
>> +				sg_chain(sg_head, task_sg_num, sg_head_cur);
>> +				sg_count += (task->task_sg_num + 1);
>> +			} else
>> +				sg_count += task->task_sg_num;
>> +
>> +			sg_head = sg_head_cur;
>> +			sg_link = sg_link_cur;
>> +			task_sg_num = task->task_sg_num;
>> +			continue;
>> +		}
>> +		sg_head = sg_first = &task->task_sg[0];
>> +		sg_link = &task->task_sg[task->task_sg_num];
>> +		task_sg_num = task->task_sg_num;
>> +		/*
>> +		 * Check for single task..
>> +		 */
>> +		if (!(list_is_last(&task->t_list, &T_TASK(cmd)->t_task_list))) {
>> +			/*
>> +			 * Clear existing SGL termination bit set in
>> +			 * transport_calc_sg_num(), see sg_mark_end()
>> +			 */
>> +			sg_end = &task->task_sg[task->task_sg_num - 1];
>> +			sg_end->page_link &= ~0x02;
>> +			sg_count += (task->task_sg_num + 1);
>> +		} else
>> +			sg_count += task->task_sg_num;
>> +	}
>> +	/*
>> +	 * Setup the starting pointer and total t_tasks_sg_linked_no including
>> +	 * padding SGs for linking and to mark the end.
>> +	 */
>> +	T_TASK(cmd)->t_tasks_sg_chained = sg_first;
>> +	T_TASK(cmd)->t_tasks_sg_chained_no = sg_count;
>> +
>> +	printk("Setup T_TASK(cmd)->t_tasks_sg_chained: %p and"
>> +		" t_tasks_sg_chained_no: %u\n", T_TASK(cmd)->t_tasks_sg_chained,
>> +		T_TASK(cmd)->t_tasks_sg_chained_no);
>> +
>> +	for_each_sg(T_TASK(cmd)->t_tasks_sg_chained, sg,
>> +			T_TASK(cmd)->t_tasks_sg_chained_no, i) {
>> +
>> +		printk("SG: %p page: %p length: %d offset: %d\n",
>> +			sg, sg_page(sg), sg->length, sg->offset);
>> +		if (sg_is_chain(sg))
>> +			printk("SG: %p sg_is_chain=1\n", sg);
>> +		if (sg_is_last(sg))
>> +			printk("SG: %p sg_is_last=1\n", sg);
>> +	}
>> +
>> +}
>> +EXPORT_SYMBOL(transport_do_task_sg_chain);
>> +
>>  static int transport_do_se_mem_map(
>>  	struct se_device *dev,
>>  	struct se_task *task,
>> diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
>> index 0833612..dbcc04a 100644
>> --- a/include/target/target_core_base.h
>> +++ b/include/target/target_core_base.h
>> @@ -439,6 +439,7 @@ struct se_transport_task {
>>  	u32			t_tasks_no;
>>  	u32			t_tasks_sectors;
>>  	u32			t_tasks_se_num;
>> +	u32			t_tasks_sg_chained_no;
>>  	atomic_t		t_fe_count;
>>  	atomic_t		t_se_count;
>>  	atomic_t		t_task_cdbs_left;
>> @@ -462,6 +463,8 @@ struct se_transport_task {
>>  	struct completion	t_transport_passthrough_wcomp;
>>  	struct completion	transport_lun_fe_stop_comp;
>>  	struct completion	transport_lun_stop_comp;
>> +	struct scatterlist	*t_tasks_sg_chained;
>> +	struct scatterlist	t_tasks_sg_bounce;
>>  	void			*t_task_buf;
>>  	void			*t_task_pt_buf;
>>  	struct list_head	t_task_list;
>> @@ -476,6 +479,7 @@ struct se_task {
>>  	u8		task_flags;
>>  	int		task_error_status;
>>  	int		task_state_flags;
>> +	int		task_padded_sg:1;
>>  	unsigned long long	task_lba;
>>  	u32		task_no;
>>  	u32		task_sectors;
>> diff --git a/include/target/target_core_fabric_ops.h b/include/target/target_core_fabric_ops.h
>> index e620eda..5dd61a9 100644
>> --- a/include/target/target_core_fabric_ops.h
>> +++ b/include/target/target_core_fabric_ops.h
>> @@ -3,6 +3,12 @@ struct target_fabric_configfs;
>>  
>>  struct target_core_fabric_ops {
>>  	struct configfs_subsystem *tf_subsys;
>> +	/*
>> +	 * Optional to signal struct se_task->task_sg[] padding entries
>> +	 * for scatterlist chaining using transport_do_task_sg_link(),
>> +	 * disabled by default
>> +	 */
>> +	int task_sg_chaining:1;
>>  	char *(*get_fabric_name)(void);
>>  	u8 (*get_fabric_proto_ident)(struct se_portal_group *);
>>  	char *(*tpg_get_wwn)(struct se_portal_group *);
>> diff --git a/include/target/target_core_transport.h b/include/target/target_core_transport.h
>> index 711ec71..2eee898 100644
>> --- a/include/target/target_core_transport.h
>> +++ b/include/target/target_core_transport.h
>> @@ -263,6 +263,7 @@ extern int transport_map_sg_to_mem(struct se_cmd *, struct list_head *,
>>  extern int transport_map_mem_to_sg(struct se_task *, struct list_head *,
>>  					void *, struct se_mem *,
>>  					struct se_mem **, u32 *, u32 *);
>> +extern void transport_do_task_sg_chain(struct se_cmd *);
>>  extern u32 transport_generic_get_cdb_count(struct se_cmd *,
>>  					struct se_transform_info *,
>>  					unsigned long long, u32, struct se_mem *,
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists