| Message ID | 20140611192132.31937.30869.stgit@phlsvlogin03.ph.intel.com (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
6/11/2014 10:22 PM, Alex Estrin: > With reference to commit c2904141696ee19551f1553944446f23cdd5d95e > (IPoIB: Fix pkey change flow for virtualization environments) > It was noticed that parent interface keeps sending broadcast group > join requests if p_key index 0 is invalid. That creates unnecessary > noise on a fabric: > > ib0: multicast join failed for ff12:401b:8000:0000:0000:0000:ffff:ffff, > status -22 > > Proposed patch re-init resources, then brings interface > up only if p_key idx 0 is valid either on bootup or on PKEY_CHANGE event. > Original code could run multicast task regardless of p_key value, > which we want to avoid. > > Reviewed-by: Ira Weiny <ira.weiny@intel.com> > Signed-off-by: Alex Estrin <alex.estrin@intel.com> Acked-by: Erez Shitrit <erezsh@dev.mellanox.co.il> > --- > drivers/infiniband/ulp/ipoib/ipoib_ib.c | 35 +++++++++++++++++++------------ > 1 files changed, 21 insertions(+), 14 deletions(-) > > diff --git a/drivers/infiniband/ulp/ipoib/ipoib_ib.c b/drivers/infiniband/ulp/ipoib/ipoib_ib.c > index 6a7003d..a2af996 100644 > --- a/drivers/infiniband/ulp/ipoib/ipoib_ib.c > +++ b/drivers/infiniband/ulp/ipoib/ipoib_ib.c > @@ -52,6 +52,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) > @@ -669,12 +670,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_warn(priv, "P_Key 0x%04x not found\n", priv->pkey); > - clear_bit(IPOIB_PKEY_ASSIGNED, &priv->flags); > + ipoib_pkey_dev_check_presence(dev); > + > + if (!test_bit(IPOIB_PKEY_ASSIGNED, &priv->flags)) { > + ipoib_warn(priv, "P_Key 0x%04x is %s\n", priv->pkey, > + !(priv->pkey & 0x7fff) ? "Invalid" : "not found"); > return -1; > } > - set_bit(IPOIB_PKEY_ASSIGNED, &priv->flags); > > ret = ipoib_init_qp(dev); > if (ret) { > @@ -712,9 +714,10 @@ dev_stop: > static void ipoib_pkey_dev_check_presence(struct net_device *dev) > { > struct ipoib_dev_priv *priv = netdev_priv(dev); > - u16 pkey_index = 0; > > - if (ib_find_pkey(priv->ca, priv->port, priv->pkey, &pkey_index)) > + if (!(priv->pkey & 0x7fff) || > + 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); > @@ -987,15 +990,17 @@ static void __ipoib_ib_dev_flush(struct ipoib_dev_priv *priv, > up_read(&priv->vlan_rwsem); > > if (!test_bit(IPOIB_FLAG_INITIALIZED, &priv->flags)) { > - /* for non-child devices must check/update the pkey value here */ > - if (level == IPOIB_FLUSH_HEAVY && > - !test_bit(IPOIB_FLAG_SUBINTERFACE, &priv->flags)) > - update_parent_pkey(priv); > - ipoib_dbg(priv, "Not flushing - IPOIB_FLAG_INITIALIZED not set.\n"); > - return; > + if (level != IPOIB_FLUSH_HEAVY) { > + ipoib_dbg(priv, "Not flushing - IPOIB_FLAG_INITIALIZED not set.\n"); > + return; > + } > } > > if (!test_bit(IPOIB_FLAG_ADMIN_UP, &priv->flags)) { > + /* interface is down. update pkey and leave. */ > + if (level == IPOIB_FLUSH_HEAVY && > + !test_bit(IPOIB_FLAG_SUBINTERFACE, &priv->flags)) > + update_parent_pkey(priv); > ipoib_dbg(priv, "Not flushing - IPOIB_FLAG_ADMIN_UP not set.\n"); > return; > } > @@ -1038,8 +1043,10 @@ static void __ipoib_ib_dev_flush(struct ipoib_dev_priv *priv, > ipoib_ib_dev_down(dev, 0); > > if (level == IPOIB_FLUSH_HEAVY) { > - ipoib_ib_dev_stop(dev, 0); > - ipoib_ib_dev_open(dev); > + if (test_bit(IPOIB_FLAG_INITIALIZED, &priv->flags)) > + ipoib_ib_dev_stop(dev, 0); > + if (ipoib_ib_dev_open(dev) != 0) > + return; > } > > /* > > -- > 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 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 11/06/2014 22:22, Alex Estrin wrote: > With reference to commit c2904141696ee19551f1553944446f23cdd5d95e > (IPoIB: Fix pkey change flow for virtualization environments) > It was noticed that parent interface keeps sending broadcast group > join requests if p_key index 0 is invalid. That creates unnecessary > noise on a fabric: > > ib0: multicast join failed for ff12:401b:8000:0000:0000:0000:ffff:ffff, > status -22 > > Proposed patch re-init resources, then brings interface > up only if p_key idx 0 is valid either on bootup or on PKEY_CHANGE event. > Original code could run multicast task regardless of p_key value, > which we want to avoid. > > Reviewed-by: Ira Weiny <ira.weiny@intel.com> > Signed-off-by: Alex Estrin <alex.estrin@intel.com> Hi Alex, I re-wrote the change-log to make it clearer and also follow some practices we use in the kernel (mentioned them to you over prev posts, but maybe it wasn't clear enough...) also see some comments further down the patch -- IPoIB: Avoid multicast join attempts when having invalid p_key Currently, the parent interface keeps sending broadcast group join requests even if p_key index 0 is invalid, which for itself is possible/common in virtualized environmentswhere a VF has been probed to VM but the actual p_key configuration has not yet been assigned by the management software. This creates unnecessary noise on the fabric and in the kernel logs: ib0: multicast join failed for ff12:401b:8000:0000:0000:0000:ffff:ffff, status -22 The original code run the multicast task regardless of the actual p_key value, which can be avoided. The fix is to re-init resources and bring interface up only if p_key index 0 is valid either when starting up or on PKEY_CHANGE event. Fixes: c290414169 ('IPoIB: Fix pkey change flow for virtualization environments') > --- > drivers/infiniband/ulp/ipoib/ipoib_ib.c | 35 +++++++++++++++++++------------ > 1 files changed, 21 insertions(+), 14 deletions(-) > > diff --git a/drivers/infiniband/ulp/ipoib/ipoib_ib.c b/drivers/infiniband/ulp/ipoib/ipoib_ib.c > index 6a7003d..a2af996 100644 > --- a/drivers/infiniband/ulp/ipoib/ipoib_ib.c > +++ b/drivers/infiniband/ulp/ipoib/ipoib_ib.c > @@ -52,6 +52,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) > @@ -669,12 +670,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_warn(priv, "P_Key 0x%04x not found\n", priv->pkey); > - clear_bit(IPOIB_PKEY_ASSIGNED, &priv->flags); > + ipoib_pkey_dev_check_presence(dev); > + > + if (!test_bit(IPOIB_PKEY_ASSIGNED, &priv->flags)) { > + ipoib_warn(priv, "P_Key 0x%04x is %s\n", priv->pkey, > + !(priv->pkey & 0x7fff) ? "Invalid" : "not found"); CHECK: Alignment should match open parenthesis #44: FILE: drivers/infiniband/ulp/ipoib/ipoib_ib.c:677: + ipoib_warn(priv, "P_Key 0x%04x is %s\n", priv->pkey, + !(priv->pkey & 0x7fff) ? "Invalid" : "not found"); please run checkpatch --strict on your patch to fix this one and it's such (there are more) basically the guideline (based on a quote ofDave Miller ...) is: when you have a multi-line function call or if () conditional,the first non-whitespace character on the second and subsequent lines should line up with the first column after the openning parenthesis on the first line. @@ -1038,8 +1043,10 @@ static void __ipoib_ib_dev_flush(struct ipoib_dev_priv *priv, ipoib_ib_dev_down(dev, 0); if (level == IPOIB_FLUSH_HEAVY) { - ipoib_ib_dev_stop(dev, 0); - ipoib_ib_dev_open(dev); + if (test_bit(IPOIB_FLAG_INITIALIZED, &priv->flags)) + ipoib_ib_dev_stop(dev, 0); + if (ipoib_ib_dev_open(dev) != 0) + return; } is testing the return code of ipoib_ib_dev_open() really part of the functional/fix change introduced by this patch or a different fix? if the latter, put to different/future patch -- 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
Hi Or, Please see below. > -----Original Message----- > From: Or Gerlitz [mailto:ogerlitz@mellanox.com] > Sent: Tuesday, June 17, 2014 2:02 AM > To: Estrin, Alex; Roland Dreier > Cc: linux-rdma@vger.kernel.org; Erez Shitrit > Subject: Re: [PATCH v3 1/1] IPoIB: Improve parent interface p_key handling. > > On 11/06/2014 22:22, Alex Estrin wrote: > > With reference to commit c2904141696ee19551f1553944446f23cdd5d95e > > (IPoIB: Fix pkey change flow for virtualization environments) > > It was noticed that parent interface keeps sending broadcast group > > join requests if p_key index 0 is invalid. That creates unnecessary > > noise on a fabric: > > > > ib0: multicast join failed for ff12:401b:8000:0000:0000:0000:ffff:ffff, > > status -22 > > > > Proposed patch re-init resources, then brings interface > > up only if p_key idx 0 is valid either on bootup or on PKEY_CHANGE event. > > Original code could run multicast task regardless of p_key value, > > which we want to avoid. > > > > Reviewed-by: Ira Weiny <ira.weiny@intel.com> > > Signed-off-by: Alex Estrin <alex.estrin@intel.com> > > Hi Alex, I re-wrote the change-log to make it clearer and also follow some > practices we use in the kernel (mentioned them to you over prev posts, > but maybe > it wasn't clear enough...) also see some comments further down the patch -- > > IPoIB: Avoid multicast join attempts when having invalid p_key > > Currently, the parent interface keeps sending broadcast group join > requests even if p_key index 0 is invalid, which for itself is > possible/common in virtualized environmentswhere a VF has been probed to > VM but the actual p_key configuration has not yet been assigned by the > management software. This creates unnecessary noise on the fabric and in > the kernel logs: > > ib0: multicast join failed for ff12:401b:8000:0000:0000:0000:ffff:ffff, > status -22 > > The original code run the multicast task regardless of the actual > p_key value, which can be avoided. The fix is to re-init resources and > bring interface up only if p_key index 0 is valid either when starting > up or on PKEY_CHANGE event. > > Fixes: c290414169 ('IPoIB: Fix pkey change flow for virtualization environments') Thanks, I'll update the change-log. > > --- > > drivers/infiniband/ulp/ipoib/ipoib_ib.c | 35 +++++++++++++++++++------------ > > 1 files changed, 21 insertions(+), 14 deletions(-) > > > > diff --git a/drivers/infiniband/ulp/ipoib/ipoib_ib.c > b/drivers/infiniband/ulp/ipoib/ipoib_ib.c > > index 6a7003d..a2af996 100644 > > --- a/drivers/infiniband/ulp/ipoib/ipoib_ib.c > > +++ b/drivers/infiniband/ulp/ipoib/ipoib_ib.c > > @@ -52,6 +52,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) > > @@ -669,12 +670,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_warn(priv, "P_Key 0x%04x not found\n", priv->pkey); > > - clear_bit(IPOIB_PKEY_ASSIGNED, &priv->flags); > > + ipoib_pkey_dev_check_presence(dev); > > + > > + if (!test_bit(IPOIB_PKEY_ASSIGNED, &priv->flags)) { > > + ipoib_warn(priv, "P_Key 0x%04x is %s\n", priv->pkey, > > + !(priv->pkey & 0x7fff) ? "Invalid" : "not found"); > CHECK: Alignment should match open parenthesis > #44: FILE: drivers/infiniband/ulp/ipoib/ipoib_ib.c:677: > + ipoib_warn(priv, "P_Key 0x%04x is %s\n", priv->pkey, > + !(priv->pkey & 0x7fff) ? "Invalid" : "not found"); > > please run checkpatch --strict on your patch to fix this one and it's > such (there are more) basically the guideline (based on a quote ofDave > Miller ...) is: > > when you have a multi-line function call or if () conditional,the first > non-whitespace character on the second and subsequent lines should line > up with the first column after the openning parenthesis on the first line. > Thanks for the tip with '--strict' option. > @@ -1038,8 +1043,10 @@ static void __ipoib_ib_dev_flush(struct ipoib_dev_priv > *priv, > ipoib_ib_dev_down(dev, 0); > > if (level == IPOIB_FLUSH_HEAVY) { > - ipoib_ib_dev_stop(dev, 0); > - ipoib_ib_dev_open(dev); > + if (test_bit(IPOIB_FLAG_INITIALIZED, &priv->flags)) > + ipoib_ib_dev_stop(dev, 0); > + if (ipoib_ib_dev_open(dev) != 0) > + return; > } > > > is testing the return code of ipoib_ib_dev_open() really part of the > functional/fix change introduced by this patch or a different fix? if > the latter, put to different/future patch Return code test was introduced as a part of the patch. It allows early return from the event handler ( therefore bypass multicast task restart) if updated p_key is invalid. Potential scenario for this case - transition from valid to invalid p_key value at runtime. Thanks, Alex.
diff --git a/drivers/infiniband/ulp/ipoib/ipoib_ib.c b/drivers/infiniband/ulp/ipoib/ipoib_ib.c index 6a7003d..a2af996 100644 --- a/drivers/infiniband/ulp/ipoib/ipoib_ib.c +++ b/drivers/infiniband/ulp/ipoib/ipoib_ib.c @@ -52,6 +52,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) @@ -669,12 +670,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_warn(priv, "P_Key 0x%04x not found\n", priv->pkey); - clear_bit(IPOIB_PKEY_ASSIGNED, &priv->flags); + ipoib_pkey_dev_check_presence(dev); + + if (!test_bit(IPOIB_PKEY_ASSIGNED, &priv->flags)) { + ipoib_warn(priv, "P_Key 0x%04x is %s\n", priv->pkey, + !(priv->pkey & 0x7fff) ? "Invalid" : "not found"); return -1; } - set_bit(IPOIB_PKEY_ASSIGNED, &priv->flags); ret = ipoib_init_qp(dev); if (ret) { @@ -712,9 +714,10 @@ dev_stop: static void ipoib_pkey_dev_check_presence(struct net_device *dev) { struct ipoib_dev_priv *priv = netdev_priv(dev); - u16 pkey_index = 0; - if (ib_find_pkey(priv->ca, priv->port, priv->pkey, &pkey_index)) + if (!(priv->pkey & 0x7fff) || + 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); @@ -987,15 +990,17 @@ static void __ipoib_ib_dev_flush(struct ipoib_dev_priv *priv, up_read(&priv->vlan_rwsem); if (!test_bit(IPOIB_FLAG_INITIALIZED, &priv->flags)) { - /* for non-child devices must check/update the pkey value here */ - if (level == IPOIB_FLUSH_HEAVY && - !test_bit(IPOIB_FLAG_SUBINTERFACE, &priv->flags)) - update_parent_pkey(priv); - ipoib_dbg(priv, "Not flushing - IPOIB_FLAG_INITIALIZED not set.\n"); - return; + if (level != IPOIB_FLUSH_HEAVY) { + ipoib_dbg(priv, "Not flushing - IPOIB_FLAG_INITIALIZED not set.\n"); + return; + } } if (!test_bit(IPOIB_FLAG_ADMIN_UP, &priv->flags)) { + /* interface is down. update pkey and leave. */ + if (level == IPOIB_FLUSH_HEAVY && + !test_bit(IPOIB_FLAG_SUBINTERFACE, &priv->flags)) + update_parent_pkey(priv); ipoib_dbg(priv, "Not flushing - IPOIB_FLAG_ADMIN_UP not set.\n"); return; } @@ -1038,8 +1043,10 @@ static void __ipoib_ib_dev_flush(struct ipoib_dev_priv *priv, ipoib_ib_dev_down(dev, 0); if (level == IPOIB_FLUSH_HEAVY) { - ipoib_ib_dev_stop(dev, 0); - ipoib_ib_dev_open(dev); + if (test_bit(IPOIB_FLAG_INITIALIZED, &priv->flags)) + ipoib_ib_dev_stop(dev, 0); + if (ipoib_ib_dev_open(dev) != 0) + return; } /*