Re: [PATCH libibverbs V5 2/2] Use neighbour lookup for RoCE UD QPs Eth L2 resolution

[Matan Barak <matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>]

On 28/8/2014 8:48 PM, Devesh Sharma wrote:
> Hi Matan,
>
> Thanks for quick response, my further comments are inline below:
>
>> -----Original Message-----
>> From: Matan Barak [mailto:matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org]
>> Sent: Thursday, August 28, 2014 9:51 PM
>> To: Devesh Sharma; Jason Gunthorpe; Or Gerlitz
>> Cc: Doug Ledford; Roland Dreier; Yishai Hadas; linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> Subject: Re: [PATCH libibverbs V5 2/2] Use neighbour lookup for RoCE UD
>> QPs Eth L2 resolution
>>
>> On 28/8/2014 12:48 PM, Devesh Sharma wrote:
>>> Hi Matan,
>>>
>>> I have been watching this thread for quite some time. I have a Basic
>>> question, do you think ib_uverbs_create_ah() in uverbs_cmd.c Should
>>> resolve to l2 address? Presently it is not calling
>> rdma_addr_find_dmac_by_grh(), am I missing something here?
>>>
>>> -Regards
>>>    Devesh
>>>
>>
>> Hi Devesh,
>>
>> Some vendors don't call ib_uverbs_create_ah and do all this creation in
>> userspace only. It's true that it might be a lot easier to do that resolution in
>> kernel, but it could create dependency of new versions of libibverbs and the
> Which dependency you are specifying here, I see the scheme posted in this series adds dependency on libibverbs.
> Do you anticipate modification even when uverbs interface is used?

If we had resolved the address handle in kernel space, we would have 
created a dependency between the user libraries to and new version odf 
the uverbs module. The scheme posted here only adds a dependency between 
the provider's library to libibverbs.
Resolving in kernel space would still require creating a dependency on a 
new provider library - as currently some providers don't even call the 
kernel when a new address handle is created.

>> provider library tn new kernels only.
>> I would like to avoid creating such a dependency.
>
> Okay, got it, any suggestions for those who use uverbs interface.
> I think another patch is needed for such vendors?
>

There are 2 options here - if all the required information (including 
the MAC address) is contained in the address handle user-space 
structure, you could just use the proposed method. However, if you 
register some kind of an address handle with the hardware and then only 
use it via user-space, we'll need another patch for uverbs.

>>
>> Matan
>>
>>>> -----Original Message-----
>>>> From: linux-rdma-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org [mailto:linux-rdma-
>>>> owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org] On Behalf Of Jason Gunthorpe
>>>> Sent: Tuesday, August 26, 2014 9:39 PM
>>>> To: Or Gerlitz
>>>> Cc: Doug Ledford; Matan Barak; Roland Dreier; Yishai Hadas; linux-
>>>> rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>>>> Subject: Re: [PATCH libibverbs V5 2/2] Use neighbour lookup for RoCE
>>>> UD QPs Eth L2 resolution
>>>>
>>>> On Tue, Aug 26, 2014 at 03:18:37PM +0300, Or Gerlitz wrote:
>>>>> On 25/08/2014 22:33, Doug Ledford wrote:
>>>>>>> On Aug 25, 2014, at 1:33 PM, Jason Gunthorpe
>>>> <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> wrote:
>>>>>>>
>>>>>>>>>> +    timer_fd = timerfd_create(CLOCK_MONOTONIC,
>>>> TFD_NONBLOCK | TFD_CLOEXEC);
>>>>>>>>>> +    if (-1 == timer_fd) {
>>>>>>>>>> +        print_err("Couldn't create timer\n");
>>>>>>>>>> +        return timer_fd;
>>>>>>>>>> +    }
>>>>>>>>>
>>>>>>>>> The use of timerfd will impact the minimum OS version, have you
>>>>>>>>> checked this is OK? Does RHEL5 still work?
>>>>>>>
>>>>>>>> It was added in linux v2.6.25. I think that an API that's more
>>>>>>>> than
>>>>>>>> 6.5 years old is valid.
>>>>>>>
>>>>>>> RHEL5 is using 2.6.18 as their base kernel. You should at least
>>>>>>> consult with the OFED people to determine if this is a problem for
>>>>>>> them.
>>>>>>
>>>>>> Please don't.  This code should not be changed for something as
>>>>>> ancient
>>>> as rhel5.
>>>>>
>>>>> Indeed. Telling people to avoid using constructs/mechanisms ~6-7
>>>>> years after they were introduced isn't something we want nor need to
>> do.
>>>>
>>>> I looked myself and it looks like OFED has dropped support for these
>>>> old distros so there isn't any problem.
>>>>
>>>> However, I still think this use of timerfd is fairly gratuitous, and
>>>> looking closer, causes little bugs:
>>>>
>>>> +		if (timerfd_settime(timer_fd, 0, &timer_time, NULL)) {
>>>> +		   			      print_err("Couldn't set timer\n");
>>>> +			return -1;
>>>>                        ^^^^^^^^^^^^^^
>>>> leaks timer_fd
>>>>
>>>>
>>>> Alos, I noticed:
>>>>
>>>> +     	/* wait for an incoming message on the netlink socket */
>>>> +	   ret = select(nfds, &fdset, NULL, NULL, NULL);
>>>> +
>>>> +		if (ret) {
>>>>
>>>> Fails to detect error return from select.
>>>>
>>>> Jason
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe linux-rdma"
>>>> in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More
>> majordomo
>>>> info at http://vger.kernel.org/majordomo-info.html
>

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html