On Thu, Feb 9, 2023 7:21 PM Li, Zhijian/李 智坚 wrote:
>
>
>
> On 09/02/2023 16:14, Matsuda, Daisuke/松田 大輔 wrote:
> > On Wed, Feb 8, 2023 5:41 PM Li, Zhijian/李 智坚 wrote:
> >>
> >> On 02/02/2023 12:42, Bob Pearson wrote:
> >>> +/* resolve the packet rkey to qp->resp.mr or set qp->resp.mr to NULL
> >>> + * if an invalid rkey is received or the rdma length is zero. For middle
> >>> + * or last packets use the stored value of mr.
> >>> + */
> >>> static enum resp_states check_rkey(struct rxe_qp *qp,
> >>> struct rxe_pkt_info *pkt)
> >>> {
> >>> @@ -473,10 +487,12 @@ static enum resp_states check_rkey(struct rxe_qp *qp,
> >>> return RESPST_EXECUTE;
> >>> }
> >>>
> >>> - /* A zero-byte op is not required to set an addr or rkey. See C9-88 */
> >>> + /* A zero-byte read or write op is not required to
> >>> + * set an addr or rkey. See C9-88
> >>> + */
> >>> if ((pkt->mask & RXE_READ_OR_WRITE_MASK) &&
> >>> - (pkt->mask & RXE_RETH_MASK) &&
> >>> - reth_len(pkt) == 0) {
> >>> + (pkt->mask & RXE_RETH_MASK) && reth_len(pkt) == 0) {
> >>> + qp->resp.mr = NULL;
> >>
> >> You are making sure 'qp->resp.mr = NULL', but I doubt the previous qp->resp.mr will leak after this assignment
> when its
> >> value is not NULL.
> >
> > I do not see what you mean by " the previous qp->resp.mr ".
> > As far as I understand, 'qp->resp.mr' is set to NULL in cleanup()
> > before responder completes its work, so it is not supposed to be
> > reused unlike 'res->read.rkey' being retained for multi-packet Read
> > responses. Could you elaborate on your view?
>
> IMO every 'qp->resp.mr' assignment will take a mr reference, so we should drop the this reference if we want to assign it
> a new mr again.
>
>
> Theoretically, it should be changed to something like
> if (qp->resp.mr) {
> rxe_put(qp->resp.mr)
> qp->resp.mr = NULL;
> }
You are correct about what you wrote.
' qp->resp.mr = NULL;' and 'rxe_put(qp->resp.mr);' come in pairs in
other parts of rxe. I agree the same principle should be applied here.
However, it seems to me that this 'qp->resp.mr' is always NULL. For all
operations other than multi-packet Read responses, cleanup() must be
executed before responder completes the work. On the other hand, for
multi-packet Read replies, responder does not execute cleanup() and thus
does not drop the reference until the last reply is sent, but it does not
execute check_rkey() either since it uses the fast path jumping from
get_req() to read_reply().
>From practical perspective, we can just remove ' qp->resp.mr = NULL;',
and then 'qp->resp.mr' will still be NULL. From theoretical perspective,
we can make the change as Zhijian suggested. What other guys think
about this?
Thanks,
Daisuke
>
>
>
>
> >>
> >>> return RESPST_EXECUTE;
> >>> }
> >>>
> >>> @@ -555,6 +571,7 @@ static enum resp_states check_rkey(struct rxe_qp *qp,
> >>> return RESPST_EXECUTE;
> >>>
> >>> err:
> >>> + qp->resp.mr = NULL;
>
> ditto
>
> Thanks
> Zhijian
>
> >>> if (mr)
