Re: [RFC PATCH 07/11] IB/Verbs: Use management helper has_mcast() and, cap_mcast() for mcast-check

["ira.weiny" <ira.weiny@intel.com>]

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