Hi Or,
Please see below.
> -----Original Message-----
> From: Or Gerlitz [mailto:[email protected]]
> Sent: Tuesday, June 17, 2014 2:02 AM
> To: Estrin, Alex; Roland Dreier
> Cc: [email protected]; 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 <[email protected]>
> > Signed-off-by: Alex Estrin <[email protected]>
>
> 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.
