On 11/3/10 4:52 PM, Patil, Kiran wrote:
>
>
> -----Original Message-----
> From: Joe Eykholt [mailto:[email protected]]
> Sent: Wednesday, November 03, 2010 4:27 PM
> To: Patil, Kiran
> Cc: [email protected]
> Subject: Re: [Open-FCoE] [RFC PATCH] TCM/LIO: Added Large Send Offload (LSO)
> support in target if underlying HW supports it.
>
>
>
> On 11/3/10 4:15 PM, Kiran Patil wrote:
>> From: Kiran Patil <[email protected]>
>>
>> Problem: Existing implementation of TCM/LIO target isn't taking advantage of
>> underlying HW (NIC)
>> Large Send Offload (LSO) feature.
>>
>> Fix: Enhance function (ft_queue_data_in) which sends data back to
>> initiator to make use of
>> LSO feature of underlying HW.
>>
>> Technical Details: Enhancement benefits in all threee cases , use_sg, mem,
>> and task_buf. This changes
>> are applicable only for SG_DATA_IO_CDB because most likely for
>> other CDB's, response data may
>> may not cross max_frame_size (2112).
>>
>> Dependencies: This depends on TCM and Joe's FC4 patches for TCM
>>
>> Signed-off-by: Kiran Patil <[email protected]>
>> ---
>>
>> drivers/target/tcm_fc/tfc_io.c | 39
>> ++++++++++++++++++++++++++++++++++++---
>> 1 files changed, 36 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/target/tcm_fc/tfc_io.c b/drivers/target/tcm_fc/tfc_io.c
>> index e77a45a..c64313d 100644
>> --- a/drivers/target/tcm_fc/tfc_io.c
>> +++ b/drivers/target/tcm_fc/tfc_io.c
>> @@ -111,7 +111,12 @@ int ft_queue_data_in(struct se_cmd *se_cmd)
>>
>> /* no scatter/gather in skb for odd word length due to fc_seq_send() */
>> use_sg = !(remaining % 4);
>> - use_sg = 0; /* XXX to test the non-sg path */
>> + /*
>> + * Change it to not used 'scatterlist' only if se_cmd_flags doesn't
>
> s/used/use/
>
> Agree, will change it.
>
>> + * indicate it as DATA_SG_IO_CDB
>> + */
>> + if (!(se_cmd->se_cmd_flags & SCF_SCSI_DATA_SG_IO_CDB))
>
> Does that flag indicate the back-end's capability for SG's or libfc's
> capability
> for SG's? It seems like any buffer that TCM gives us can be represented by
> an SG list to hand to the libfc LLD.
>
> This is the description of those SCSI_SCF* given by Nick:
>
> *) SCF_SCSI_DATA_SG_IO_CDB
>
> Used for all WRITE_* and READ_* bulk I/O. This may involve more
> than a single struct se_task to map received transfer length in sectors
> to the backend DEV_ATTRIB(dev)->max_sectors limitations.
>
> A list of struct se_mem is setup T_TASK(cmd)->t_mem_list using either
> physical scatterlists passed into TCM Core (see TCM_Loop) or using
> internally allocated memory that setup in struct se_mem->se_page, and
> then mapped into struct se_task->task_sg[].
>
> *) SCF_SCSI_CONTROL_SG_IO_CDB
>
> Used for control CDBs that underlying modern Linux/SCSI LLDs want to be
> in SG format
>
> *) SCF_SCSI_CONTROL_NONSG_IO_CDB
>
> Used for control CDBs that we do internal TCM emulation using a
> contigious buffer. This makes the emulation code alot easier to
> implement, and modern SCSI LLDs are expected to be able to accept both
> types for CONTROL cdbs.
>
> *) SCF_SCSI_NON_DATA_CDB
>
> Used for CDBs that contain a zero transfer length (TEST_UNIT_READY) and
> that no mappable SCSI payload exists.
>
> Meanwhile I will try to find out answer to your question w.r.t "back-end
> capability for SG or libfc". I see the point which, any buffer which TCM
> gives us can be represented as SG (Nick, please correct me if I missed
> anything).
I'll let you and Nick figure it out, but it seems like this check isn't needed.
>> + use_sg = 0; /* XXX to test the non-sg path */
>>
>> while (remaining) {
>> if (!mem_len) {
>> @@ -123,7 +128,13 @@ int ft_queue_data_in(struct se_cmd *se_cmd)
>> page = mem->se_page;
>> }
>> if (!frame_len) {
>> - frame_len = cmd->sess->max_frame;
>> + /*
>> + * If lport's has capability of Large Send Offload LSO)
>> + * , then allow 'frame_len' to be as big as 'lso_max'
>> + * if indicated transfer length is >= lport->lso_max
>> + */
>> + frame_len = (lport->seq_offload) ? lport->lso_max :
>> + cmd->sess->max_frame;
>
> Why not set this up in the session instead of testing seq_offload for each
> frame?
> Basically add lso_max to the session as a separate field. But ... that's OK
> to do in a later enhancement.
>
> Agree, will do this as later enhancement
>
>> frame_len = min(frame_len, remaining);
>> fp = fc_frame_alloc(lport, use_sg ? 0 : frame_len);
>> if (!fp)
>> @@ -131,6 +142,13 @@ int ft_queue_data_in(struct se_cmd *se_cmd)
>> to = fc_frame_payload_get(fp, 0);
>> fh_off = frame_off;
>> frame_off += frame_len;
>> + /*
>> + * Setup the frame's max payload which is used by base
>> + * driver to indicate HW about max frame size, so that
>> + * HW can do fragmentation appropriately based on
>> + * "GSO_SIZE".
>> + */
>> + fr_max_payload(fp) = cmd->sess->max_frame;
>> }
>> tlen = min(mem_len, frame_len);
>>
>> @@ -167,7 +185,17 @@ int ft_queue_data_in(struct se_cmd *se_cmd)
>> frame_len -= tlen;
>> remaining -= tlen;
>>
>> - if (frame_len)
>> + if (use_sg) {
>> + /*
>> + * If using scatterlist, then allow (continue) building
>> + * 'skb' as long as 'nr_frags' is less than
>> + * FC_FRAME_SG_LEN and frame_len is valid, otherwise
>> + * send the frame.
>> + */
>> + if (frame_len &&
>> + (skb_shinfo(fp_skb(fp))->nr_frags < FC_FRAME_SG_LEN))
>> + continue;
>> + } else if (frame_len)
>> continue;
>> if (!remaining)
>> f_ctl |= FC_FC_END_SEQ;
>> @@ -177,6 +205,11 @@ int ft_queue_data_in(struct se_cmd *se_cmd)
>> if (error) {
>> WARN_ON(1);
>> /* XXX For now, initiator will retry */
>> + printk(KERN_ERR "%s: Failed to send frame %p, "
>> + "xid <0x%x>, remaining <0x%x>, "
>> + "lso_max <0x%x>\n",
>> + __func__, fp, ep->xid, remaining,
>> + lport->lso_max);
>
> WARN_ON could be removed. Maybe printk should be rate-limited or deleted.
>
> Question: If we remove WARN_ON and printk, how would we know that, target was
> unable to send frame.
You would see it on the initiator. It would resend. printk is a good idea for
now.
> Agree with rate limited. Any suggestion w.r.t rate and should it be global
> rate or local to the current invocation of ft_queue_data_in? Any suggestion.
> I would think, in a given invocation of ft_queue_data_in, if unable to send
> frame 5 times, then print, otherwise ignore.
I think it's OK to globally limit the printk. You can do:
if (printk_ratelimit())
printk( ...);
which takes care of the whole problem. At least you'll see it the
first ten times. It limits the messages to no more than 10 every 5 seconds.
Joe
>> }
>> }
>> return ft_queue_status(se_cmd);
>>
>> _______________________________________________
>> devel mailing list
>> [email protected]
>> http://www.open-fcoe.org/mailman/listinfo/devel
_______________________________________________
devel mailing list
[email protected]
http://www.open-fcoe.org/mailman/listinfo/devel
