[ofa-general] Re: [IPoIB][RFC] remove redundant gid query

[James Lentini]

On Mon, 21 May 2007, Roland Dreier wrote:

>  > > It does look like we're doing some work we don't need to do.  However
>  > > ipoib_add_port() could run before an SM has brought up the local port,
>  > 
>  > The same could be true for ipoib_mcast_join_task()
>  > 
>  > These are both instances of the general problem that if the GID at 
>  > index 0 changes, the IPoIB code is not automatically notified. Agree?
> 
> Yes, although what is there now should be semi-OK: a multicast join
> can't succeed until the port is up, so ipoib should eventually get the
> right GID.  And I would argue that an SM that changes a port's GID
> prefix without at least generating a client reregister event is broken.

Expecting the SM to request a client reregister is reasonable.

>From IPoIB down, everything seems OK. 

I'm wondering about the layers above IPoIB.

When ipoib_add_port() calls register_netdev(), there is at least one 
place where the networking stack examines the dev_addr value (see 
rtnl_fill_ifinfo(), a netlink message is created with the device and 
broadcast hardware addresses).

If the GID changes, the IPoIB net_device's dev_addr changes. IPoIB 
doesn't inform the upper layers when this happens.

>  > > so the GID prefix might change later.
>  > > 
>  > > I'm not sure what the best way to clean this up is.
>  > 
>  > As an aside: Why does ipoib_add_port() treat an error return from 
>  > ib_query_gid() as fatal while ipoib_mcast_join_task() only emits a 
>  > warning?
> 
> I guess because it's much easier to bail out of ipoib_add_port() than
> it is to do something intelligent in ipoib_mcast_join_task().

Would adding a warning if the GID changes be of use?

Signed-off-by: James Lentini [jlentini at netapp.com]

--- a/drivers/infiniband/ulp/ipoib/ipoib.h	2007-04-25 23:08:32.000000000 -0400
+++ b/drivers/infiniband/ulp/ipoib/ipoib.h	2007-05-22 11:17:52.000000000 -0400
@@ -563,11 +563,18 @@
 		if (mcast_debug_level > 0)		\
 			ipoib_printk(KERN_DEBUG, priv, format , ## arg); \
 	} while (0)
+
+#define ipoib_dbg_chkgid(priv, a, b) 				\
+	do {							\
+		if (memcmp((a), (b), sizeof (union ib_gid)))	\
+			ipoib_warn(priv, "gid changed\n");	\
+	} while (0)
 #else /* CONFIG_INFINIBAND_IPOIB_DEBUG */
 #define ipoib_dbg(priv, format, arg...)			\
 	do { (void) (priv); } while (0)
 #define ipoib_dbg_mcast(priv, format, arg...)		\
 	do { (void) (priv); } while (0)
+#define ipoib_dbg_chkgid(priv, a, b)
 #endif /* CONFIG_INFINIBAND_IPOIB_DEBUG */
 
 #ifdef CONFIG_INFINIBAND_IPOIB_DEBUG_DATA
--- a/drivers/infiniband/ulp/ipoib/ipoib_multicast.c	2007-05-22 11:07:30.000000000 -0400
+++ b/drivers/infiniband/ulp/ipoib/ipoib_multicast.c	2007-05-22 11:17:22.000000000 -0400
@@ -525,8 +525,10 @@
 
 	if (ib_query_gid(priv->ca, priv->port, 0, &priv->local_gid))
 		ipoib_warn(priv, "ib_gid_entry_get() failed\n");
-	else
+	else {
+		ipoib_dbg_chkgid(priv, priv->dev->dev_addr + 4, priv->local_gid.raw);
 		memcpy(priv->dev->dev_addr + 4, priv->local_gid.raw, sizeof (union ib_gid));
+	}
 
 	{
 		struct ib_port_attr attr;