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? > 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? > > 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
