-----Original Message----- From: Boaz Harrosh [mailto:bharrosh@xxxxxxxxxxx] Subject: Re: [PATCH 2/2 2.6.29] cxgb3i - accelerating open-iscsi initiator > +3. edit /etc/iscsi/iscsid.conf > + The default setting for MaxRecvDataSegmentLength (131072) is too big, search > + and replace all occurances of "xxx.iscsi.MaxRecvDataSegmentLength" to be a > + value no bigger than 15360 (for example 8192): > + > + discovery.sendtargets.iscsi.MaxRecvDataSegmentLength = 8192 > + node.conn[0].iscsi.MaxRecvDataSegmentLength = 8192 > + What happens if you don't do this? it will then not use acceleration and fall back to SW tcp? or it will not work at all? Can the driver limit that no matter what the user put? [Karen] If node.conn[0].iscsi.MaxRecvDataSegmentLength is not changed, the login will fail and a message will be printed to dmesg in the format of "cxgb3i: ERR! MaxRecvSegmentLength <X> too big. Need to be <= <Y>." The discovery session is not accelerated, so it will go through. I will update the .txt file. > +config SCSI_CXGB3_ISCSI > + tristate "Chelsio S3xx iSCSI support" > + select CHELSIO_T3 > + select SCSI_ISCSI_ATTRS > + select ISCSI_TCP Looks like you don't need ISCSI_TCP since Mike's last changes. ISCSI_TCP will give you iscsi_tcp.ko, but all you need is libiscsi_tcp.ko which will be enabled by default. (I think) [Karen] Will remove select ISCSI_TCP. > + ---help--- > + This driver supports iSCSI offload for the Chelsio S3 series devices. > diff --git a/drivers/scsi/cxgb3i/Makefile b/drivers/scsi/cxgb3i/Makefile > new file mode 100644 > index 0000000..ee7d6d2 > --- /dev/null > +++ b/drivers/scsi/cxgb3i/Makefile > @@ -0,0 +1,4 @@ > +EXTRA_CFLAGS += -I$(TOPDIR)/drivers/net/cxgb3 > + +ccflags-y += -I$(TOPDIR)/drivers/net/cxgb3 Make Sam's life easier. It's better to put this in a Kbuild file. It is not a real Makefile anyway. [Karen] Will rename Makefile to Kbuild. > +#define ISCSI_PDU_HEADER_MAX (56 + 256) /* bhs + digests + ahs */ > + Actually sizeof(iscsi_sw_tcp_hdrbuf), which is ? I'm not sure a bit less I think like 308. or: +#define ISCSI_PDU_HEADER_MAX \ + sizeof(struct iscsi_hdr) + ISCSI_MAX_AHS_SIZE + ISCSI_DIGEST_SIZE; [Karen] Will use defines instead of actual numbers. > + > + ddp = cxgb3i_alloc_big_mem(sizeof(struct cxgb3i_ddp_info) + > + ppmax * > + (sizeof(struct cxgb3i_gather_list *) + > + sizeof(struct sk_buff *)), > + GFP_KERNEL); >From what I understand so far: DDP is only used for read of PDU, right? so we only need space for the read DDP. the writes are made by regular SKBs right? How is then the acceleration works with out-going data? OK I guess that is a separate issue. DDP acceleration for reads, and separately, digest accelerations for both. [Karen] Yes, ddp is for read only, the write will have digest acceleration. > + if (!ddp) { > + ddp_log_warn("%s unable to alloc ddp 0x%d, ddp disabled.\n", > + tdev->name, ppmax); > + return 0; > + } > + ddp->gl_map = (struct cxgb3i_gather_list **)(ddp + 1); > + ddp->gl_skb = (struct sk_buff **)(((char *)ddp->gl_map) + > + ppmax * > + sizeof(struct cxgb3i_gather_list *)); > + spin_lock_init(&ddp->map_lock); > + > + ddp->tdev = tdev; > + ddp->pdev = uinfo.pdev; > + ddp->max_txsz = min_t(unsigned int, uinfo.max_txsz, ULP2_MAX_PKT_SIZE); > + ddp->max_rxsz = min_t(unsigned int, uinfo.max_rxsz, ULP2_MAX_PKT_SIZE); Please note that from what I understand, only the out-going headers can be big, like iscsi_cmd header. But the in-coming headers are always size_of(struct iscsi_hdr). So there is no symmetry here. Actually only iscsi_cmd can get big, the other out-going data packets are with small headers, but I guess that is an open-iscsi limitation. [Karen] Here the max size is the packet size (i.e., the total pdu size). The h/w does support sending and recv AHS. I will make sure the code is consistent in handling of AHS. Mike correct me if I'm wrong? > +/** > + * cxgb3i_reserve_itt - generate tag for a give task > + * Try to set up ddp for a scsi read task. > + * @task: iscsi task > + * @hdr_itt: tag, filled in by this function > + */ > +int cxgb3i_reserve_itt(struct iscsi_task *task, itt_t *hdr_itt) > +{ > + struct scsi_cmnd *sc = task->sc; > + struct iscsi_conn *conn = task->conn; > + struct iscsi_session *sess = conn->session; > + struct iscsi_tcp_conn *tcp_conn = conn->dd_data; > + struct cxgb3i_conn *cconn = tcp_conn->dd_data; > + struct cxgb3i_adapter *snic = cconn->hba->snic; > + struct cxgb3i_tag_format *tformat = &snic->tag_format; > + u32 sw_tag = (sess->age << cconn->task_idx_bits) | task->itt; > + u32 tag; > + int err = -EINVAL; > + > + if (sc && (sc->sc_data_direction == DMA_FROM_DEVICE) && > + cxgb3i_sw_tag_usable(tformat, sw_tag)) { OK. In bidi, sc_data_direction == DMA_TO_DEVICE, but scsi_in(sc) will return an sdb just the same. So the check here must be on scsi_in(sc) != NULL or "... && scsi_bidi_cmnd(sc)". Because we do need to initialize a read also for bidi. But on the other side, from what I understand in the bidi case, the current code will just return an cxgb3i_set_non_ddp_tag() which is maybe better. That is, with bidi commands we don't do DDP. I guess this is a good thing then. Strike out my comment above. [Karen] Will change it to check scsi_in(sc). > +int cxgb3i_conn_alloc_pdu(struct iscsi_task *task, u8 opcode) > +{ > + struct iscsi_tcp_task *tcp_task = task->dd_data; > + struct sk_buff *skb; > + > + task->hdr = NULL; > + skb = alloc_skb(sizeof(struct iscsi_hdr) + TX_HEADER_LEN, GFP_ATOMIC); BUG: See below + skb = alloc_skb(sizeof(struct iscsi_hdr) + ISCSI_DIGEST_SIZE + TX_HEADER_LEN, GFP_ATOMIC); > + if (!skb) > + return -ENOMEM; > + > + cxgb3i_tx_debug("task 0x%p, opcode 0x%x, skb 0x%p.\n", > + task, opcode, skb); > + > + tcp_task->dd_data = skb; > + skb_reserve(skb, TX_HEADER_LEN); > + skb_put(skb, sizeof(struct iscsi_hdr)); > + task->hdr = (struct iscsi_hdr *)skb->data; > + task->hdr_max = sizeof(struct iscsi_hdr); > + 3 things 1. task->hdr_max should be without the digest (4 bytes) but if digest is enabled then it is assumed that there is place for it beyond hdr_max. In short: task->hdr_max = allocated_size - ISCSI_DIGEST_SIZE. [Karen] Since the h/w will insert the digest there is no need to allocate the space for it. The h/w will take care of inserting the bytes and shift the payload after the digest. 2. task->hdr_max = sizeof(struct iscsi_hdr); is the very bare minimum. No bidi and no AHSs. All you need to do to enable them at the libiscsi level is give them more room here. See: iscsi_sw_tcp_pdu_alloc() in iscsi_tcp.c [Karen] Will change the code to allocate max. ahs bytes always. 3. At this stage libiscsi does not know what the size of the command header will be It will only know that later after it prepars the header, just allocated here. So the above: + skb_put(skb, sizeof(struct iscsi_hdr)); Will need to be set later mabe at cxgb3i_conn_init_pdu() and it will need to be + skb_put(skb, task->hdr_len); task->hdr_len starts with Zero, and acumulates the total command header size once libiscsi::iscsi_prep_scsi_cmd_pdu() is finished. [Karen] Will move skb_put() call to cxgb3i_conn_init_pdu(). ��.n��������+%������w��{.n�����{������ܨ}���Ơz�j:+v�����w����ޙ��&�)ߡ�a����z�ޗ���ݢj��w�f
