Thanks a bunch Joe for prink_ratelimit suggestion. Shall address this in "RFC v2". Meanwhile, will figure out "check is needed or not (means any given buffer by TCM be represented as SG or not".
-- Kiran P. -----Original Message----- From: Joe Eykholt [mailto:[email protected]] Sent: Wednesday, November 03, 2010 5:07 PM To: Patil, Kiran Cc: [email protected]; Nicholas A. Bellinger; Love, Robert W; Zou, Yi 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: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
