Re: [PATCH RDMA/netlink] RDMA/netlink: Adhere to returning zero on success

From: Mark Haywood <mark.haywood@oracle.com>
To: "Håkon Bugge" <haakon.bugge@oracle.com>,
	"Leon Romanovsky" <leon@kernel.org>
Cc: Doug Ledford <dledford@redhat.com>,
	Jason Gunthorpe <jgg@mellanox.com>,
	OFED mailing list <linux-rdma@vger.kernel.org>
Subject: Re: [PATCH RDMA/netlink] RDMA/netlink: Adhere to returning zero on success
Date: Fri, 13 Dec 2019 11:51:58 -0500	[thread overview]
Message-ID: <b4147ad1-1f43-e174-669a-500452bab061@oracle.com> (raw)
In-Reply-To: <c7d1cb3d-c55f-2bca-c616-6f50a619383b@oracle.com>

On 12/12/19 9:14 AM, Mark Haywood wrote:
>
>
> On 12/12/19 7:37 AM, Håkon Bugge wrote:
>>
>>> On 12 Dec 2019, at 13:27, Leon Romanovsky <leon@kernel.org> wrote:
>>>
>>> On Thu, Dec 12, 2019 at 01:16:54PM +0100, Håkon Bugge wrote:
>>>>
>>>>> On 12 Dec 2019, at 13:10, Leon Romanovsky <leon@kernel.org> wrote:
>>>>>
>>>>> On Thu, Dec 12, 2019 at 12:59:51PM +0100, Håkon Bugge wrote:
>>>>>>
>>>>>>> On 12 Dec 2019, at 12:40, Leon Romanovsky <leon@kernel.org> wrote:
>>>>>>>
>>>>>>> On Wed, Dec 11, 2019 at 08:31:18PM +0100, Håkon Bugge wrote:
>>>>>>>>
>>>>>>>>> On 11 Dec 2019, at 14:13, Håkon Bugge 
>>>>>>>>> <haakon.bugge@oracle.com> wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>> On 11 Dec 2019, at 13:39, Leon Romanovsky <leon@kernel.org> 
>>>>>>>>>> wrote:
>>>>>>>>>>
>>>>>>>>>> On Wed, Dec 11, 2019 at 11:34:00AM +0100, Håkon Bugge wrote:
>>>>>>>>>>> In rdma_nl_rcv_skb(), the local variable err is assigned the 
>>>>>>>>>>> return
>>>>>>>>>>> value of the supplied callback function, which could be one of
>>>>>>>>>>> ib_nl_handle_resolve_resp(), ib_nl_handle_set_timeout(), or
>>>>>>>>>>> ib_nl_handle_ip_res_resp(). These three functions all return 
>>>>>>>>>>> skb->len
>>>>>>>>>>> on success.
>>>>>>>>>>>
>>>>>>>>>>> rdma_nl_rcv_skb() is merely a copy of netlink_rcv_skb(). The 
>>>>>>>>>>> callback
>>>>>>>>>>> functions used by the latter have the convention: "Returns 0 on
>>>>>>>>>>> success or a negative error code".
>>>>>>>>>>>
>>>>>>>>>>> In particular, the statement (equal for both functions):
>>>>>>>>>>>
>>>>>>>>>>> if (nlh->nlmsg_flags & NLM_F_ACK || err)
>>>>>>>>>>>
>>>>>>>>>>> implies that rdma_nl_rcv_skb() always will ack a message, 
>>>>>>>>>>> independent
>>>>>>>>>>> of the NLM_F_ACK being set in nlmsg_flags or not.
>>>>>>>>>> The more accurate description is that rdma_nl_rcv_skb() 
>>>>>>>>>> always generates
>>>>>>>>>> NLMSG_ERROR without relation to NLM_F_ACK flag. The NLM_F_ACK 
>>>>>>>>>> flag is
>>>>>>>>>> requested to get acknowledges for the success.
>>>>>>>>
>>>>>>>> Yes. And when, lets say a legitimate path record response, 
>>>>>>>> containing N positive bytes, is sent back from ibacm to the 
>>>>>>>> kernel, rdma_nl_rcv_skb() think this is an error, due to "if 
>>>>>>>> (nlh->nlmsg_flags & NLM_F_ACK || err)" _and_ 
>>>>>>>> ib_nl_handle_resolve_resp() returning N.
>>>>>>> How did you test this patch?
>>>>>>> Do we have open-source applications which don't set NLM_F_ACK for
>>>>>>> ib_nl_*() calls?
>>>>>> As I alluded to above, yes, ibacm doesn't set it.
>>>>> In this regards, I'm amazed that this patch didn't break ibacm.
>>>> On the contrary. The patch avoids the kernel sending back an 
>>>> error/ACK for every path record / resolve response.
>>> As long as ibacm continues to work with this patch, i'm ok.
>>> What type of testing did you perform?
>> I'll let Mark respond to the testing. The background is that ibacm 
>> was very *liberal* when it comes checking the requests it received 
>> from the kernel. In an attempt to tighten that, Mark discovered that 
>> ibacm received an unexpected ACK from the kernel just after having 
>> sent a response.
>
>
> Without this patch, for every response to a RDMA_NL_LS_OP_RESOLVE 
> request, ibacm receives an ACK with a nlmsgerr error value equal to 
> the length of the response message.

To be clear, ibacm does nothing with the ACK other than write a log 
message that it received an unexpected request. That is why this patch 
has no ill-effect on ibacm and is why ...

>
> With this patch, no ACKs are received.

this is preferable.

>
> If I add the NLM_F_ACK to the nlmsg_header flags when responding to 
> the RDMA_NL_LS_OP_RESOLVE request, ibacm once again receives the ACKs.
>
> Mark
>
>
>>
>> That aside, I think the RDMA NL callbacks shall adhere to the 
>> RTNETLINK conventions, thus, that's why this commit changes the 
>> callbacks and not the  rdma_nl_rcv_skb().
>>
>>
>> Thxs, Håkon
>>
>>> Thanks
>>>
>>>>
>>>> Håkon
>>>>
>>>>> Thanks
>