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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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 provider library tn new kernels only.
I would like to avoid creating such a dependency.

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




[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Photo]     [Yosemite News]     [Yosemite Photos]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux