On 2/9/23 04:20, lizhijian@xxxxxxxxxxx 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;
> }
>
Generally I agree with the idea that a (few) assignments like this, even if after minutes of looking
over the code can be shown to be unneeded because it is already NULL or will get deleted, are helpful
if it makes the code easier to understand and prevents later changes to unintentionally break things.
Bob
>
>
>
>>>
>>>> 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)
