[Fwd: [ofa-general] More responder_resources problems]

[Sean Hefty]

>Yes, but the actual programming of the values into the QP is done by
>cm_init_qp_rtr_attr/cm_init_qp_rts_attr (well, in many cases) - which
>takes the values from the rep/req directly, without modification.

The values exchanged in the REP are saved to cm_id_priv.  Those values are used.
The passive side ULP is responsible for using the correct value.  Either by
returning what was sent in the REQ, or by adjusting the values down.  Note that
the active side will see the values in the REP and can reject the connection if
they are set too large.
 
>There is a bug here, it just isn't really obvious to me where the
>fixes should go to match the CM design. I was imagining that cm.c
>would adjust the REQ after reception, but there may be some downsides
>to that?

The CM does adjust the value in the cm_id_priv structure based on the REP.

>All that I see in here is switching REQ's responder_resources value
>into the REQ's initiator_depth value (and vice versa) it does not
>limit it.

The limits are left up to the ULP.  Maybe the problem is that the ULPs are not
validating the limits?

>Well, I see how the override gets into the REP, but how does the REQ
>get factored into the override? For instance, the rping example does
>this:
>
>        memset(&conn_param, 0, sizeof conn_param);
>        conn_param.responder_resources = 1;
>        conn_param.initiator_depth = 1;
>        ret = rdma_accept(cb->child_cm_id, &conn_param);
>
>And rdma_accept does:
>
>   ret = ucma_valid_param(id_priv, conn_param);
>            [^^ Only checks local device capabilities]

This is a sanity check only, intended to help catch errors sooner.  Since it is
also used on the active side before sending a REQ, it can only check against the
local device capabilities.  The sanity check could be expanded, but I don't see
a strong reason to add it.  The modify QP operations will fail later if the
specified values are too large.

>   ret = ucma_modify_qp_rtr(id, conn_param);
>[.. then on to ucma_modify_qp_rtr .. ]
>        if (conn_param)
>                qp_attr.max_dest_rd_atomic = conn_param->responder_resources;
>        return ibv_modify_qp(id->qp, &qp_attr, qp_attr_mask);
>
>Which just can't be entirely right. The client can specify values that
>are greater than those specified in the REQ. Since the client doesn't
>seem to have access to the REQ prior to calling rdma_accept the
>responsibility to limit the values must fall on librdmacm.

The rdma_conn_param structure reported as part of a connection event carries the
initiator_depth and responder_resources fields in the REQ.  Yes, the client can
specify values that were greater than those in the REQ, but those values may
technically still work.

>Maybe something more like this in ucma_modify_qp_rtr:
>
>if (conn_param) {
>   /* Note: at this point qp_attr.max_dest_rd_atomic is
>      REQ.initiator_depth. */
>   conn_param->responder_resouces = min(conn_param->responder_resouces,
>                                        qp_attr.max_dest_rd_atomic,
>                                        id_priv->cma_dev-
>>max_responder_resources);
>   qp_attr.max_dest_rd_atomic = conn_param->responder_resouces;
>
>   /* Note: at this point qp_attr.max_rd_atomic is
>      REQ.responder_resources. */
>   conn_param->initiator_depth = min(conn_param->initiator_depth,
>                                        qp_attr.max_rd_atomic,
>                                        id_priv->cma_dev->max_initiator_depth);
>   qp_attr.max_rd_atomic = conn_param->initiator_depth;
>}
>
>ie, consider the REQ values as reported through rdma_init_qp_attr,
>and limit the user's requested values on the passive side to be no
>greater than what the remote can do/

I don't like the idea of reducing the limits without the user's knowledge.  I
would rather fail the connection, which is what happens today (either through
the ucma_valid_param() checks or when modifying the QP).

>Also support user passive side control over initiator depth.

This is there today.

I think I'm missing seeing whatever problem you're seeing.

- Sean