-----Original Message-----
From: Boaz Harrosh [mailto:[EMAIL PROTECTED]
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().
--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"open-iscsi" group.
To post to this group, send email to open-iscsi@googlegroups.com
To unsubscribe from this group, send email to [EMAIL PROTECTED]
For more options, visit this group at http://groups.google.com/group/open-iscsi
-~----------~----~----~----~------~----~------~--~---
