Re: [PATCH for-next] IB/cma: Introduce rdma_set_min_rnr_timer()

Haakon Bugge
2021-03-31T17:46:52Z

> On 31 Mar 2021, at 19:39, Jason Gunthorpe <jgg@nvidia.com> wrote:
>
> On Wed, Mar 31, 2021 at 05:38:26PM +0000, Haakon Bugge wrote:
>>
>>
>>> On 31 Mar 2021, at 19:35, Jason Gunthorpe <jgg@nvidia.com> wrote:
>>>
>>> On Wed, Mar 31, 2021 at 05:09:27PM +0000, Parav Pandit wrote:
>>>>
>>>>
>>>>> From: Haakon Bugge <haakon.bugge@oracle.com>
>>>>> Sent: Wednesday, March 31, 2021 8:20 PM
>>>>>
>>>>>> On 31 Mar 2021, at 15:35, Jason Gunthorpe <jgg@nvidia.com> wrote:
>>>>>>
>>>>>> On Wed, Mar 31, 2021 at 01:34:06PM +0000, Haakon Bugge wrote:
>>>>>>
>>>>>>>> Actually I bet you could do this same thing entirely in userspace by
>>>>>>>> adjusting rdma_init_qp_attr() to copy the data that would be stored
>>>>>>>> in the cm_id.. ??
>>>>>>>
>>>>>>> This will definitely not solve the issue for kernel ULP, e.g., RDS.
>>>>>>
>>>>>> Sure, that makes sense to have some rdmacm api in-kernel only
>>>>>
>>>>> Let me send a v2 doing only that.
>>>>>
>>>>>>> Further, why do we have rdma_set_option() with option
>>>>> RDMA_OPTION_ID_ACK_TIMEOUT ?
>>>>>>
>>>>>> It may have been a mistake to do it like that
>>>>>
>>>> Timeout value goes in the CM request message so setting it through
>>>> the cm_id object was likely correct. This reflects into cm msg as
>>>> well as in the QP of the cm_id.
>>>
>>> Ah, yes if it goes in the wire in a CM message it has to go to the
>>> kernel.
>>
>> But does it go on the wire? No. The RNR Retry timer is not part of
>> the negotiation with the peer.
>
> I think Parav was talking about the ID_ACK_TIMEOUT

True. Local ACK Timeout is part of CM REQ. But that makes 2c1619edef61 ("IB/cma: Define option to set ack timeout and pack tos_set") fuzzy, as it claims in the commentary: "The timeout will affect the local side of the QP, it is not negotiated with remote side ..."

Håkon

>
> Jason