| Message ID | 20190610135527.2638-1-honli@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [rdma-core,ibacm,v2] ibacm: only open InfiniBand port | expand |
> On 10 Jun 2019, at 15:55, Honggang Li <honli@redhat.com> wrote: > > The low 64 bits of cxgb3 and cxgb4 devices' GID are zeros. If the > "provider" was set in the option file, ibacm will fail with > segment fault. > > $ sed -i -e 's/# provider ibacmp 0xFE80000000000000/provider ibacmp 0xFE80000000000000/g' /etc/rdma/ibacm_opts.cfg > $ /usr/sbin/ibacm --systemd > Segmentation fault (core dumped) > $ gdb /usr/sbin/ibacm core.ibacm > (gdb) bt > 0 0x00005625a4809217 in acm_assign_provider (port=0x5625a4bc6f28) at /usr/src/debug/rdma-core-25.0-1.el8.x86_64/ibacm/src/acm.c:2285 > 1 acm_port_up (port=0x5625a4bc6f28) at /usr/src/debug/rdma-core-25.0-1.el8.x86_64/ibacm/src/acm.c:2372 > 2 0x00005625a48073d2 in acm_activate_devices () at /usr/src/debug/rdma-core-25.0-1.el8.x86_64/ibacm/src/acm.c:2564 > 3 main (argc=<optimized out>, argv=<optimized out>) at /usr/src/debug/rdma-core-25.0-1.el8.x86_64/ibacm/src/acm.c:3270 > > Note: The rpm was built with tarball generated from upstream repo. The last > commit is aa41a65ec86bdb9c1c86e57885ee588b39558238. > > acm_open_dev function should not open an umad port for iWARP or RoCE devices. It is "a umad port" (as suggested), even though the "u" is a vowel, it is pronounced as a consonant, hence "a umad port". > Signed-off-by: Honggang Li <honli@redhat.com> > --- > ibacm/src/acm.c | 26 ++++++++++++++++++++++---- > 1 file changed, 22 insertions(+), 4 deletions(-) > > diff --git a/ibacm/src/acm.c b/ibacm/src/acm.c > index a21069d4..5c8a5d3c 100644 > --- a/ibacm/src/acm.c > +++ b/ibacm/src/acm.c > @@ -2600,9 +2600,11 @@ static void acm_open_dev(struct ibv_device *ibdev) > { > struct acmc_device *dev; > struct ibv_device_attr attr; > + struct ibv_port_attr port_attr; > struct ibv_context *verbs; > size_t size; > int i, ret; > + unsigned int opened_ib_port_cnt = 0; > > acm_log(1, "%s\n", ibdev->name); > verbs = ibv_open_device(ibdev); > @@ -2628,13 +2630,29 @@ static void acm_open_dev(struct ibv_device *ibdev) > list_head_init(&dev->prov_dev_context_list); > > for (i = 0; i < dev->port_cnt; i++) { > + acm_log(1, "%s port %d\n", ibdev->name, i + 1); > + ret = ibv_query_port(dev->device.verbs, i + 1, &port_attr); > + if (ret) { > + acm_log(0, "ERROR - ibv_query_port failed\n"); With the richness below when printing port or ports, may be add the value of ret here as well? > + continue; > + } > + if (port_attr.link_layer != IBV_LINK_LAYER_INFINIBAND) { > + acm_log(1, "not an InfiniBand port\n"); > + continue; > + } > + > acm_open_port(&dev->port[i], dev, i + 1); > + opened_ib_port_cnt++; > } > > - list_add(&dev_list, &dev->entry); > - > - acm_log(1, "%s opened\n", ibdev->name); > - return; > + if (opened_ib_port_cnt > 0) { or simpler, if (opened_ib_port_cnt) { > + list_add(&dev_list, &dev->entry); > + acm_log(1, "%d InfiniBand %s opened for %s\n", > + opened_ib_port_cnt, > + opened_ib_port_cnt == 1 ? "port":"ports", Spaces around ":". I am also OK with the literal "port(s)" as well. Otherwise, LGTM, Signed-off-by: Håkon Bugge <haakon.bugge@oracle.com> Thxs, Håkon > + ibdev->name); > + return; > + } > > err1: > ibv_close_device(verbs); > -- > 2.20.1 >
On Tue, Jun 11, 2019 at 02:29:31PM +0200, Håkon Bugge wrote: > > > > On 10 Jun 2019, at 15:55, Honggang Li <honli@redhat.com> wrote: > > > > The low 64 bits of cxgb3 and cxgb4 devices' GID are zeros. If the > > "provider" was set in the option file, ibacm will fail with > > segment fault. > > > > $ sed -i -e 's/# provider ibacmp 0xFE80000000000000/provider ibacmp 0xFE80000000000000/g' /etc/rdma/ibacm_opts.cfg > > $ /usr/sbin/ibacm --systemd > > Segmentation fault (core dumped) > > $ gdb /usr/sbin/ibacm core.ibacm > > (gdb) bt > > 0 0x00005625a4809217 in acm_assign_provider (port=0x5625a4bc6f28) at /usr/src/debug/rdma-core-25.0-1.el8.x86_64/ibacm/src/acm.c:2285 > > 1 acm_port_up (port=0x5625a4bc6f28) at /usr/src/debug/rdma-core-25.0-1.el8.x86_64/ibacm/src/acm.c:2372 > > 2 0x00005625a48073d2 in acm_activate_devices () at /usr/src/debug/rdma-core-25.0-1.el8.x86_64/ibacm/src/acm.c:2564 > > 3 main (argc=<optimized out>, argv=<optimized out>) at /usr/src/debug/rdma-core-25.0-1.el8.x86_64/ibacm/src/acm.c:3270 > > > > Note: The rpm was built with tarball generated from upstream repo. The last > > commit is aa41a65ec86bdb9c1c86e57885ee588b39558238. > > > > acm_open_dev function should not open an umad port for iWARP or RoCE devices. > > It is "a umad port" (as suggested), even though the "u" is a vowel, it is pronounced as a consonant, hence "a umad port". ok, fixed. > > > Signed-off-by: Honggang Li <honli@redhat.com> > > --- > > ibacm/src/acm.c | 26 ++++++++++++++++++++++---- > > 1 file changed, 22 insertions(+), 4 deletions(-) > > > > diff --git a/ibacm/src/acm.c b/ibacm/src/acm.c > > index a21069d4..5c8a5d3c 100644 > > --- a/ibacm/src/acm.c > > +++ b/ibacm/src/acm.c > > @@ -2600,9 +2600,11 @@ static void acm_open_dev(struct ibv_device *ibdev) > > { > > struct acmc_device *dev; > > struct ibv_device_attr attr; > > + struct ibv_port_attr port_attr; > > struct ibv_context *verbs; > > size_t size; > > int i, ret; > > + unsigned int opened_ib_port_cnt = 0; > > > > acm_log(1, "%s\n", ibdev->name); > > verbs = ibv_open_device(ibdev); > > @@ -2628,13 +2630,29 @@ static void acm_open_dev(struct ibv_device *ibdev) > > list_head_init(&dev->prov_dev_context_list); > > > > for (i = 0; i < dev->port_cnt; i++) { > > + acm_log(1, "%s port %d\n", ibdev->name, i + 1); > > + ret = ibv_query_port(dev->device.verbs, i + 1, &port_attr); > > + if (ret) { > > + acm_log(0, "ERROR - ibv_query_port failed\n"); > > With the richness below when printing port or ports, may be add the value of ret here as well? sure, fixed. > > > + continue; > > + } > > + if (port_attr.link_layer != IBV_LINK_LAYER_INFINIBAND) { > > + acm_log(1, "not an InfiniBand port\n"); > > + continue; > > + } > > + > > acm_open_port(&dev->port[i], dev, i + 1); > > + opened_ib_port_cnt++; > > } > > > > - list_add(&dev_list, &dev->entry); > > - > > - acm_log(1, "%s opened\n", ibdev->name); > > - return; > > + if (opened_ib_port_cnt > 0) { > > or simpler, if (opened_ib_port_cnt) { ok, fixed. > > > + list_add(&dev_list, &dev->entry); > > + acm_log(1, "%d InfiniBand %s opened for %s\n", > > + opened_ib_port_cnt, > > + opened_ib_port_cnt == 1 ? "port":"ports", > > Spaces around ":". I am also OK with the literal "port(s)" as well. fixed. please see v3 for details. > > Otherwise, LGTM, > > Signed-off-by: Håkon Bugge <haakon.bugge@oracle.com> > > > Thxs, Håkon > > > > + ibdev->name); > > + return; > > + } > > > > err1: > > ibv_close_device(verbs); > > -- > > 2.20.1 > > >
diff --git a/ibacm/src/acm.c b/ibacm/src/acm.c index a21069d4..5c8a5d3c 100644 --- a/ibacm/src/acm.c +++ b/ibacm/src/acm.c @@ -2600,9 +2600,11 @@ static void acm_open_dev(struct ibv_device *ibdev) { struct acmc_device *dev; struct ibv_device_attr attr; + struct ibv_port_attr port_attr; struct ibv_context *verbs; size_t size; int i, ret; + unsigned int opened_ib_port_cnt = 0; acm_log(1, "%s\n", ibdev->name); verbs = ibv_open_device(ibdev); @@ -2628,13 +2630,29 @@ static void acm_open_dev(struct ibv_device *ibdev) list_head_init(&dev->prov_dev_context_list); for (i = 0; i < dev->port_cnt; i++) { + acm_log(1, "%s port %d\n", ibdev->name, i + 1); + ret = ibv_query_port(dev->device.verbs, i + 1, &port_attr); + if (ret) { + acm_log(0, "ERROR - ibv_query_port failed\n"); + continue; + } + if (port_attr.link_layer != IBV_LINK_LAYER_INFINIBAND) { + acm_log(1, "not an InfiniBand port\n"); + continue; + } + acm_open_port(&dev->port[i], dev, i + 1); + opened_ib_port_cnt++; } - list_add(&dev_list, &dev->entry); - - acm_log(1, "%s opened\n", ibdev->name); - return; + if (opened_ib_port_cnt > 0) { + list_add(&dev_list, &dev->entry); + acm_log(1, "%d InfiniBand %s opened for %s\n", + opened_ib_port_cnt, + opened_ib_port_cnt == 1 ? "port":"ports", + ibdev->name); + return; + } err1: ibv_close_device(verbs);
The low 64 bits of cxgb3 and cxgb4 devices' GID are zeros. If the "provider" was set in the option file, ibacm will fail with segment fault. $ sed -i -e 's/# provider ibacmp 0xFE80000000000000/provider ibacmp 0xFE80000000000000/g' /etc/rdma/ibacm_opts.cfg $ /usr/sbin/ibacm --systemd Segmentation fault (core dumped) $ gdb /usr/sbin/ibacm core.ibacm (gdb) bt 0 0x00005625a4809217 in acm_assign_provider (port=0x5625a4bc6f28) at /usr/src/debug/rdma-core-25.0-1.el8.x86_64/ibacm/src/acm.c:2285 1 acm_port_up (port=0x5625a4bc6f28) at /usr/src/debug/rdma-core-25.0-1.el8.x86_64/ibacm/src/acm.c:2372 2 0x00005625a48073d2 in acm_activate_devices () at /usr/src/debug/rdma-core-25.0-1.el8.x86_64/ibacm/src/acm.c:2564 3 main (argc=<optimized out>, argv=<optimized out>) at /usr/src/debug/rdma-core-25.0-1.el8.x86_64/ibacm/src/acm.c:3270 Note: The rpm was built with tarball generated from upstream repo. The last commit is aa41a65ec86bdb9c1c86e57885ee588b39558238. acm_open_dev function should not open an umad port for iWARP or RoCE devices. Signed-off-by: Honggang Li <honli@redhat.com> --- ibacm/src/acm.c | 26 ++++++++++++++++++++++---- 1 file changed, 22 insertions(+), 4 deletions(-)