| Message ID | 552BB552.1030905@profitbricks.com (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
On Mon, Apr 13, 2015 at 02:23:46PM +0200, Michael Wang wrote: > > Use raw management helpers to reform IB-core cm. > > Cc: Steve Wise <swise@opengridcomputing.com> > Cc: Tom Talpey <tom@talpey.com> > 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/core/cm.c | 22 +++++++++++++++++++--- > 1 file changed, 19 insertions(+), 3 deletions(-) > > diff --git a/drivers/infiniband/core/cm.c b/drivers/infiniband/core/cm.c > index e28a494..50321fe 100644 > --- a/drivers/infiniband/core/cm.c > +++ b/drivers/infiniband/core/cm.c > @@ -3761,9 +3761,7 @@ static void cm_add_one(struct ib_device *ib_device) > unsigned long flags; > int ret; > u8 i; > - > - if (rdma_node_get_transport(ib_device->node_type) != RDMA_TRANSPORT_IB) > - return; > + int count = 0; I'm ok with this as an intermediate patch but going forward if we are going to have calls like static inline int cap_ib_cm_dev(struct ib_device *device) Then I think we should have similar calls like cap_ib_mad_dev(device) Which eliminates the clean up below... > > cm_dev = kzalloc(sizeof(*cm_dev) + sizeof(*port) * > ib_device->phys_port_cnt, GFP_KERNEL); > @@ -3783,6 +3781,9 @@ static void cm_add_one(struct ib_device *ib_device) > > set_bit(IB_MGMT_METHOD_SEND, reg_req.method_mask); > for (i = 1; i <= ib_device->phys_port_cnt; i++) { > + if (!rdma_ib_or_iboe(ib_device, i)) > + continue; > + > port = kzalloc(sizeof *port, GFP_KERNEL); > if (!port) > goto error1; > @@ -3809,7 +3810,16 @@ static void cm_add_one(struct ib_device *ib_device) > ret = ib_modify_port(ib_device, i, 0, &port_modify); > if (ret) > goto error3; > + > + count++; > } > + > + if (!count) { > + device_unregister(cm_dev->device); > + kfree(cm_dev); > + return; Here. I worry about mistakes being made when we loop through only to find that none of the ports support the feature and then we have to clean up. As this is initialization code I don't see any issue with looping through the ports 2 times and making the code cleaner. This applies to the SA and CM modules as well. However, in the ib_cm module you already have cap_ib_cm_dev(device) so you should use it at the start of cm_add_one. Ira -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > - if (rdma_node_get_transport(ib_device->node_type) != > RDMA_TRANSPORT_IB) > > - return; > > + int count = 0; > > I'm ok with this as an intermediate patch but going forward if we are > going to > have calls like > > static inline int cap_ib_cm_dev(struct ib_device *device) I would rather keep everything to checking per port, not per device. Specifically, because we have code that does this: > > port = kzalloc(sizeof *port, GFP_KERNEL); > > if (!port) > > goto error1; > > @@ -3809,7 +3810,16 @@ static void cm_add_one(struct ib_device > *ib_device) > > ret = ib_modify_port(ib_device, i, 0, &port_modify); > > if (ret) > > goto error3; It will also help keep the checks consistent, so that we don't end up with CM checks being per device, but SA checks being per port. - Sean -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Apr 13, 2015 at 06:40:35PM +0000, Hefty, Sean wrote: > > > - if (rdma_node_get_transport(ib_device->node_type) != > > RDMA_TRANSPORT_IB) > > > - return; > > > + int count = 0; > > > > I'm ok with this as an intermediate patch but going forward if we are > > going to > > have calls like > > > > static inline int cap_ib_cm_dev(struct ib_device *device) > > I would rather keep everything to checking per port, not per device. > Specifically, because we have code that does this: Argee. I asked Michael for it and stand by it, the property is per-port, not per device. Having the per-device tests just muddles the logic, look at the trouble Sean notices in #10 when we are now forced to think of things clearly. Jason -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Apr 13, 2015 at 01:29:30PM -0600, Jason Gunthorpe wrote: > On Mon, Apr 13, 2015 at 06:40:35PM +0000, Hefty, Sean wrote: > > > > - if (rdma_node_get_transport(ib_device->node_type) != > > > RDMA_TRANSPORT_IB) > > > > - return; > > > > + int count = 0; > > > > > > I'm ok with this as an intermediate patch but going forward if we are > > > going to > > > have calls like > > > > > > static inline int cap_ib_cm_dev(struct ib_device *device) > > > > I would rather keep everything to checking per port, not per device. > > Specifically, because we have code that does this: > > Argee. > > I asked Michael for it and stand by it, the property is per-port, not > per device. Having the per-device tests just muddles the logic, look > at the trouble Sean notices in #10 when we are now forced to think of > things clearly. What about having those be helpers within the corresponding C code? For example move cap_ib_cm_dev() into cm.c. Or just put the logic at the top of cm_add_one()? I think that having the ib_umad, ib_sa, and ib_cm modules skip devices which have no ports which support those functions makes the code clean. But I understand the desire to have checks from the devices be per port. Also for these function clean up patches we preserve the existing logic. Ira -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04/13/2015 08:12 PM, ira.weiny wrote: [snip] >> - >> - if (rdma_node_get_transport(ib_device->node_type) != RDMA_TRANSPORT_IB) >> - return; >> + int count = 0; > > I'm ok with this as an intermediate patch but going forward if we are going to > have calls like > > static inline int cap_ib_cm_dev(struct ib_device *device) Actually I really don't want to introduce this kind of helper, it's slow, ugly and break the consistency, but I can't find a good way to avoid that... For example the check inside cma_listen_on_dev(), how could we do per-port check while don't even know which port will be used later... > > Then I think we should have similar calls like > > cap_ib_mad_dev(device) > > Which eliminates the clean up below... I'd like to avoid using such helper as long as possible :-P > >> >> cm_dev = kzalloc(sizeof(*cm_dev) + sizeof(*port) * >> ib_device->phys_port_cnt, GFP_KERNEL); >> @@ -3783,6 +3781,9 @@ static void cm_add_one(struct ib_device *ib_device) >> >> set_bit(IB_MGMT_METHOD_SEND, reg_req.method_mask); >> for (i = 1; i <= ib_device->phys_port_cnt; i++) { >> + if (!rdma_ib_or_iboe(ib_device, i)) >> + continue; >> + >> port = kzalloc(sizeof *port, GFP_KERNEL); >> if (!port) >> goto error1; >> @@ -3809,7 +3810,16 @@ static void cm_add_one(struct ib_device *ib_device) >> ret = ib_modify_port(ib_device, i, 0, &port_modify); >> if (ret) >> goto error3; >> + >> + count++; >> } >> + >> + if (!count) { >> + device_unregister(cm_dev->device); >> + kfree(cm_dev); >> + return; > > Here. > > I worry about mistakes being made when we loop through only to find that none > of the ports support the feature and then we have to clean up. As this is > initialization code I don't see any issue with looping through the ports 2 > times and making the code cleaner. This style of logical could be found in other core module too, may be keep consistent is not a bad idea? After all, it's just initialization code which relatively rarely used :-) Regards, Michael Wang > > This applies to the SA and CM modules as well. > > However, in the ib_cm module you already have cap_ib_cm_dev(device) so you > should use it at the start of cm_add_one. > > Ira > -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04/13/2015 09:29 PM, Jason Gunthorpe wrote: > On Mon, Apr 13, 2015 at 06:40:35PM +0000, Hefty, Sean wrote: >>>> - if (rdma_node_get_transport(ib_device->node_type) != >>> RDMA_TRANSPORT_IB) >>>> - return; >>>> + int count = 0; >>> >>> I'm ok with this as an intermediate patch but going forward if we are >>> going to >>> have calls like >>> >>> static inline int cap_ib_cm_dev(struct ib_device *device) >> >> I would rather keep everything to checking per port, not per device. >> Specifically, because we have code that does this: > > Argee. > > I asked Michael for it and stand by it, the property is per-port, not > per device. Having the per-device tests just muddles the logic, look > at the trouble Sean notices in #10 when we are now forced to think of > things clearly. The only per-dev checking left is all included in #24 (now may be #10 too), which is inside: 1. cma_listen_on_dev 2. ib_ucm_add_one I can't find a good way to apply per-port check in this two, seems like they are at the stage which not related to port yet... any ideas on how to improve that? Regards, Michael Wang > > Jason > -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" 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/core/cm.c b/drivers/infiniband/core/cm.c index e28a494..50321fe 100644 --- a/drivers/infiniband/core/cm.c +++ b/drivers/infiniband/core/cm.c @@ -3761,9 +3761,7 @@ static void cm_add_one(struct ib_device *ib_device) unsigned long flags; int ret; u8 i; - - if (rdma_node_get_transport(ib_device->node_type) != RDMA_TRANSPORT_IB) - return; + int count = 0; cm_dev = kzalloc(sizeof(*cm_dev) + sizeof(*port) * ib_device->phys_port_cnt, GFP_KERNEL); @@ -3783,6 +3781,9 @@ static void cm_add_one(struct ib_device *ib_device) set_bit(IB_MGMT_METHOD_SEND, reg_req.method_mask); for (i = 1; i <= ib_device->phys_port_cnt; i++) { + if (!rdma_ib_or_iboe(ib_device, i)) + continue; + port = kzalloc(sizeof *port, GFP_KERNEL); if (!port) goto error1; @@ -3809,7 +3810,16 @@ static void cm_add_one(struct ib_device *ib_device) ret = ib_modify_port(ib_device, i, 0, &port_modify); if (ret) goto error3; + + count++; } + + if (!count) { + device_unregister(cm_dev->device); + kfree(cm_dev); + return; + } + ib_set_client_data(ib_device, &cm_client, cm_dev); write_lock_irqsave(&cm.device_lock, flags); @@ -3825,6 +3835,9 @@ error1: port_modify.set_port_cap_mask = 0; port_modify.clr_port_cap_mask = IB_PORT_CM_SUP; while (--i) { + if (!rdma_ib_or_iboe(ib_device, i)) + continue; + port = cm_dev->port[i-1]; ib_modify_port(ib_device, port->port_num, 0, &port_modify); ib_unregister_mad_agent(port->mad_agent); @@ -3853,6 +3866,9 @@ static void cm_remove_one(struct ib_device *ib_device) write_unlock_irqrestore(&cm.device_lock, flags); for (i = 1; i <= ib_device->phys_port_cnt; i++) { + if (!rdma_ib_or_iboe(ib_device, i)) + continue; + port = cm_dev->port[i-1]; ib_modify_port(ib_device, port->port_num, 0, &port_modify); ib_unregister_mad_agent(port->mad_agent);
Use raw management helpers to reform IB-core cm. Cc: Steve Wise <swise@opengridcomputing.com> Cc: Tom Talpey <tom@talpey.com> 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/core/cm.c | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-)