| Message ID | 55157B71.6040505@profitbricks.com |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
On Fri, Mar 27, 2015 at 04:46:57PM +0100, Michael Wang wrote: > > Introduce helper has_mcast() and cap_mcast() to help us check if an > IB device or it's port support Multicast. > > 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/cma.c | 2 +- > drivers/infiniband/core/multicast.c | 8 ++++---- > include/rdma/ib_verbs.h | 28 ++++++++++++++++++++++++++++ > 3 files changed, 33 insertions(+), 5 deletions(-) > > diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c > index 276fb76..cbbc85b 100644 > +++ b/drivers/infiniband/core/cma.c > @@ -3398,7 +3398,7 @@ void rdma_leave_multicast(struct rdma_cm_id *id, struct sockaddr *addr) > ib_detach_mcast(id->qp, > &mc->multicast.ib->rec.mgid, > be16_to_cpu(mc->multicast.ib->rec.mlid)); > - if (rdma_transport_is_ib(id_priv->cma_dev->device)) { > + if (has_mcast(id_priv->cma_dev->device)) { This might make more sense as cap_ib_multicast / cap_ip_multicast > switch (rdma_port_get_link_layer(id->device, id->port_num)) { > case IB_LINK_LAYER_INFINIBAND: > ib_sa_free_multicast(mc->multicast.ib); > diff --git a/drivers/infiniband/core/multicast.c b/drivers/infiniband/core/multicast.c > index 17573ff..ffeaf27 100644 > +++ b/drivers/infiniband/core/multicast.c > @@ -780,7 +780,7 @@ static void mcast_event_handler(struct ib_event_handler *handler, > int index; > > dev = container_of(handler, struct mcast_device, event_handler); > - if (!rdma_port_ll_is_ib(dev->device, event->element.port_num)) > + if (!cap_mcast(dev->device, event->element.port_num)) > return; These should probably be cap_ib_sa - that is what they are guarding against. But it seems redudent, since mcast_add_one will already not add a port that is not IB, so mcast_event_handler is not callable. Something to do with rocee/ib switching? > index = event->element.port_num - dev->start_port; > @@ -807,7 +807,7 @@ static void mcast_add_one(struct ib_device *device) > int i; > int count = 0; > > - if (!rdma_transport_is_ib(device)) > + if (!has_mcast(device)) > return; Again, this seems redundant, every port is tested directly below, why is this check needed? Looking at this, I do wonder how a port can dynamically change between rocee and IB.. If the link value changes then mcast_remove_one will not be a perfect reversal of mcast_add_one. Bug? It feels necessary to understand what happens when a port dynamically switches to ethernet on mlx hardware to validate these patches :( Jason -- To unsubscribe from this list: send the line "unsubscribe netdev" 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 10:28:20AM -0600, Jason Gunthorpe wrote: > On Fri, Mar 27, 2015 at 04:46:57PM +0100, Michael Wang wrote: > > > > Introduce helper has_mcast() and cap_mcast() to help us check if an > > IB device or it's port support Multicast. > > > > 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/cma.c | 2 +- > > drivers/infiniband/core/multicast.c | 8 ++++---- > > include/rdma/ib_verbs.h | 28 ++++++++++++++++++++++++++++ > > 3 files changed, 33 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c > > index 276fb76..cbbc85b 100644 > > +++ b/drivers/infiniband/core/cma.c > > @@ -3398,7 +3398,7 @@ void rdma_leave_multicast(struct rdma_cm_id *id, struct sockaddr *addr) > > ib_detach_mcast(id->qp, > > &mc->multicast.ib->rec.mgid, > > be16_to_cpu(mc->multicast.ib->rec.mlid)); > > - if (rdma_transport_is_ib(id_priv->cma_dev->device)) { > > + if (has_mcast(id_priv->cma_dev->device)) { > > This might make more sense as cap_ib_multicast / cap_ip_multicast Agreed. > > > switch (rdma_port_get_link_layer(id->device, id->port_num)) { > > case IB_LINK_LAYER_INFINIBAND: > > ib_sa_free_multicast(mc->multicast.ib); > > > diff --git a/drivers/infiniband/core/multicast.c b/drivers/infiniband/core/multicast.c > > index 17573ff..ffeaf27 100644 > > +++ b/drivers/infiniband/core/multicast.c > > @@ -780,7 +780,7 @@ static void mcast_event_handler(struct ib_event_handler *handler, > > int index; > > > > dev = container_of(handler, struct mcast_device, event_handler); > > - if (!rdma_port_ll_is_ib(dev->device, event->element.port_num)) > > + if (!cap_mcast(dev->device, event->element.port_num)) > > return; > > These should probably be cap_ib_sa - that is what they are guarding > against. > > But it seems redudent, since mcast_add_one will already not add a port that is > not IB, so mcast_event_handler is not callable. Something to do with > rocee/ib switching? I'm not sure about this either. This check seems to be necessary only on a per-port level. It does seem apparent that one can't go from Eth to IB. What happens if you go from IB to Eth on the port? > > > index = event->element.port_num - dev->start_port; > > @@ -807,7 +807,7 @@ static void mcast_add_one(struct ib_device *device) > > int i; > > int count = 0; > > > > - if (!rdma_transport_is_ib(device)) > > + if (!has_mcast(device)) > > return; > > Again, this seems redundant, every port is tested directly below, why > is this check needed? Agreed. Same as my comments about the SA support. This is really only needed on ports which need to register with the SA (or perhaps some future entity) for Mcast support. Also this is part of the ib_sa module and exports the function ib_sa_join_multicast. So that this point it is covered under the cap_sa(device, port) call. So the implementation of cap_mcast at this point is: cap_mcast(device, port) { return cap_sa(device,port); } > > Looking at this, I do wonder how a port can dynamically change between > rocee and IB.. If the link value changes then mcast_remove_one will > not be a perfect reversal of mcast_add_one. Bug? > > It feels necessary to understand what happens when a port dynamically > switches to ethernet on mlx hardware to validate these patches :( Agreed. :-( -- Ira > > Jason -- To unsubscribe from this list: send the line "unsubscribe netdev" 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 6:05 PM, ira.weiny <ira.weiny@intel.com> wrote: > On Fri, Mar 27, 2015 at 10:28:20AM -0600, Jason Gunthorpe wrote: >> On Fri, Mar 27, 2015 at 04:46:57PM +0100, Michael Wang wrote: >> > [snip] >> > - if (rdma_transport_is_ib(id_priv->cma_dev->device)) { >> > + if (has_mcast(id_priv->cma_dev->device)) { >> >> This might make more sense as cap_ib_multicast / cap_ip_multicast > > Agreed. > Will be changed in next version :-) >> >> > switch (rdma_port_get_link_layer(id->device, id->port_num)) { >> > case IB_LINK_LAYER_INFINIBAND: >> > ib_sa_free_multicast(mc->multicast.ib); >> >> > diff --git a/drivers/infiniband/core/multicast.c b/drivers/infiniband/core/multicast.c >> > index 17573ff..ffeaf27 100644 >> > +++ b/drivers/infiniband/core/multicast.c >> > @@ -780,7 +780,7 @@ static void mcast_event_handler(struct ib_event_handler *handler, >> > int index; >> > >> > dev = container_of(handler, struct mcast_device, event_handler); >> > - if (!rdma_port_ll_is_ib(dev->device, event->element.port_num)) >> > + if (!cap_mcast(dev->device, event->element.port_num)) >> > return; >> >> These should probably be cap_ib_sa - that is what they are guarding >> against. >> >> But it seems redudent, since mcast_add_one will already not add a port that is >> not IB, so mcast_event_handler is not callable. Something to do with >> rocee/ib switching? > > I'm not sure about this either. This check seems to be necessary only on a > per-port level. It does seem apparent that one can't go from Eth to IB. What > happens if you go from IB to Eth on the port? I also feel it's redundant at first glance, but just not sure if it could be removed, lack of some knowledge :-P > >> >> > index = event->element.port_num - dev->start_port; >> > @@ -807,7 +807,7 @@ static void mcast_add_one(struct ib_device *device) >> > int i; >> > int count = 0; >> > >> > - if (!rdma_transport_is_ib(device)) >> > + if (!has_mcast(device)) >> > return; >> >> Again, this seems redundant, every port is tested directly below, why >> is this check needed? > > Agreed. Same as my comments about the SA support. This is really only > needed on ports which need to register with the SA (or perhaps some future > entity) for Mcast support. I will recheck all the logical around has_XX() see if we can get rid of them ;-) > > Also this is part of the ib_sa module and exports the function > ib_sa_join_multicast. So that this point it is covered under the > cap_sa(device, port) call. > > So the implementation of cap_mcast at this point is: > > cap_mcast(device, port) > { > return cap_sa(device,port); > } > Sounds good :-) will be in next version. >> >> Looking at this, I do wonder how a port can dynamically change between >> rocee and IB.. If the link value changes then mcast_remove_one will >> not be a perfect reversal of mcast_add_one. Bug? >> >> It feels necessary to understand what happens when a port dynamically >> switches to ethernet on mlx hardware to validate these patches :( > > Agreed. Maybe we can temporarily reserve the old logical, and gradually solve these problems? Regards, Michael Wang > > :-( > > -- Ira > >> >> Jason -- To unsubscribe from this list: send the line "unsubscribe netdev" 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 01:05:08PM -0400, ira.weiny wrote: > > But it seems redudent, since mcast_add_one will already not add a port that is > > not IB, so mcast_event_handler is not callable. Something to do with > > rocee/ib switching? > > I'm not sure about this either. This check seems to be necessary only on a > per-port level. It does seem apparent that one can't go from Eth to IB. What > happens if you go from IB to Eth on the port? Hmm... I see a mlx4_change_port_types which ultimately calls ib_unregister_device, which suggests the port type doesn't change at runtime (yay) So maybe these checks really are redundant? Jason -- To unsubscribe from this list: send the line "unsubscribe netdev" 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 06:31:26PM +0100, Yun Wang wrote: > Maybe we can temporarily reserve the old logical, and gradually solve > these problems? It is best to make behavioral changes in small patches, yes. I think it is best to address these sorts of problems before trying to tackle the driver interface side - that will avoid complexity. ie how does a driver set has_ipoib? It isn't even a sensible question when it is a per port capability. Jason -- To unsubscribe from this list: send the line "unsubscribe netdev" 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 6:49 PM, Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote: > On Fri, Mar 27, 2015 at 06:31:26PM +0100, Yun Wang wrote: > >> Maybe we can temporarily reserve the old logical, and gradually solve >> these problems? > > It is best to make behavioral changes in small patches, yes. > > I think it is best to address these sorts of problems before trying to > tackle the driver interface side - that will avoid complexity. > > ie how does a driver set has_ipoib? It isn't even a sensible question > when it is a per port capability. I used to imaging it would like: init_device_mgmt_attributs(device) { for_each_port(device) if (port support XX) { port.mgmt_attribute |= CAP_XX if !(device.mgmt_attribute & HAS_XX) device.mgmt_attribute |= HAS_XX } } That is incase if a device got one port have some capability, the device have it too, so has_XX() will check on device level that if it has any port support XX, and cap_XX() to check on port level. But if has_XX() is only for optimizing the init/exit path, then it doesn't make sense to me any more... Regards, Michael Wang > > Jason -- To unsubscribe from this list: send the line "unsubscribe netdev" 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 06:47 PM, Jason Gunthorpe wrote: > On Fri, Mar 27, 2015 at 01:05:08PM -0400, ira.weiny wrote: > >>> But it seems redudent, since mcast_add_one will already not add a port that is >>> not IB, so mcast_event_handler is not callable. Something to do with >>> rocee/ib switching? >> I'm not sure about this either. This check seems to be necessary only on a >> per-port level. It does seem apparent that one can't go from Eth to IB. What >> happens if you go from IB to Eth on the port? > Hmm... I see a mlx4_change_port_types which ultimately calls > ib_unregister_device, which suggests the port type doesn't change at > runtime (yay) Yeah, seems like mlx4 will reinitialize the device when port link layer changed. I've take a look at other HW, they directly return a static type or infer from transport type (I suppose this won't change dynamically). Thus I also agreed check inside mcast_event_handler() is unnecessary, maybe we can change that logical to WARN_ON(!cap_mcast()) ? Regards, Michael Wang > > So maybe these checks really are redundant? > > Jason -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 2015-03-27 at 16:46 +0100, Michael Wang wrote: > Introduce helper has_mcast() and cap_mcast() to help us check if an > IB device or it's port support Multicast. This probably needs reworded or rethought. In truth, *all* rdma devices are multicast capable. *BUT*, IB/OPA devices require multicast registration done the IB way (including for sendonly multicast sends), while Ethernet devices do multicast the Ethernet way. These tests are really just for IB specific multicast registration and deregistration. Call it has_mcast() and cap_mcast() is incorrect. > 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/cma.c | 2 +- > drivers/infiniband/core/multicast.c | 8 ++++---- > include/rdma/ib_verbs.h | 28 ++++++++++++++++++++++++++++ > 3 files changed, 33 insertions(+), 5 deletions(-) > > diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c > index 276fb76..cbbc85b 100644 > --- a/drivers/infiniband/core/cma.c > +++ b/drivers/infiniband/core/cma.c > @@ -3398,7 +3398,7 @@ void rdma_leave_multicast(struct rdma_cm_id *id, struct sockaddr *addr) > ib_detach_mcast(id->qp, > &mc->multicast.ib->rec.mgid, > be16_to_cpu(mc->multicast.ib->rec.mlid)); > - if (rdma_transport_is_ib(id_priv->cma_dev->device)) { > + if (has_mcast(id_priv->cma_dev->device)) { > switch (rdma_port_get_link_layer(id->device, id->port_num)) { > case IB_LINK_LAYER_INFINIBAND: > ib_sa_free_multicast(mc->multicast.ib); > diff --git a/drivers/infiniband/core/multicast.c b/drivers/infiniband/core/multicast.c > index 17573ff..ffeaf27 100644 > --- a/drivers/infiniband/core/multicast.c > +++ b/drivers/infiniband/core/multicast.c > @@ -780,7 +780,7 @@ static void mcast_event_handler(struct ib_event_handler *handler, > int index; > > dev = container_of(handler, struct mcast_device, event_handler); > - if (!rdma_port_ll_is_ib(dev->device, event->element.port_num)) > + if (!cap_mcast(dev->device, event->element.port_num)) > return; > > index = event->element.port_num - dev->start_port; > @@ -807,7 +807,7 @@ static void mcast_add_one(struct ib_device *device) > int i; > int count = 0; > > - if (!rdma_transport_is_ib(device)) > + if (!has_mcast(device)) > return; > > dev = kmalloc(sizeof *dev + device->phys_port_cnt * sizeof *port, > @@ -823,7 +823,7 @@ static void mcast_add_one(struct ib_device *device) > } > > for (i = 0; i <= dev->end_port - dev->start_port; i++) { > - if (!rdma_port_ll_is_ib(device, dev->start_port + i)) > + if (!cap_mcast(device, dev->start_port + i)) > continue; > port = &dev->port[i]; > port->dev = dev; > @@ -861,7 +861,7 @@ static void mcast_remove_one(struct ib_device *device) > flush_workqueue(mcast_wq); > > for (i = 0; i <= dev->end_port - dev->start_port; i++) { > - if (rdma_port_ll_is_ib(device, dev->start_port + i)) { > + if (cap_mcast(device, dev->start_port + i)) { > port = &dev->port[i]; > deref_port(port); > wait_for_completion(&port->comp); > diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h > index fa8ffa3..e796104 100644 > --- a/include/rdma/ib_verbs.h > +++ b/include/rdma/ib_verbs.h > @@ -1823,6 +1823,19 @@ static inline int has_sa(struct ib_device *device) > } > > /** > + * has_mcast - Check if a device support Multicast. > + * > + * @device: Device to be checked > + * > + * Return 0 when a device has none port to support > + * Multicast. > + */ > +static inline int has_mcast(struct ib_device *device) > +{ > + return rdma_transport_is_ib(device); > +} > + > +/** > * cap_smi - Check if the port of device has the capability > * Subnet Management Interface. > * > @@ -1852,6 +1865,21 @@ static inline int cap_sa(struct ib_device *device, u8 port_num) > return rdma_port_ll_is_ib(device, port_num); > } > > +/** > + * cap_mcast - Check if the port of device has the capability > + * Multicast. > + * > + * @device: Device to be checked > + * @port_num: Port number of the device > + * > + * Return 0 when port of the device don't support > + * Multicast. > + */ > +static inline int cap_mcast(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); >
On 03/30/2015 06:11 PM, Doug Ledford wrote: > On Fri, 2015-03-27 at 16:46 +0100, Michael Wang wrote: >> Introduce helper has_mcast() and cap_mcast() to help us check if an >> IB device or it's port support Multicast. > This probably needs reworded or rethought. In truth, *all* rdma devices > are multicast capable. *BUT*, IB/OPA devices require multicast > registration done the IB way (including for sendonly multicast sends), > while Ethernet devices do multicast the Ethernet way. These tests are > really just for IB specific multicast registration and deregistration. > Call it has_mcast() and cap_mcast() is incorrect. Thanks for the explanation :-) Jason also mentioned we should use cap_ib_XX() instead, I'll use that name then we can distinguish the management between Eth and IB/OPA. Regards, Michael Wang > >> 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/cma.c | 2 +- >> drivers/infiniband/core/multicast.c | 8 ++++---- >> include/rdma/ib_verbs.h | 28 ++++++++++++++++++++++++++++ >> 3 files changed, 33 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c >> index 276fb76..cbbc85b 100644 >> --- a/drivers/infiniband/core/cma.c >> +++ b/drivers/infiniband/core/cma.c >> @@ -3398,7 +3398,7 @@ void rdma_leave_multicast(struct rdma_cm_id *id, struct sockaddr *addr) >> ib_detach_mcast(id->qp, >> &mc->multicast.ib->rec.mgid, >> be16_to_cpu(mc->multicast.ib->rec.mlid)); >> - if (rdma_transport_is_ib(id_priv->cma_dev->device)) { >> + if (has_mcast(id_priv->cma_dev->device)) { >> switch (rdma_port_get_link_layer(id->device, id->port_num)) { >> case IB_LINK_LAYER_INFINIBAND: >> ib_sa_free_multicast(mc->multicast.ib); >> diff --git a/drivers/infiniband/core/multicast.c b/drivers/infiniband/core/multicast.c >> index 17573ff..ffeaf27 100644 >> --- a/drivers/infiniband/core/multicast.c >> +++ b/drivers/infiniband/core/multicast.c >> @@ -780,7 +780,7 @@ static void mcast_event_handler(struct ib_event_handler *handler, >> int index; >> >> dev = container_of(handler, struct mcast_device, event_handler); >> - if (!rdma_port_ll_is_ib(dev->device, event->element.port_num)) >> + if (!cap_mcast(dev->device, event->element.port_num)) >> return; >> >> index = event->element.port_num - dev->start_port; >> @@ -807,7 +807,7 @@ static void mcast_add_one(struct ib_device *device) >> int i; >> int count = 0; >> >> - if (!rdma_transport_is_ib(device)) >> + if (!has_mcast(device)) >> return; >> >> dev = kmalloc(sizeof *dev + device->phys_port_cnt * sizeof *port, >> @@ -823,7 +823,7 @@ static void mcast_add_one(struct ib_device *device) >> } >> >> for (i = 0; i <= dev->end_port - dev->start_port; i++) { >> - if (!rdma_port_ll_is_ib(device, dev->start_port + i)) >> + if (!cap_mcast(device, dev->start_port + i)) >> continue; >> port = &dev->port[i]; >> port->dev = dev; >> @@ -861,7 +861,7 @@ static void mcast_remove_one(struct ib_device *device) >> flush_workqueue(mcast_wq); >> >> for (i = 0; i <= dev->end_port - dev->start_port; i++) { >> - if (rdma_port_ll_is_ib(device, dev->start_port + i)) { >> + if (cap_mcast(device, dev->start_port + i)) { >> port = &dev->port[i]; >> deref_port(port); >> wait_for_completion(&port->comp); >> diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h >> index fa8ffa3..e796104 100644 >> --- a/include/rdma/ib_verbs.h >> +++ b/include/rdma/ib_verbs.h >> @@ -1823,6 +1823,19 @@ static inline int has_sa(struct ib_device *device) >> } >> >> /** >> + * has_mcast - Check if a device support Multicast. >> + * >> + * @device: Device to be checked >> + * >> + * Return 0 when a device has none port to support >> + * Multicast. >> + */ >> +static inline int has_mcast(struct ib_device *device) >> +{ >> + return rdma_transport_is_ib(device); >> +} >> + >> +/** >> * cap_smi - Check if the port of device has the capability >> * Subnet Management Interface. >> * >> @@ -1852,6 +1865,21 @@ static inline int cap_sa(struct ib_device *device, u8 port_num) >> return rdma_port_ll_is_ib(device, port_num); >> } >> >> +/** >> + * cap_mcast - Check if the port of device has the capability >> + * Multicast. >> + * >> + * @device: Device to be checked >> + * @port_num: Port number of the device >> + * >> + * Return 0 when port of the device don't support >> + * Multicast. >> + */ >> +static inline int cap_mcast(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); >> > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Mar 30, 2015 at 10:30:36AM +0200, Michael Wang wrote: > Thus I also agreed check inside mcast_event_handler() is unnecessary, > maybe we can change that logical to WARN_ON(!cap_mcast()) ? Seems reasonable to me. Jason -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Mar 30, 2015 at 06:20:48PM +0200, Michael Wang wrote: > On 03/30/2015 06:11 PM, Doug Ledford wrote: > > On Fri, 2015-03-27 at 16:46 +0100, Michael Wang wrote: > >> Introduce helper has_mcast() and cap_mcast() to help us check if an > >> IB device or it's port support Multicast. > > This probably needs reworded or rethought. In truth, *all* rdma devices > > are multicast capable. *BUT*, IB/OPA devices require multicast > > registration done the IB way (including for sendonly multicast sends), > > while Ethernet devices do multicast the Ethernet way. These tests are > > really just for IB specific multicast registration and deregistration. > > Call it has_mcast() and cap_mcast() is incorrect. > > Thanks for the explanation :-) > > Jason also mentioned we should use cap_ib_XX() instead, I'll use > that name then we can distinguish the management between > Eth and IB/OPA. > > Regards, > Michael Wang > > > > > >> 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/cma.c | 2 +- > >> drivers/infiniband/core/multicast.c | 8 ++++---- > >> include/rdma/ib_verbs.h | 28 ++++++++++++++++++++++++++++ > >> 3 files changed, 33 insertions(+), 5 deletions(-) > >> > >> diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c > >> index 276fb76..cbbc85b 100644 > >> --- a/drivers/infiniband/core/cma.c > >> +++ b/drivers/infiniband/core/cma.c > >> @@ -3398,7 +3398,7 @@ void rdma_leave_multicast(struct rdma_cm_id *id, struct sockaddr *addr) > >> ib_detach_mcast(id->qp, > >> &mc->multicast.ib->rec.mgid, > >> be16_to_cpu(mc->multicast.ib->rec.mlid)); > >> - if (rdma_transport_is_ib(id_priv->cma_dev->device)) { > >> + if (has_mcast(id_priv->cma_dev->device)) { You need a similar check in rdma_join_multicast. Ira > >> switch (rdma_port_get_link_layer(id->device, id->port_num)) { > >> case IB_LINK_LAYER_INFINIBAND: > >> ib_sa_free_multicast(mc->multicast.ib); > >> diff --git a/drivers/infiniband/core/multicast.c b/drivers/infiniband/core/multicast.c > >> index 17573ff..ffeaf27 100644 > >> --- a/drivers/infiniband/core/multicast.c > >> +++ b/drivers/infiniband/core/multicast.c > >> @@ -780,7 +780,7 @@ static void mcast_event_handler(struct ib_event_handler *handler, > >> int index; > >> > >> dev = container_of(handler, struct mcast_device, event_handler); > >> - if (!rdma_port_ll_is_ib(dev->device, event->element.port_num)) > >> + if (!cap_mcast(dev->device, event->element.port_num)) > >> return; > >> > >> index = event->element.port_num - dev->start_port; > >> @@ -807,7 +807,7 @@ static void mcast_add_one(struct ib_device *device) > >> int i; > >> int count = 0; > >> > >> - if (!rdma_transport_is_ib(device)) > >> + if (!has_mcast(device)) > >> return; > >> > >> dev = kmalloc(sizeof *dev + device->phys_port_cnt * sizeof *port, > >> @@ -823,7 +823,7 @@ static void mcast_add_one(struct ib_device *device) > >> } > >> > >> for (i = 0; i <= dev->end_port - dev->start_port; i++) { > >> - if (!rdma_port_ll_is_ib(device, dev->start_port + i)) > >> + if (!cap_mcast(device, dev->start_port + i)) > >> continue; > >> port = &dev->port[i]; > >> port->dev = dev; > >> @@ -861,7 +861,7 @@ static void mcast_remove_one(struct ib_device *device) > >> flush_workqueue(mcast_wq); > >> > >> for (i = 0; i <= dev->end_port - dev->start_port; i++) { > >> - if (rdma_port_ll_is_ib(device, dev->start_port + i)) { > >> + if (cap_mcast(device, dev->start_port + i)) { > >> port = &dev->port[i]; > >> deref_port(port); > >> wait_for_completion(&port->comp); > >> diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h > >> index fa8ffa3..e796104 100644 > >> --- a/include/rdma/ib_verbs.h > >> +++ b/include/rdma/ib_verbs.h > >> @@ -1823,6 +1823,19 @@ static inline int has_sa(struct ib_device *device) > >> } > >> > >> /** > >> + * has_mcast - Check if a device support Multicast. > >> + * > >> + * @device: Device to be checked > >> + * > >> + * Return 0 when a device has none port to support > >> + * Multicast. > >> + */ > >> +static inline int has_mcast(struct ib_device *device) > >> +{ > >> + return rdma_transport_is_ib(device); > >> +} > >> + > >> +/** > >> * cap_smi - Check if the port of device has the capability > >> * Subnet Management Interface. > >> * > >> @@ -1852,6 +1865,21 @@ static inline int cap_sa(struct ib_device *device, u8 port_num) > >> return rdma_port_ll_is_ib(device, port_num); > >> } > >> > >> +/** > >> + * cap_mcast - Check if the port of device has the capability > >> + * Multicast. > >> + * > >> + * @device: Device to be checked > >> + * @port_num: Port number of the device > >> + * > >> + * Return 0 when port of the device don't support > >> + * Multicast. > >> + */ > >> +static inline int cap_mcast(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); > >> > > > > -- > 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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 03/31/2015 01:47 AM, ira.weiny wrote: > On Mon, Mar 30, 2015 at 06:20:48PM +0200, Michael Wang wrote: >> [snip] >>>> 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/cma.c | 2 +- >>>> drivers/infiniband/core/multicast.c | 8 ++++---- >>>> include/rdma/ib_verbs.h | 28 ++++++++++++++++++++++++++++ >>>> 3 files changed, 33 insertions(+), 5 deletions(-) >>>> >>>> diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c >>>> index 276fb76..cbbc85b 100644 >>>> --- a/drivers/infiniband/core/cma.c >>>> +++ b/drivers/infiniband/core/cma.c >>>> @@ -3398,7 +3398,7 @@ void rdma_leave_multicast(struct rdma_cm_id *id, struct sockaddr *addr) >>>> ib_detach_mcast(id->qp, >>>> &mc->multicast.ib->rec.mgid, >>>> be16_to_cpu(mc->multicast.ib->rec.mlid)); >>>> - if (rdma_transport_is_ib(id_priv->cma_dev->device)) { >>>> + if (has_mcast(id_priv->cma_dev->device)) { > > You need a similar check in rdma_join_multicast. Thanks for the remind, I'll give it a reform :-) Regards, Michael Wang > > Ira > > >>>> switch (rdma_port_get_link_layer(id->device, id->port_num)) { >>>> case IB_LINK_LAYER_INFINIBAND: >>>> ib_sa_free_multicast(mc->multicast.ib); >>>> diff --git a/drivers/infiniband/core/multicast.c b/drivers/infiniband/core/multicast.c >>>> index 17573ff..ffeaf27 100644 >>>> --- a/drivers/infiniband/core/multicast.c >>>> +++ b/drivers/infiniband/core/multicast.c >>>> @@ -780,7 +780,7 @@ static void mcast_event_handler(struct ib_event_handler *handler, >>>> int index; >>>> >>>> dev = container_of(handler, struct mcast_device, event_handler); >>>> - if (!rdma_port_ll_is_ib(dev->device, event->element.port_num)) >>>> + if (!cap_mcast(dev->device, event->element.port_num)) >>>> return; >>>> >>>> index = event->element.port_num - dev->start_port; >>>> @@ -807,7 +807,7 @@ static void mcast_add_one(struct ib_device *device) >>>> int i; >>>> int count = 0; >>>> >>>> - if (!rdma_transport_is_ib(device)) >>>> + if (!has_mcast(device)) >>>> return; >>>> >>>> dev = kmalloc(sizeof *dev + device->phys_port_cnt * sizeof *port, >>>> @@ -823,7 +823,7 @@ static void mcast_add_one(struct ib_device *device) >>>> } >>>> >>>> for (i = 0; i <= dev->end_port - dev->start_port; i++) { >>>> - if (!rdma_port_ll_is_ib(device, dev->start_port + i)) >>>> + if (!cap_mcast(device, dev->start_port + i)) >>>> continue; >>>> port = &dev->port[i]; >>>> port->dev = dev; >>>> @@ -861,7 +861,7 @@ static void mcast_remove_one(struct ib_device *device) >>>> flush_workqueue(mcast_wq); >>>> >>>> for (i = 0; i <= dev->end_port - dev->start_port; i++) { >>>> - if (rdma_port_ll_is_ib(device, dev->start_port + i)) { >>>> + if (cap_mcast(device, dev->start_port + i)) { >>>> port = &dev->port[i]; >>>> deref_port(port); >>>> wait_for_completion(&port->comp); >>>> diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h >>>> index fa8ffa3..e796104 100644 >>>> --- a/include/rdma/ib_verbs.h >>>> +++ b/include/rdma/ib_verbs.h >>>> @@ -1823,6 +1823,19 @@ static inline int has_sa(struct ib_device *device) >>>> } >>>> >>>> /** >>>> + * has_mcast - Check if a device support Multicast. >>>> + * >>>> + * @device: Device to be checked >>>> + * >>>> + * Return 0 when a device has none port to support >>>> + * Multicast. >>>> + */ >>>> +static inline int has_mcast(struct ib_device *device) >>>> +{ >>>> + return rdma_transport_is_ib(device); >>>> +} >>>> + >>>> +/** >>>> * cap_smi - Check if the port of device has the capability >>>> * Subnet Management Interface. >>>> * >>>> @@ -1852,6 +1865,21 @@ static inline int cap_sa(struct ib_device *device, u8 port_num) >>>> return rdma_port_ll_is_ib(device, port_num); >>>> } >>>> >>>> +/** >>>> + * cap_mcast - Check if the port of device has the capability >>>> + * Multicast. >>>> + * >>>> + * @device: Device to be checked >>>> + * @port_num: Port number of the device >>>> + * >>>> + * Return 0 when port of the device don't support >>>> + * Multicast. >>>> + */ >>>> +static inline int cap_mcast(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); >>>> >> -- >> 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 -- To unsubscribe from this list: send the line "unsubscribe netdev" 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/cma.c b/drivers/infiniband/core/cma.c index 276fb76..cbbc85b 100644 --- a/drivers/infiniband/core/cma.c +++ b/drivers/infiniband/core/cma.c @@ -3398,7 +3398,7 @@ void rdma_leave_multicast(struct rdma_cm_id *id, struct sockaddr *addr) ib_detach_mcast(id->qp, &mc->multicast.ib->rec.mgid, be16_to_cpu(mc->multicast.ib->rec.mlid)); - if (rdma_transport_is_ib(id_priv->cma_dev->device)) { + if (has_mcast(id_priv->cma_dev->device)) { switch (rdma_port_get_link_layer(id->device, id->port_num)) { case IB_LINK_LAYER_INFINIBAND: ib_sa_free_multicast(mc->multicast.ib); diff --git a/drivers/infiniband/core/multicast.c b/drivers/infiniband/core/multicast.c index 17573ff..ffeaf27 100644 --- a/drivers/infiniband/core/multicast.c +++ b/drivers/infiniband/core/multicast.c @@ -780,7 +780,7 @@ static void mcast_event_handler(struct ib_event_handler *handler, int index; dev = container_of(handler, struct mcast_device, event_handler); - if (!rdma_port_ll_is_ib(dev->device, event->element.port_num)) + if (!cap_mcast(dev->device, event->element.port_num)) return; index = event->element.port_num - dev->start_port; @@ -807,7 +807,7 @@ static void mcast_add_one(struct ib_device *device) int i; int count = 0; - if (!rdma_transport_is_ib(device)) + if (!has_mcast(device)) return; dev = kmalloc(sizeof *dev + device->phys_port_cnt * sizeof *port, @@ -823,7 +823,7 @@ static void mcast_add_one(struct ib_device *device) } for (i = 0; i <= dev->end_port - dev->start_port; i++) { - if (!rdma_port_ll_is_ib(device, dev->start_port + i)) + if (!cap_mcast(device, dev->start_port + i)) continue; port = &dev->port[i]; port->dev = dev; @@ -861,7 +861,7 @@ static void mcast_remove_one(struct ib_device *device) flush_workqueue(mcast_wq); for (i = 0; i <= dev->end_port - dev->start_port; i++) { - if (rdma_port_ll_is_ib(device, dev->start_port + i)) { + if (cap_mcast(device, dev->start_port + i)) { port = &dev->port[i]; deref_port(port); wait_for_completion(&port->comp); diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h index fa8ffa3..e796104 100644 --- a/include/rdma/ib_verbs.h +++ b/include/rdma/ib_verbs.h @@ -1823,6 +1823,19 @@ static inline int has_sa(struct ib_device *device) } /** + * has_mcast - Check if a device support Multicast. + * + * @device: Device to be checked + * + * Return 0 when a device has none port to support + * Multicast. + */ +static inline int has_mcast(struct ib_device *device) +{ + return rdma_transport_is_ib(device); +} + +/** * cap_smi - Check if the port of device has the capability * Subnet Management Interface. * @@ -1852,6 +1865,21 @@ static inline int cap_sa(struct ib_device *device, u8 port_num) return rdma_port_ll_is_ib(device, port_num); } +/** + * cap_mcast - Check if the port of device has the capability + * Multicast. + * + * @device: Device to be checked + * @port_num: Port number of the device + * + * Return 0 when port of the device don't support + * Multicast. + */ +static inline int cap_mcast(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_mcast() and cap_mcast() to help us check if an IB device or it's port support Multicast. 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/cma.c | 2 +- drivers/infiniband/core/multicast.c | 8 ++++---- include/rdma/ib_verbs.h | 28 ++++++++++++++++++++++++++++ 3 files changed, 33 insertions(+), 5 deletions(-)