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;
}
>>
>>> 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)
