RE: Can someone help me understand the reason for this code in ib_isert.c?

[Chris Moore <Chris.Moore@Emulex.Com>]

> 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@emulex.com> 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

Sorry for the long delay, I was dealing with some hardware issues.

I've tested the proposed patch and it fixes the ocrdma issue.

Tested-by: Chris Moore <chris.moore@emulex.com>

Thanks,

Chris