[Openib-windows] Bug or by design?

[Saul]

I've been thinking about this a little more, isn't there still an issue 
here?  Basically this second patch makes it so that you can never set the 
SQ_PSN on an RTS->RTS change.  This is because a) we are saying that on the 
RTS->RTS transition SQ_PSN is not a required param, but b) we are not 
providing a flag for the rts.opts field to indicate that we want to 
optionally set this field.   The patch I posted originally made it so that 
you always had to specify the SQ_PSN.  The core of the problem is basically 
that SQ_PSN is required for RTR->RTS but we want it to be optional for 
RTS->RTS.  In that case, it seems like what we really need is an 
IB_MOD_QP_xxx flag so that the SQ_PSN can be optional, but we need addtional 
logic to only allow it to be optional if the current state is already RTS. 
Would it not be simpler/cleaner to just mandate that SQ_PSN always has to be 
specified for any transition to RTS, even if the current state is already 
RTS?

Thanks


----- Original Message ----- 
From: "Leonid Keller" <leonid at mellanox.co.il>
To: "Saul" <26oo at cox.net>
Sent: Tuesday, August 15, 2006 1:30 PM
Subject: RE: [Openib-windows] Bug or by design?


yep

> -----Original Message-----
> From: Saul [mailto:26oo at cox.net]
> Sent: Tuesday, August 15, 2006 9:18 PM
> To: Leonid Keller; Fabian Tillier
> Cc: openib-windows at openib.org
> Subject: Re: [Openib-windows] Bug or by design?
>
> Ok, that makes sense.
>
> So are you thinking something like this instead?
>
> at line 738 in hca_data.c
>
>          case IB_QPT_UNRELIABLE_DGRM:
>          case IB_QPT_QP0:
>          case IB_QPT_QP1:
>          default:
>             if (qp_p->state != IB_QPS_RTS) {
>                *qp_attr_mask_p |= /* required flags */
>                   IB_QP_SQ_PSN;
>             }
>
>
> ----- Original Message -----
> From: "Leonid Keller" <leonid at mellanox.co.il>
> To: "Saul" <26oo at cox.net>; "Fabian Tillier" <ftillier at silverstorm.com>
> Cc: <openib-windows at openib.org>
> Sent: Tuesday, August 15, 2006 1:10 PM
> Subject: RE: [Openib-windows] Bug or by design?
>
>
> I believe, your patch
>
> <       t[IBQPS_RTS].req_param[UD]      = IB_QP_SQ_PSN;
> <       t[IBQPS_RTS].req_param[UC]      = IB_QP_SQ_PSN;
> <       t[IBQPS_RTS].req_param[RC]      = IB_QP_TIMEOUT
> |IB_QP_RETRY_CNT
>
> |IB_QP_RNR_RETRY |IB_QP_SQ_PSN |IB_QP_MAX_QP_RD_ATOMIC;
> <       t[IBQPS_RTS].req_param[MLX]     = IB_QP_SQ_PSN;
>
>  is a not a best solution, because IB_QP_SQ_PSN is not a required
> parameter for RTS-RTS transition.
>
> You've pinpoint a real problem: mlnx_conv_qp_modify_attr() routine
> doesn't check the type of transition.
> For example, in your case it presumes, that one performs an RTR-RTS
> transition and that's why it sets the required IB_QP_SQ_PSN flag.
> [BTW, there is no mapping from IB_MOD_QP_xxx to IB_QP_SQ_PSN, just
> because it is a required parameter]
>
> I'm working on a patch now.
>
>
> > -----Original Message-----
> > From: Saul [mailto:26oo at cox.net]
> > Sent: Monday, August 14, 2006 9:24 PM
> > To: Fabian Tillier; Leonid Keller
> > Cc: openib-windows at openib.org
> > Subject: Re: [Openib-windows] Bug or by design?
> >
> > I later discovered that you cannot go from RTS -> RTR -> RTS,
> > so I scrapped that idea.  I made another post to the list
> > with the "patch" that I put into mthca_qp.c to fix things.
> > It appears to me that there just needed to be some required
> > params specified for the RTS->RTS state change, just like
> > there are for the RTR->RTS change.  Without anything being
> > specified as required, but with the routine
> > mlnx_conv_qp_modify_attr() forcing IB_QP_SQ_PSN to be
> > required (line 743) it was a no win situation.  If there was
> > an IB_MOD_QP_xxx flag for SQ_PSN I would be fine, but since
> > there is not I was stuck.
> >
> > Please let me know if the changes that I made to mthca_qp.c
> > are incorrect.
> >
> > Thanks
> >
> > ----- Original Message -----
> > From: "Fabian Tillier" <ftillier at silverstorm.com>
> > To: "Saul" <26oo at cox.net>; "Leonid Keller" <leonid at mellanox.co.il>
> > Cc: <openib-windows at openib.org>
> > Sent: Monday, August 14, 2006 12:23 PM
> > Subject: Re: [Openib-windows] Bug or by design?
> >
> >
> > > Hi Saul,
> > >
> > > On 8/12/06, Saul <26oo at cox.net> wrote:
> > >>
> > >> Hello,
> > >>
> > >> I am having a problem updating the qkey of a QP that is in
> > the RTS state.
> > >> The API I'm using is
> > >>
> > >> ib_modify_qp(const ib_qp_handle_t h_qp, const ib_qp_mod_t *const
> > >> p_qp_mod);
> > >>
> > >> I set qp_mod.req_state (to IB_QPS_RTS),
> > qp_mod.state.rts.qkey (to my new
> > >> qkey) and qp_mod.state.rts.opts (to IB_MOD_QP_KEY).
> > >>
> > >> The problem is that down in the mthca driver it uses the
> > integer style
> > >> attribute mask.  In the routine mlnx_conv_qp_modify_attr()
> > the code adds
> > >> the
> > >> attribute flag IB_QP_SQ_PSN as a required flag.  I can set
> > this field in
> > >> qp_mod without any problem but there is no IB_MOD_QP_xxx
> > flag that maps
> > >> to
> > >> IB_QP_SQ_PSN.  Therefore the check at line 562 in
> > mthca_qp.c fails the
> > >> operation.
> > >>
> > >> So my question is:  is this supposed to operate this way?
> > Can I not call
> > >> ib_modify_qp() while at RTS if I want to stay at RTS?  I
> > would think that
> > >> this is ok because it is not flagged as an invalid state
> > transition in
> > >> the
> > >> driver.  Is there some other API that I should use to
> > perform this qkey
> > >> update?  Or do I need to transition to RTR, then back to
> > RTS to avoid
> > >> this?
> > >
> > > This should work.  You can't move back from RTS to RTR without
> > > resetting the QP first and moving it back through INIT, which will
> > > break your connection.
> > >
> > > Leonid, can you look into this?
> > >
> > > Thanks,
> > >
> > > - Fab
> > >
> >
> >
>
>