[PATCH] pkey fix for ipoib - resubmission

[PATCH] pkey fix for ipoib - resubmission

[Mike Heinz]

I never got a response to this patch, so I'm sending it again.

-----------------

IPoIB is coded to use the 1st PKey in the PKey table as its ib0 interface. Additional ib0.pkey interfaces may be created using the /sys/class/... add_child interface.

However, there is a race.  During normal boot, IPoIB will be started before the port is Active.  Hence the pkey table has not yet been programmed and has a default pkey table (with 0xffff as only pkey).

Later when the SM moves the port to Active, the SM may program the pkey table differently.  However at this point IPoIB has already started using the incorrect pkey.

It appears that the initially formatted 'broadcast' mgid is never updated to supply actual pkey value if ipoib comes up before hca port. Proposed patch targets two issues:

1. Suppress activation of interface and join multicast group queries (it will fail anyway) until hca port is initialized. When port becomes active - update pkey value and move on.
2. Update broadcast mgid based on actual pkey, then issue join broadcast group request.

Signed-Off-By: Michael Heinz <michael.heinz-h88ZbnxC6KDQT0dZR+AlfA@public.gmane.org>

-------
diff --git a/drivers/infiniband/ulp/ipoib/ipoib_ib.c b/drivers/infiniband/ulp/ipoib/ipoib_ib.c
index ec6b4fb..496d96c 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_ib.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_ib.c
@@ -51,6 +51,7 @@ MODULE_PARM_DESC(data_debug_level,
 #endif
 
 static DEFINE_MUTEX(pkey_mutex);
+static void ipoib_pkey_dev_check_presence(struct net_device *dev);
 
 struct ipoib_ah *ipoib_create_ah(struct net_device *dev,
 				 struct ib_pd *pd, struct ib_ah_attr *attr) @@ -654,12 +655,13 @@ int ipoib_ib_dev_open(struct net_device *dev)
 	struct ipoib_dev_priv *priv = netdev_priv(dev);
 	int ret;
 
-	if (ib_find_pkey(priv->ca, priv->port, priv->pkey, &priv->pkey_index)) {
+	ipoib_pkey_dev_check_presence(dev);
+
+	if (!test_bit(IPOIB_PKEY_ASSIGNED, &priv->flags)) {
 		ipoib_warn(priv, "P_Key 0x%04x not found\n", priv->pkey);
 		clear_bit(IPOIB_PKEY_ASSIGNED, &priv->flags);
 		return -1;
 	}
-	set_bit(IPOIB_PKEY_ASSIGNED, &priv->flags);
 
 	ret = ipoib_init_qp(dev);
 	if (ret) {
@@ -694,9 +696,26 @@ int ipoib_ib_dev_open(struct net_device *dev)  static void ipoib_pkey_dev_check_presence(struct net_device *dev)  {
 	struct ipoib_dev_priv *priv = netdev_priv(dev);
-	u16 pkey_index = 0;
+	struct ib_port_attr    port_attr;
+
+	if (!test_bit(IPOIB_FLAG_SUBINTERFACE, &priv->flags)) {
+		clear_bit(IPOIB_PKEY_ASSIGNED, &priv->flags);
+		if (ib_query_port(priv->ca, priv->port, &port_attr)) {
+			ipoib_warn(priv, "Query port attrs failed\n");
+			return;
+		}
+
+		if (port_attr.state != IB_PORT_ACTIVE)
+			return;
+
+		if (ib_query_pkey(priv->ca, priv->port, 0, &priv->pkey)) {
+			ipoib_warn(priv, "Query P_Key table entry 0 failed\n");
+			return;
+		}
+		set_bit(IPOIB_PKEY_ASSIGNED, &priv->flags);
+	}
 
-	if (ib_find_pkey(priv->ca, priv->port, priv->pkey, &pkey_index))
+	if (ib_find_pkey(priv->ca, priv->port, priv->pkey, &priv->pkey_index))
 		clear_bit(IPOIB_PKEY_ASSIGNED, &priv->flags);
 	else
 		set_bit(IPOIB_PKEY_ASSIGNED, &priv->flags); @@ -955,7 +974,8 @@ static void __ipoib_ib_dev_flush(struct ipoib_dev_priv *priv,
 		}
 
 		/* restart QP only if P_Key index is changed */
-		if (test_and_set_bit(IPOIB_PKEY_ASSIGNED, &priv->flags) &&
+		if (test_bit(IPOIB_FLAG_SUBINTERFACE, &priv->flags) &&
+		    test_and_set_bit(IPOIB_PKEY_ASSIGNED, &priv->flags) &&
 		    new_index == priv->pkey_index) {
 			ipoib_dbg(priv, "Not flushing - P_Key index not changed.\n");
 			return;
diff --git a/drivers/infiniband/ulp/ipoib/ipoib_multicast.c b/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
index 3871ac6..6fe6527 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
@@ -552,6 +552,13 @@ void ipoib_mcast_join_task(struct work_struct *work)
 		}
 
 		spin_lock_irq(&priv->lock);
+
+		if (!test_bit(IPOIB_FLAG_SUBINTERFACE, &priv->flags)) {
+			/* fix broadcast gid in case if pkey was changed */
+			priv->pkey |= 0x8000;
+			priv->dev->broadcast[8] = priv->pkey >> 8;
+			priv->dev->broadcast[9] = priv->pkey & 0xff;
+		}
 		memcpy(broadcast->mcmember.mgid.raw, priv->dev->broadcast + 4,
 		       sizeof (union ib_gid));
 		priv->broadcast = broadcast;
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] pkey fix for ipoib - resubmission

[Eli Cohen]

On Wed, Jun 16, 2010 at 4:59 PM, Mike Heinz <michael.heinz-h88ZbnxC6KDQT0dZR+AlfA@public.gmane.org> wrote:
>
> IPoIB is coded to use the 1st PKey in the PKey table as its ib0 interface. Additional ib0.pkey interfaces may be created using the /sys/class/... add_child interface.
>
> However, there is a race.  During normal boot, IPoIB will be started before the port is Active.  Hence the pkey table has not yet been programmed and has a default pkey table (with 0xffff as only pkey).
So what's wrong with using the default pkey? It is a valid and I don't
see why we should ignore it.

>
> Later when the SM moves the port to Active, the SM may program the pkey table differently.  However at this point IPoIB has already started using the incorrect pkey.
>
> It appears that the initially formatted 'broadcast' mgid is never updated to supply actual pkey value if ipoib comes up before hca port. Proposed patch targets two issues:
>
> 1. Suppress activation of interface and join multicast group queries (it will fail anyway) until hca port is initialized. When port becomes active - update pkey value and move on.
I don't think this is required.


> 2. Update broadcast mgid based on actual pkey, then issue join broadcast group request.
I agree that the broadcast MGID is not updated. But it seems to me
that all that's needed is to update priv->dev->broadcast with the
updated pkey at ipoib_open(). The rest is already taken care of since
pkey change events are handled by IPoIB.



>
> Signed-Off-By: Michael Heinz <michael.heinz-h88ZbnxC6KDQT0dZR+AlfA@public.gmane.org>
>
> -------
> diff --git a/drivers/infiniband/ulp/ipoib/ipoib_ib.c b/drivers/infiniband/ulp/ipoib/ipoib_ib.c
> index ec6b4fb..496d96c 100644
> --- a/drivers/infiniband/ulp/ipoib/ipoib_ib.c
> +++ b/drivers/infiniband/ulp/ipoib/ipoib_ib.c
> @@ -51,6 +51,7 @@ MODULE_PARM_DESC(data_debug_level,
>  #endif
>
>  static DEFINE_MUTEX(pkey_mutex);
> +static void ipoib_pkey_dev_check_presence(struct net_device *dev);
>
>  struct ipoib_ah *ipoib_create_ah(struct net_device *dev,
>                                 struct ib_pd *pd, struct ib_ah_attr *attr) @@ -654,12 +655,13 @@ int ipoib_ib_dev_open(struct net_device *dev)
>        struct ipoib_dev_priv *priv = netdev_priv(dev);
>        int ret;
>
> -       if (ib_find_pkey(priv->ca, priv->port, priv->pkey, &priv->pkey_index)) {
> +       ipoib_pkey_dev_check_presence(dev);
> +
> +       if (!test_bit(IPOIB_PKEY_ASSIGNED, &priv->flags)) {
>                ipoib_warn(priv, "P_Key 0x%04x not found\n", priv->pkey);
>                clear_bit(IPOIB_PKEY_ASSIGNED, &priv->flags);
>                return -1;
>        }
> -       set_bit(IPOIB_PKEY_ASSIGNED, &priv->flags);
>
>        ret = ipoib_init_qp(dev);
>        if (ret) {
> @@ -694,9 +696,26 @@ int ipoib_ib_dev_open(struct net_device *dev)  static void ipoib_pkey_dev_check_presence(struct net_device *dev)  {
>        struct ipoib_dev_priv *priv = netdev_priv(dev);
> -       u16 pkey_index = 0;
> +       struct ib_port_attr    port_attr;
> +
> +       if (!test_bit(IPOIB_FLAG_SUBINTERFACE, &priv->flags)) {
> +               clear_bit(IPOIB_PKEY_ASSIGNED, &priv->flags);
> +               if (ib_query_port(priv->ca, priv->port, &port_attr)) {
> +                       ipoib_warn(priv, "Query port attrs failed\n");
> +                       return;
> +               }
> +
> +               if (port_attr.state != IB_PORT_ACTIVE)
> +                       return;
> +
> +               if (ib_query_pkey(priv->ca, priv->port, 0, &priv->pkey)) {
> +                       ipoib_warn(priv, "Query P_Key table entry 0 failed\n");
> +                       return;
> +               }
> +               set_bit(IPOIB_PKEY_ASSIGNED, &priv->flags);
> +       }
>
> -       if (ib_find_pkey(priv->ca, priv->port, priv->pkey, &pkey_index))
> +       if (ib_find_pkey(priv->ca, priv->port, priv->pkey, &priv->pkey_index))
>                clear_bit(IPOIB_PKEY_ASSIGNED, &priv->flags);
>        else
>                set_bit(IPOIB_PKEY_ASSIGNED, &priv->flags); @@ -955,7 +974,8 @@ static void __ipoib_ib_dev_flush(struct ipoib_dev_priv *priv,
>                }
>
>                /* restart QP only if P_Key index is changed */
> -               if (test_and_set_bit(IPOIB_PKEY_ASSIGNED, &priv->flags) &&
> +               if (test_bit(IPOIB_FLAG_SUBINTERFACE, &priv->flags) &&
> +                   test_and_set_bit(IPOIB_PKEY_ASSIGNED, &priv->flags) &&
>                    new_index == priv->pkey_index) {
>                        ipoib_dbg(priv, "Not flushing - P_Key index not changed.\n");
>                        return;
> diff --git a/drivers/infiniband/ulp/ipoib/ipoib_multicast.c b/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
> index 3871ac6..6fe6527 100644
> --- a/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
> +++ b/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
> @@ -552,6 +552,13 @@ void ipoib_mcast_join_task(struct work_struct *work)
>                }
>
>                spin_lock_irq(&priv->lock);
> +
> +               if (!test_bit(IPOIB_FLAG_SUBINTERFACE, &priv->flags)) {
> +                       /* fix broadcast gid in case if pkey was changed */
> +                       priv->pkey |= 0x8000;
> +                       priv->dev->broadcast[8] = priv->pkey >> 8;
> +                       priv->dev->broadcast[9] = priv->pkey & 0xff;
> +               }
>                memcpy(broadcast->mcmember.mgid.raw, priv->dev->broadcast + 4,
>                       sizeof (union ib_gid));
>                priv->broadcast = broadcast;
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] pkey fix for ipoib - resubmission

[Mike Heinz]

> So what's wrong with using the default pkey? It is a valid and I don't
see why we should ignore it.

In fabrics using quality of service and virtual fabrics, the default pkey is probably the wrong one to use for network traffic - and may not work at all. Remember, the real default pkey is 0x7fff, not 0xffff - and 0x7fff only guarantees communications with the SM not with other nodes.

> I don't think this is required.

It is certainly required for any fabric that does not permit the use of 0xffff as a pkey for ipoib traffic.


-----Original Message-----
From: linux-rdma-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org [mailto:linux-rdma-owner@vger.kernel.org] On Behalf Of Eli Cohen
Sent: Thursday, June 17, 2010 12:37 PM
To: Mike Heinz
Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Roland Dreier
Subject: Re: [PATCH] pkey fix for ipoib - resubmission

On Wed, Jun 16, 2010 at 4:59 PM, Mike Heinz <michael.heinz-h88ZbnxC6KDQT0dZR+AlfA@public.gmane.org> wrote:
>
> IPoIB is coded to use the 1st PKey in the PKey table as its ib0 interface. Additional ib0.pkey interfaces may be created using the /sys/class/... add_child interface.
>
> However, there is a race.  During normal boot, IPoIB will be started before the port is Active.  Hence the pkey table has not yet been programmed and has a default pkey table (with 0xffff as only pkey).
So what's wrong with using the default pkey? It is a valid and I don't
see why we should ignore it.

>
> Later when the SM moves the port to Active, the SM may program the pkey table differently.  However at this point IPoIB has already started using the incorrect pkey.
>
> It appears that the initially formatted 'broadcast' mgid is never updated to supply actual pkey value if ipoib comes up before hca port. Proposed patch targets two issues:
>
> 1. Suppress activation of interface and join multicast group queries (it will fail anyway) until hca port is initialized. When port becomes active - update pkey value and move on.
I don't think this is required.


> 2. Update broadcast mgid based on actual pkey, then issue join broadcast group request.
I agree that the broadcast MGID is not updated. But it seems to me
that all that's needed is to update priv->dev->broadcast with the
updated pkey at ipoib_open(). The rest is already taken care of since
pkey change events are handled by IPoIB.



>
> Signed-Off-By: Michael Heinz <michael.heinz-h88ZbnxC6KDQT0dZR+AlfA@public.gmane.org>
>
> -------
> diff --git a/drivers/infiniband/ulp/ipoib/ipoib_ib.c b/drivers/infiniband/ulp/ipoib/ipoib_ib.c
> index ec6b4fb..496d96c 100644
> --- a/drivers/infiniband/ulp/ipoib/ipoib_ib.c
> +++ b/drivers/infiniband/ulp/ipoib/ipoib_ib.c
> @@ -51,6 +51,7 @@ MODULE_PARM_DESC(data_debug_level,
>  #endif
>
>  static DEFINE_MUTEX(pkey_mutex);
> +static void ipoib_pkey_dev_check_presence(struct net_device *dev);
>
>  struct ipoib_ah *ipoib_create_ah(struct net_device *dev,
>                                 struct ib_pd *pd, struct ib_ah_attr *attr) @@ -654,12 +655,13 @@ int ipoib_ib_dev_open(struct net_device *dev)
>        struct ipoib_dev_priv *priv = netdev_priv(dev);
>        int ret;
>
> -       if (ib_find_pkey(priv->ca, priv->port, priv->pkey, &priv->pkey_index)) {
> +       ipoib_pkey_dev_check_presence(dev);
> +
> +       if (!test_bit(IPOIB_PKEY_ASSIGNED, &priv->flags)) {
>                ipoib_warn(priv, "P_Key 0x%04x not found\n", priv->pkey);
>                clear_bit(IPOIB_PKEY_ASSIGNED, &priv->flags);
>                return -1;
>        }
> -       set_bit(IPOIB_PKEY_ASSIGNED, &priv->flags);
>
>        ret = ipoib_init_qp(dev);
>        if (ret) {
> @@ -694,9 +696,26 @@ int ipoib_ib_dev_open(struct net_device *dev)  static void ipoib_pkey_dev_check_presence(struct net_device *dev)  {
>        struct ipoib_dev_priv *priv = netdev_priv(dev);
> -       u16 pkey_index = 0;
> +       struct ib_port_attr    port_attr;
> +
> +       if (!test_bit(IPOIB_FLAG_SUBINTERFACE, &priv->flags)) {
> +               clear_bit(IPOIB_PKEY_ASSIGNED, &priv->flags);
> +               if (ib_query_port(priv->ca, priv->port, &port_attr)) {
> +                       ipoib_warn(priv, "Query port attrs failed\n");
> +                       return;
> +               }
> +
> +               if (port_attr.state != IB_PORT_ACTIVE)
> +                       return;
> +
> +               if (ib_query_pkey(priv->ca, priv->port, 0, &priv->pkey)) {
> +                       ipoib_warn(priv, "Query P_Key table entry 0 failed\n");
> +                       return;
> +               }
> +               set_bit(IPOIB_PKEY_ASSIGNED, &priv->flags);
> +       }
>
> -       if (ib_find_pkey(priv->ca, priv->port, priv->pkey, &pkey_index))
> +       if (ib_find_pkey(priv->ca, priv->port, priv->pkey, &priv->pkey_index))
>                clear_bit(IPOIB_PKEY_ASSIGNED, &priv->flags);
>        else
>                set_bit(IPOIB_PKEY_ASSIGNED, &priv->flags); @@ -955,7 +974,8 @@ static void __ipoib_ib_dev_flush(struct ipoib_dev_priv *priv,
>                }
>
>                /* restart QP only if P_Key index is changed */
> -               if (test_and_set_bit(IPOIB_PKEY_ASSIGNED, &priv->flags) &&
> +               if (test_bit(IPOIB_FLAG_SUBINTERFACE, &priv->flags) &&
> +                   test_and_set_bit(IPOIB_PKEY_ASSIGNED, &priv->flags) &&
>                    new_index == priv->pkey_index) {
>                        ipoib_dbg(priv, "Not flushing - P_Key index not changed.\n");
>                        return;
> diff --git a/drivers/infiniband/ulp/ipoib/ipoib_multicast.c b/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
> index 3871ac6..6fe6527 100644
> --- a/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
> +++ b/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
> @@ -552,6 +552,13 @@ void ipoib_mcast_join_task(struct work_struct *work)
>                }
>
>                spin_lock_irq(&priv->lock);
> +
> +               if (!test_bit(IPOIB_FLAG_SUBINTERFACE, &priv->flags)) {
> +                       /* fix broadcast gid in case if pkey was changed */
> +                       priv->pkey |= 0x8000;
> +                       priv->dev->broadcast[8] = priv->pkey >> 8;
> +                       priv->dev->broadcast[9] = priv->pkey & 0xff;
> +               }
>                memcpy(broadcast->mcmember.mgid.raw, priv->dev->broadcast + 4,
>                       sizeof (union ib_gid));
>                priv->broadcast = broadcast;
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] pkey fix for ipoib - resubmission

[Mike Heinz]

I never got a response to this patch, so I'm sending it again.

-----------------

IPoIB is coded to use the 1st PKey in the PKey table as its ib0 interface. Additional ib0.pkey interfaces may be created using the /sys/class/... add_child interface.

However, there is a race.  During normal boot, IPoIB will be started before the port is Active.  Hence the pkey table has not yet been programmed and has a default pkey table (with 0xffff as only pkey).

Later when the SM moves the port to Active, the SM may program the pkey table differently.  However at this point IPoIB has already started using the incorrect pkey.

It appears that the initially formatted 'broadcast' mgid is never updated to supply actual pkey value if ipoib comes up before hca port. Proposed patch targets two issues:

1. Suppress activation of interface and join multicast group queries (it will fail anyway) until hca port is initialized. When port becomes active - update pkey value and move on.
2. Update broadcast mgid based on actual pkey, then issue join broadcast group request.

Signed-Off-By: Michael Heinz <michael.heinz-h88ZbnxC6KDQT0dZR+AlfA@public.gmane.org>

-------
diff --git a/drivers/infiniband/ulp/ipoib/ipoib_ib.c b/drivers/infiniband/ulp/ipoib/ipoib_ib.c
index ec6b4fb..496d96c 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_ib.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_ib.c
@@ -51,6 +51,7 @@ MODULE_PARM_DESC(data_debug_level,
 #endif
 
 static DEFINE_MUTEX(pkey_mutex);
+static void ipoib_pkey_dev_check_presence(struct net_device *dev);
 
 struct ipoib_ah *ipoib_create_ah(struct net_device *dev,
 				 struct ib_pd *pd, struct ib_ah_attr *attr) @@ -654,12 +655,13 @@ int ipoib_ib_dev_open(struct net_device *dev)
 	struct ipoib_dev_priv *priv = netdev_priv(dev);
 	int ret;
 
-	if (ib_find_pkey(priv->ca, priv->port, priv->pkey, &priv->pkey_index)) {
+	ipoib_pkey_dev_check_presence(dev);
+
+	if (!test_bit(IPOIB_PKEY_ASSIGNED, &priv->flags)) {
 		ipoib_warn(priv, "P_Key 0x%04x not found\n", priv->pkey);
 		clear_bit(IPOIB_PKEY_ASSIGNED, &priv->flags);
 		return -1;
 	}
-	set_bit(IPOIB_PKEY_ASSIGNED, &priv->flags);
 
 	ret = ipoib_init_qp(dev);
 	if (ret) {
@@ -694,9 +696,26 @@ int ipoib_ib_dev_open(struct net_device *dev)  static void ipoib_pkey_dev_check_presence(struct net_device *dev)  {
 	struct ipoib_dev_priv *priv = netdev_priv(dev);
-	u16 pkey_index = 0;
+	struct ib_port_attr    port_attr;
+
+	if (!test_bit(IPOIB_FLAG_SUBINTERFACE, &priv->flags)) {
+		clear_bit(IPOIB_PKEY_ASSIGNED, &priv->flags);
+		if (ib_query_port(priv->ca, priv->port, &port_attr)) {
+			ipoib_warn(priv, "Query port attrs failed\n");
+			return;
+		}
+
+		if (port_attr.state != IB_PORT_ACTIVE)
+			return;
+
+		if (ib_query_pkey(priv->ca, priv->port, 0, &priv->pkey)) {
+			ipoib_warn(priv, "Query P_Key table entry 0 failed\n");
+			return;
+		}
+		set_bit(IPOIB_PKEY_ASSIGNED, &priv->flags);
+	}
 
-	if (ib_find_pkey(priv->ca, priv->port, priv->pkey, &pkey_index))
+	if (ib_find_pkey(priv->ca, priv->port, priv->pkey, &priv->pkey_index))
 		clear_bit(IPOIB_PKEY_ASSIGNED, &priv->flags);
 	else
 		set_bit(IPOIB_PKEY_ASSIGNED, &priv->flags); @@ -955,7 +974,8 @@ static void __ipoib_ib_dev_flush(struct ipoib_dev_priv *priv,
 		}
 
 		/* restart QP only if P_Key index is changed */
-		if (test_and_set_bit(IPOIB_PKEY_ASSIGNED, &priv->flags) &&
+		if (test_bit(IPOIB_FLAG_SUBINTERFACE, &priv->flags) &&
+		    test_and_set_bit(IPOIB_PKEY_ASSIGNED, &priv->flags) &&
 		    new_index == priv->pkey_index) {
 			ipoib_dbg(priv, "Not flushing - P_Key index not changed.\n");
 			return;
diff --git a/drivers/infiniband/ulp/ipoib/ipoib_multicast.c b/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
index 3871ac6..6fe6527 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
@@ -552,6 +552,13 @@ void ipoib_mcast_join_task(struct work_struct *work)
 		}
 
 		spin_lock_irq(&priv->lock);
+
+		if (!test_bit(IPOIB_FLAG_SUBINTERFACE, &priv->flags)) {
+			/* fix broadcast gid in case if pkey was changed */
+			priv->pkey |= 0x8000;
+			priv->dev->broadcast[8] = priv->pkey >> 8;
+			priv->dev->broadcast[9] = priv->pkey & 0xff;
+		}
 		memcpy(broadcast->mcmember.mgid.raw, priv->dev->broadcast + 4,
 		       sizeof (union ib_gid));
 		priv->broadcast = broadcast;
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] pkey fix for ipoib - resubmission

[Jason Gunthorpe]

On Fri, Jun 18, 2010 at 12:09:25PM -0500, Mike Heinz wrote:

> 1. Suppress activation of interface and join multicast group queries
> (it will fail anyway) until hca port is initialized. When port
> becomes active - update pkey value and move on.

> 2. Update broadcast mgid based on actual pkey, then issue join
> broadcast group request.

What happens if the SM reprograms the pkey table later?

I'm not sure this is the right approach.. See the recent dicussion
about using pkey indexes at all.

Each ipoib interface is assigned a pkey, by number, not index. If the
SM has not supplied that pkey to the port then the ipoib should not
come up. If the SM adds it later then it should come up, if it the SM
removes it later then it should go down.

The default ipoib interface uses the default pkey. If your network
need ipoib to use a different pkey then you have to change the pkey
number assignment when the ipoib interface is created, not magically
attempt to autoconfigure.

I believe you cannot safely change the broadcast GID once the ipoib
interface has started.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [ewg] [PATCH] pkey fix for ipoib - resubmission

[Todd Rimmer]

Jason wrote:

> What happens if the SM reprograms the pkey table later?
> 
> I'm not sure this is the right approach.. See the recent dicussion
> about using pkey indexes at all.
> 
> Each ipoib interface is assigned a pkey, by number, not index. If the
> SM has not supplied that pkey to the port then the ipoib should not
> come up. If the SM adds it later then it should come up, if it the SM
> removes it later then it should go down.
> 
> The default ipoib interface uses the default pkey. If your network
> need ipoib to use a different pkey then you have to change the pkey
> number assignment when the ipoib interface is created, not magically
> attempt to autoconfigure.
> 
> I believe you cannot safely change the broadcast GID once the ipoib
> interface has started.

The goal for IPoIB should be a mechanism which can mimic the ease of setting up VLANs in Ethernet.  In Ethernet a default VLAN can be programmed into a switch in which case no configuration of the server is needed.  Ethernet switch vendors often provide centralized tools for setting up VLANS in all the switches.  It is only when you want a given server to participate in multiple VLANs that you need to setup specific configuration on the server to do so.

For IB, the centralized tool is the SM and a "VLAN" is an IB Partition.  The present capability of using the 1st PKey table entry is a nice simple and powerful way to mimic the ease of use of Ethernet VLANs.  Only when a user wants IPoIB to run on two different partitions (which is rare) is a server specific configuration needed.

When IPoIB sees a PKey table change, it will reregister with the broadcast GID.  PKey table changes typically also involve a restart of the SM and hence a reregistration request is necessary anyway.

Todd Rimmer
Chief Architect 
QLogic Network Systems Group
Voice: 610-233-4852     Fax: 610-233-4777
Todd.Rimmer-8votvDbAKxjQT0dZR+AlfA@public.gmane.org  www.QLogic.com
 

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [ewg] [PATCH] pkey fix for ipoib - resubmission

[Jason Gunthorpe]

On Fri, Jun 18, 2010 at 12:32:52PM -0500, Todd Rimmer wrote:

> For IB, the centralized tool is the SM and a "VLAN" is an IB
> Partition.  The present capability of using the 1st PKey table entry
> is a nice simple and powerful way to mimic the ease of use of
> Ethernet VLANs.  Only when a user wants IPoIB to run on two
> different partitions (which is rare) is a server specific
> configuration needed.

I agree that automatic configuration is desirable, but this isn't how
the Linux ipoib implementation is designed.

You cannot easily change the broadcast GID after starting. It gets
into places, and this patch does not address that. I suspect that
un-doing all the places the GID gets into is fairly hard..

Certainly, changing the pkey once the interface has been running for
awhile is involved, and 'auto configuration' that works once at boot
is, IMHO, evil.

So.. to go down this path, I think the first patch should be: 'here
is a patch that lets us change the pkey on a running interface, and
here is a detailed discussion of why it is correct and race
free'.

With the current code the simplest approach to this would be to
destroy the entire interface and make a new one with the pkey you
want. Since doing this requires preserving a lot of net state this is
best done from user space using the existing APIs.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [ewg] [PATCH] pkey fix for ipoib - resubmission

[Todd Rimmer]

Jason wrote:
> You cannot easily change the broadcast GID after starting. It gets
> into places, and this patch does not address that. I suspect that
> un-doing all the places the GID gets into is fairly hard..

We have successfully tested this patch by bouncing the SM with a new partition table config and IPoIB successfully moved to the new Pkey and continued working.  Within IPoIB this code was the only remaining place which incorrectly used the stale Pkey which the hardware initialized to prior to the SM programming the PKey table.

In any case, that particular requirement is less important than the ability for a server to boot to the correct pkey once at boot time, without needing specific configuration files created on the server which match the SM's configuration.

This patch clear addresses the more important requirement and has been observed in our test team to successfully handle the Partition table change case as well.

Todd Rimmer
Chief Architect 
QLogic Network Systems Group
Voice: 610-233-4852     Fax: 610-233-4777
Todd.Rimmer-8votvDbAKxjQT0dZR+AlfA@public.gmane.org  www.QLogic.com
 

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [ewg] [PATCH] pkey fix for ipoib - resubmission

[Jason Gunthorpe]

On Fri, Jun 18, 2010 at 12:56:56PM -0500, Todd Rimmer wrote:
> Jason wrote:
> > You cannot easily change the broadcast GID after starting. It gets
> > into places, and this patch does not address that. I suspect that
> > un-doing all the places the GID gets into is fairly hard..
> 
> We have successfully tested this patch by bouncing the SM with a new
> partition table config and IPoIB successfully moved to the new Pkey
> and continued working.  Within IPoIB this code was the only
> remaining place which incorrectly used the stale Pkey which the
> hardware initialized to prior to the SM programming the PKey table.

Uhm, no... the pkey is copied from the broadcast GID into many other
places. For instance it is used by the kernel to construct the
hwardware address for all IP/IPv6 joins. Without fixing up the maddr
table you break all existing multicast joins, which will completely
break IPv6, and breaks corner cases for IPv4..

Be aware that mainline and OFED are different in this regard, OFED
overrides the pkey unconditionally for multicast addresses, while
mainline does not.

Fixing the maddr table, and dealing with race issues with ongoing
joins my not be straightforward, but, IMHO, necessary for this patch
to be acceptable.

ipoib bonding had much the same problem with invalid maddrs, and a
patch was put in that flushed the maddr table in certain bond
scenarios. Perhaps something like that is a straightforward solution
here as well?

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [ewg] [PATCH] pkey fix for ipoib - resubmission

[Todd Rimmer]

Jason,

> Fixing the maddr table, and dealing with race issues with ongoing
> joins my not be straightforward, but, IMHO, necessary for this patch
> to be acceptable.
> 

So you are pointing out an existing bug, which is no worse with this patch than without it.  Namely that pkey changes with IPv6 may have issues.  IPv4 pkey changes should be ok.

In any case, this patch still successfully addresses the most important requirement of having a rebooted server come up with the correct MGID.

In at least the HPC world, PKey changes are extremely rare, however having server's reliably boot is important.

> ipoib bonding had much the same problem with invalid maddrs, and a
> patch was put in that flushed the maddr table in certain bond
> scenarios. Perhaps something like that is a straightforward solution
> here as well?
Perhaps that could be a second patch which would build on top of this initial fix for the most important requirement.  It would seem that using the PKEY change and/or re-registration HCA events to trigger a maddr flush in IPoIB would be straightforward.  If that approach worked well for bonding, it should work equally well in these cases.

Todd Rimmer
Chief Architect 
QLogic Network Systems Group
Voice: 610-233-4852     Fax: 610-233-4777
Todd.Rimmer-8votvDbAKxjQT0dZR+AlfA@public.gmane.org  www.QLogic.com
 

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [ewg] [PATCH] pkey fix for ipoib - resubmission

[Jason Gunthorpe]

On Fri, Jun 18, 2010 at 01:38:13PM -0500, Todd Rimmer wrote:
> Jason,
> 
> > Fixing the maddr table, and dealing with race issues with ongoing
> > joins my not be straightforward, but, IMHO, necessary for this patch
> > to be acceptable.

> So you are pointing out an existing bug, which is no worse with this
> patch than without it.  Namely that pkey changes with IPv6 may have
> issues.  IPv4 pkey changes should be ok.

No.. It is my understanding, that today, the pkey of an IPOIB
interface can never change once it is established.

This patch changes that invariant without actually addressing all
the complexity of making it work.

> In at least the HPC world, PKey changes are extremely rare, however
> having server's reliably boot is important.

Various IPv6 multicast joins occur as soon as the interface is up'd,
using the pkey in-place at that time. Changing the pkey while the
interface is down might work OK, but changing it while it is up'd will
break all multicast joins that were started before, ie IPv6.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [ewg] [PATCH] pkey fix for ipoib - resubmission

[Or Gerlitz]

Jason Gunthorpe wrote:
> Be aware that mainline and OFED are different in this regard, OFED overrides 
> the pkey unconditionally for multicast addresses, while mainline doesn't
 
Can you clarify this, please?

> ipoib bonding had much the same problem with invalid maddrs, and a
> patch was put in that flushed the maddr table in certain bond scenarios. 

Yes, reading through this thread, I tend to agree with Jason that we're in the same boat (problem) that used to be for bonding/ipoib and was fixed in commit 75c78500ddad74b229cd0691496b8549490496a2  "bonding: remap multicast addresses without using dev_close() and dev_open()", so I assume a similar solution can/should be applied here as well, unless someone comes with a magic approach to eliminate the problem all together...

Or.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [ewg] [PATCH] pkey fix for ipoib - resubmission

[Jason Gunthorpe]

On Tue, Jun 22, 2010 at 09:09:01AM +0300, Or Gerlitz wrote:
> Jason Gunthorpe wrote:
> > Be aware that mainline and OFED are different in this regard, OFED overrides 
> > the pkey unconditionally for multicast addresses, while mainline doesn't
>  
> Can you clarify this, please?

Hmm.. OFED works on kernels that have compiled-in inline'd multicast
map functions that do not include the pkey copy, while mainline's
multicast map functions do. So to work around this there is a bit of
code in OFED to overwrite the pkey in the multicast hw address. This
means on OFED with those kernels ip maddr returns the wrong hw
address sometimes..

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [ewg] [PATCH] pkey fix for ipoib - resubmission

[Or Gerlitz]

Jason Gunthorpe wrote:
> OFED works on kernels that have compiled-in inline'd multicast map functions that do not include the pkey copy, while mainline's multicast map functions do. So to work around this there is a bit of code in OFED to overwrite the pkey in the multicast hw address. This means on OFED with those kernels ip maddr returns the wrong hw address sometimes..
okay, got it. Anyway, with this not being the essence of the patch nor 
the discussion here, I would wait to hear what  Todd and Mike think 
about your suggestion to apply the approach taken for the bonding 
problem and solution.

Or.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html