Re: [PATCH 2/6] tcm: Add support forstruct target_core_fabric_ops->task_sg_chaining

From: Nicholas A. Bellinger
Date: Tue Sep 07 2010 - 16:46:40 EST

Next message: Randy Dunlap: "Re: [PATCH] west bridge, device modules fixes"
Previous message: Ira W. Snyder: "[PATCH] fsldma: move DMA_SLAVE support functions to the driver"
In reply to: Boaz Harrosh: "Re: [PATCH 2/6] tcm: Add support for struct target_core_fabric_ops->task_sg_chaining"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Tue, 2010-09-07 at 12:52 +0300, Boaz Harrosh wrote:
> 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@xxxxxxxxxxxxxxx>
> >>
> >> 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?)
>

Np on this. Btw, the drivers/target/lio-target code was not included in
the last RFC posting, but I will be getting to this soon. ;-)

> 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.

Yep, this point is understand wrt to sg_chaining=1 no longer being an
option.

>
> 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 in complete agreement here wrt to new modules having to support
proper chaining.

> 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.
>

Hmmm. It appears that lib/scatterlist.c:__sg_alloc_table() would in
fact me a nice fit for the transport_calc_sg_num() allocation (see
below). I will have a look at how much would be involved to convert the
current code to use these helpers.

> 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().

So, note the actual allocation of the contigious struct
se_task->task_sg[] that are being chained together in
transport_do_task_sg_chain() is done in transport_calc_sg_num() here:

http://git.kernel.org/?p=linux/kernel/git/nab/lio-core-2.6.git;a=blob;f=drivers/target/target_core_transport.c;hb=refs/heads/lio-4.0#l6810

which includes the proper use of sg_init_table(), and a call to
sg_mark_end() with SGL padding for the TFO->task_sg_chainging=1 case
with this patch.

Thanks for your comments Boaz!

--nab

>
> Good lock
> Boaz
>
> > --nab
> >
> >> Signed-off-by: Nicholas A. Bellinger <nab@xxxxxxxxxxxxxxx>
> >> ---
> >> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Next message: Randy Dunlap: "Re: [PATCH] west bridge, device modules fixes"
Previous message: Ira W. Snyder: "[PATCH] fsldma: move DMA_SLAVE support functions to the driver"
In reply to: Boaz Harrosh: "Re: [PATCH 2/6] tcm: Add support for struct target_core_fabric_ops->task_sg_chaining"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]