> -----Original Message----- > From: Matan Barak [mailto:matanb@xxxxxxxxxxxx] > Sent: Sunday, August 31, 2014 1:39 PM > To: Devesh Sharma; Jason Gunthorpe; Or Gerlitz > Cc: Doug Ledford; Roland Dreier; Yishai Hadas; linux-rdma@xxxxxxxxxxxxxxx > Subject: Re: [PATCH libibverbs V5 2/2] Use neighbour lookup for RoCE UD > QPs Eth L2 resolution > > 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@xxxxxxxxxxxx] > >> Sent: Thursday, August 28, 2014 9:51 PM > >> To: Devesh Sharma; Jason Gunthorpe; Or Gerlitz > >> Cc: Doug Ledford; Roland Dreier; Yishai Hadas; > >> linux-rdma@xxxxxxxxxxxxxxx > >> 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. > Okay, got it. > >> 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. Since, libocrdma does ah creation purely in kernel through uverbs interface, such patch is required to be posted. > > >> > >> Matan > >> > >>>> -----Original Message----- > >>>> From: linux-rdma-owner@xxxxxxxxxxxxxxx [mailto:linux-rdma- > >>>> owner@xxxxxxxxxxxxxxx] 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@xxxxxxxxxxxxxxx > >>>> 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@xxxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx 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@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
