| Message ID | 55157BC6.70400@profitbricks.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Mar 27, 2015 at 04:48:22PM +0100, Michael Wang wrote: > diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c > index 3341754..fcd7558 100644 > +++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c > @@ -1655,7 +1655,7 @@ static void ipoib_add_one(struct ib_device *device) > struct ipoib_dev_priv *priv; > int s, e, p; > > - if (!rdma_transport_is_ib(device)) > + if (!has_ipoib(device)) > return; This is a good example of a test that doesn't really make sense, IPoIB is certainly a port specific attribute. > dev_list = kmalloc(sizeof *dev_list, GFP_KERNEL); > @@ -1673,7 +1673,7 @@ static void ipoib_add_one(struct ib_device *device) > } > > for (p = s; p <= e; ++p) { > - if (!rdma_port_ll_is_ib(device, p)) > + if (!cap_ipoib(device, p)) > continue; And down here we test every port. The routine should just test every port and do nothing if no ports need IPoIB. > dev = ipoib_add_port("ib%d", device, p); > if (!IS_ERR(dev)) { > @@ -1690,7 +1690,7 @@ static void ipoib_remove_one(struct ib_device *device) > struct ipoib_dev_priv *priv, *tmp; > struct list_head *dev_list; > > - if (!rdma_transport_is_ib(device)) > + if (!has_ipoib(device)) > return; This test should be made redundant by having ib_get_client_data return null if ipoib_add_one didn't do anything. Maybe that already happens? Jason -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 03/27/2015 05:06 PM, Jason Gunthorpe wrote: > On Fri, Mar 27, 2015 at 04:48:22PM +0100, Michael Wang wrote: > >> diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c >> index 3341754..fcd7558 100644 >> +++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c >> @@ -1655,7 +1655,7 @@ static void ipoib_add_one(struct ib_device *device) >> struct ipoib_dev_priv *priv; >> int s, e, p; >> >> - if (!rdma_transport_is_ib(device)) >> + if (!has_ipoib(device)) >> return; > This is a good example of a test that doesn't really make sense, IPoIB > is certainly a port specific attribute. According to my understanding, seems like the logical is: if 'device have no port need ipoib initialization' return > >> dev_list = kmalloc(sizeof *dev_list, GFP_KERNEL); >> @@ -1673,7 +1673,7 @@ static void ipoib_add_one(struct ib_device *device) >> } >> >> for (p = s; p <= e; ++p) { >> - if (!rdma_port_ll_is_ib(device, p)) >> + if (!cap_ipoib(device, p)) >> continue; > And down here we test every port. > > The routine should just test every port and do nothing if no ports > need IPoIB. And here since there are some initial work to do, it iterate all the port to find out which one need to be initialized. > >> dev = ipoib_add_port("ib%d", device, p); >> if (!IS_ERR(dev)) { >> @@ -1690,7 +1690,7 @@ static void ipoib_remove_one(struct ib_device *device) >> struct ipoib_dev_priv *priv, *tmp; >> struct list_head *dev_list; >> >> - if (!rdma_transport_is_ib(device)) >> + if (!has_ipoib(device)) >> return; > This test should be made redundant by having ib_get_client_data return > null if ipoib_add_one didn't do anything. Maybe that already happens? Here if we have done nothing when add a device, then nothing need to be cleanup when remove it. I'm not sure if it's a good idea, but seems like the idea is use twice check on different level to save some cycles? Regards, Michael Wang > > Jason -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Mar 27, 2015 at 05:15:42PM +0100, Michael Wang wrote: > I'm not sure if it's a good idea, but seems like the idea is use twice > check on different level to save some cycles? That is what it looks like to me, right now. Which is why I'd drop it. This isn't performance sensitive, make it simple. Jason -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c index 3341754..fcd7558 100644 --- a/drivers/infiniband/ulp/ipoib/ipoib_main.c +++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c @@ -1655,7 +1655,7 @@ static void ipoib_add_one(struct ib_device *device) struct ipoib_dev_priv *priv; int s, e, p; - if (!rdma_transport_is_ib(device)) + if (!has_ipoib(device)) return; dev_list = kmalloc(sizeof *dev_list, GFP_KERNEL); @@ -1673,7 +1673,7 @@ static void ipoib_add_one(struct ib_device *device) } for (p = s; p <= e; ++p) { - if (!rdma_port_ll_is_ib(device, p)) + if (!cap_ipoib(device, p)) continue; dev = ipoib_add_port("ib%d", device, p); if (!IS_ERR(dev)) { @@ -1690,7 +1690,7 @@ static void ipoib_remove_one(struct ib_device *device) struct ipoib_dev_priv *priv, *tmp; struct list_head *dev_list; - if (!rdma_transport_is_ib(device)) + if (!has_ipoib(device)) return; dev_list = ib_get_client_data(device, &ipoib_client); diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h index 0ef9cd7..eead588 100644 --- a/include/rdma/ib_verbs.h +++ b/include/rdma/ib_verbs.h @@ -1849,6 +1849,19 @@ static inline int has_iwarp(struct ib_device *device) } /** + * has_ipoib - Check if a device support IP over Infiniband. + * + * @device: Device to be checked + * + * Return 0 when a device has none port to support + * IP over Infiniband. + */ +static inline int has_ipoib(struct ib_device *device) +{ + return rdma_transport_is_ib(device); +} + +/** * cap_smi - Check if the port of device has the capability * Subnet Management Interface. * @@ -1893,6 +1906,21 @@ static inline int cap_mcast(struct ib_device *device, u8 port_num) return rdma_port_ll_is_ib(device, port_num); } +/** + * cap_ipoib - Check if the port of device has the capability + * IP over Infiniband. + * + * @device: Device to be checked + * @port_num: Port number of the device + * + * Return 0 when port of the device don't support + * IP over Infiniband. + */ +static inline int cap_ipoib(struct ib_device *device, u8 port_num) +{ + return rdma_port_ll_is_ib(device, port_num); +} + int ib_query_gid(struct ib_device *device, u8 port_num, int index, union ib_gid *gid);
Introduce helper has_ipoib() and cap_ipoib() to help us check if an IB device or it's port support IP over Infiniband. Cc: Jason Gunthorpe <jgunthorpe@obsidianresearch.com> Cc: Doug Ledford <dledford@redhat.com> Cc: Ira Weiny <ira.weiny@intel.com> Cc: Sean Hefty <sean.hefty@intel.com> Signed-off-by: Michael Wang <yun.wang@profitbricks.com> --- drivers/infiniband/ulp/ipoib/ipoib_main.c | 6 +++--- include/rdma/ib_verbs.h | 28 ++++++++++++++++++++++++++++ 2 files changed, 31 insertions(+), 3 deletions(-)