On Wed, 2014-10-22 at 12:02 +0300, Or Gerlitz wrote: > On 10/22/2014 8:06 AM, Nicholas A. Bellinger wrote: > > On Tue, 2014-10-21 at 00:13 +0300, Or Gerlitz wrote: > >> On Mon, Oct 20, 2014 at 6:29 PM, Chris Moore <Chris.Moore@xxxxxxxxxx> wrote: > >>> The following code is in isert_conn_setup_qp() in ib_isert.c: > >>> > >>> /* > >>> * FIXME: Use devattr.max_sge - 2 for max_send_sge as > >>> * work-around for RDMA_READ.. > >>> */ > >>> attr.cap.max_send_sge = device->dev_attr.max_sge - 2; > >>> > >>> It's not clear from the comment what this is a work-around for, and I wasn't able > >>> to figure it out from looking at logs. > >> I believe this refers to some IBTA spec corner case which comes into > >> play with the max_sges advertized by mlx4, Eli, can you shed some > >> light (IBTA pointer) on that? is this the case (i.e dev_attr.max_sge > >> isn't always achievable) with mlx5 too? > >> > > It's for ConnectX-2 reporting max_sge=31 during development, while the > > largest max_send_sge for stable large block RDMA reads was (is..?) 29 > > SGEs. > > > >>> In trying to get isert working with ocrdma I ran into a problem where the code > >>> thought there was only 1 send SGE available when in fact the device has 3. > >>> Then I found the above work-around, which explained the problem. > >> Nic, any reason for attr.cap.max_send_sge = 1 not to work OK? > >> > > IIRC, pre fastreg code supported max_send_sge=1 by limiting the transfer > > size per RDMA read, and would issue subsequent RDMA read + offset from > > completion up to the total se_cmd->data_length. > > > > Not sure how this works with fastreg today. Sagi..? > > > > While on fastreg mode, for RDMA reads we use only one SGE, talking to > Sagi he explained me that the problem with creating the QP with > max_send_sge = 1 has to do with other flows where two or even three SGEs > are needed, e.g when the iscsi/iser header (doesn't exist in RDMA ops) > is taken from one spot of the memory and the data (sense) taken from > another one? > > Nic, we need to see what is the minimum needed by the code (two?) and > tweak that line to make sure it gets there as long as the device > supports it, SB, makes sense? > > > diff --git a/drivers/infiniband/ulp/isert/ib_isert.c > b/drivers/infiniband/ulp/isert/ib_isert.c > index 0bea577..24abbb3 100644 > --- a/drivers/infiniband/ulp/isert/ib_isert.c > +++ b/drivers/infiniband/ulp/isert/ib_isert.c > @@ -115,9 +115,10 @@ isert_conn_setup_qp(struct isert_conn *isert_conn, > struct rdma_cm_id *cma_id, > attr.cap.max_recv_wr = ISERT_QP_MAX_RECV_DTOS; > /* > * FIXME: Use devattr.max_sge - 2 for max_send_sge as > - * work-around for RDMA_READ.. > + * work-around for RDMA_READ.. still need to make sure to > + * have at least two SGEs for SCSI responses. > */ > - attr.cap.max_send_sge = device->dev_attr.max_sge - 2; > + attr.cap.max_send_sge = max(2, device->dev_attr.max_sge - 2); > isert_conn->max_sge = attr.cap.max_send_sge; > > attr.cap.max_recv_sge = 1; > > After a quick audit, the minimum max_send_sge=2 patch looks correct for the hardcoded tx_desc->num_sge cases in isert_put_login_tx(), isert_put_response(), isert_put_reject() and isert_put_text_rsp(). For the RDMA read path with non fast-reg, isert_build_rdma_wr() is still correctly limiting the SGEs per operation based upon the negotiated isert_conn->max_sge set above in isert_conn_setup_qp(). Chris, please confirm on ocrdma with Or's patch, and I'll include his patch into target-pending/queue for now, and move into master once you give a proper Tested-By. Thanks! --nab -- To unsubscribe from this list: send the line "unsubscribe target-devel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
