Re: [ofa-general] [PATCHv2 RESEND] IB/IPoIB: Don't let a bad muticast address in the join list stop subsequent joins
Roland Dreier wrote:
> > Illegal multicast address can be handed for IPoIB from userspace. For
> example
> > the command ip maddr add 33:33:00:00:00:01 dev ib0 injects an illegal
> muticast
> > address to IPoIB that will start a join task for this address. However,
> whenever
> > an illegal multicast address is passed to IPoIB it stops all subsequent
> > requests from join attempts. That happens because IPoIB joins to multicast
> > addresses in the order they arrived and doesn't handle the next address
> until the
> > current address join finishes with success.
> >
> > This patch moves the multicast address to the end of the list after a join
> attempt.
> > Even if the join fails the next attempt will be with a different address.
> >
> > Signed-off-by: Moni Shoua
>
> Was a consensus ever reached on this patch?
Not yet. I got a comment from Jason which I intend to refer to soon.
>
> > - if (test_bit(IPOIB_MCAST_RUN, &priv->flags))
> > - queue_delayed_work(ipoib_workqueue, &priv->mcast_task,
> > - mcast->backoff * HZ);
> > + if (test_bit(IPOIB_MCAST_RUN, &priv->flags)) {
> > + list_for_each_entry(next_mcast, &priv->multicast_list, list) {
> > + if (!test_bit(IPOIB_MCAST_FLAG_SENDONLY,
> &next_mcast->flags)
> > + && !test_bit(IPOIB_MCAST_FLAG_BUSY,
> &next_mcast->flags)
> > + && !test_bit(IPOIB_MCAST_FLAG_ATTACHED,
> &next_mcast->flags))
> > + break;
> > + }
> > + if (&next_mcast->list != &priv->multicast_list)
> > + queue_delayed_work(ipoib_workqueue, &priv->mcast_task,
> > + next_mcast->backoff * HZ);
> > + }
>
> I have to admit this duplicated loop doesn't look that attractive to
> me... maybe factor it out into a helper or something?
I'll take this comment to into the next version of the patch. thanks
>
> - R.
> ___
> general mailing list
> [email protected]
> http://lists.openfabrics.org/cgi-bin/mailman/listinfo/general
>
> To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general
>
___
general mailing list
[email protected]
http://lists.openfabrics.org/cgi-bin/mailman/listinfo/general
To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general
Re: [ofa-general] [PATCHv2 RESEND] IB/IPoIB: Don't let a bad muticast address in the join list stop subsequent joins
> Illegal multicast address can be handed for IPoIB from userspace. For example
> the command ip maddr add 33:33:00:00:00:01 dev ib0 injects an illegal
> muticast
> address to IPoIB that will start a join task for this address. However,
> whenever
> an illegal multicast address is passed to IPoIB it stops all subsequent
> requests from join attempts. That happens because IPoIB joins to multicast
> addresses in the order they arrived and doesn't handle the next address
> until the
> current address join finishes with success.
>
> This patch moves the multicast address to the end of the list after a join
> attempt.
> Even if the join fails the next attempt will be with a different address.
>
> Signed-off-by: Moni Shoua
Was a consensus ever reached on this patch?
> -if (test_bit(IPOIB_MCAST_RUN, &priv->flags))
> -queue_delayed_work(ipoib_workqueue, &priv->mcast_task,
> - mcast->backoff * HZ);
> +if (test_bit(IPOIB_MCAST_RUN, &priv->flags)) {
> +list_for_each_entry(next_mcast, &priv->multicast_list, list) {
> +if (!test_bit(IPOIB_MCAST_FLAG_SENDONLY,
> &next_mcast->flags)
> +&& !test_bit(IPOIB_MCAST_FLAG_BUSY,
> &next_mcast->flags)
> +&& !test_bit(IPOIB_MCAST_FLAG_ATTACHED,
> &next_mcast->flags))
> +break;
> +}
> +if (&next_mcast->list != &priv->multicast_list)
> +queue_delayed_work(ipoib_workqueue, &priv->mcast_task,
> +next_mcast->backoff * HZ);
> +}
I have to admit this duplicated loop doesn't look that attractive to
me... maybe factor it out into a helper or something?
- R.
___
general mailing list
[email protected]
http://lists.openfabrics.org/cgi-bin/mailman/listinfo/general
To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general
[ofa-general] [PATCHv2 RESEND] IB/IPoIB: Don't let a bad muticast address in the join list stop subsequent joins
Hi Roland
http://lists.openfabrics.org/pipermail/general/2009-July/060496.html
The discussion in the link above didn't end with a decision. You were asking
about a way to inject illegal mcast addresses from userspace to ib_ipoib and
Jason pointed about such (described below). Could you please review the patch?
thanks
MoniS
---
Illegal multicast address can be handed for IPoIB from userspace. For example
the command ip maddr add 33:33:00:00:00:01 dev ib0 injects an illegal muticast
address to IPoIB that will start a join task for this address. However, whenever
an illegal multicast address is passed to IPoIB it stops all subsequent
requests from join attempts. That happens because IPoIB joins to multicast
addresses in the order they arrived and doesn't handle the next address until
the
current address join finishes with success.
This patch moves the multicast address to the end of the list after a join
attempt.
Even if the join fails the next attempt will be with a different address.
Signed-off-by: Moni Shoua
--
drivers/infiniband/ulp/ipoib/ipoib_multicast.c | 20
1 file changed, 16 insertions(+), 4 deletions(-)
diff --git a/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
b/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
index a0e9753..3c3c63d 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
@@ -379,6 +379,7 @@ static int ipoib_mcast_join_complete(int status,
struct ipoib_mcast *mcast = multicast->context;
struct net_device *dev = mcast->dev;
struct ipoib_dev_priv *priv = netdev_priv(dev);
+ struct ipoib_mcast *next_mcast;
ipoib_dbg_mcast(priv, "join completion for %pI6 (status %d)\n",
mcast->mcmember.mgid.raw, status);
@@ -427,9 +428,17 @@ static int ipoib_mcast_join_complete(int status,
mutex_lock(&mcast_mutex);
spin_lock_irq(&priv->lock);
- if (test_bit(IPOIB_MCAST_RUN, &priv->flags))
- queue_delayed_work(ipoib_workqueue, &priv->mcast_task,
- mcast->backoff * HZ);
+ if (test_bit(IPOIB_MCAST_RUN, &priv->flags)) {
+ list_for_each_entry(next_mcast, &priv->multicast_list, list) {
+ if (!test_bit(IPOIB_MCAST_FLAG_SENDONLY,
&next_mcast->flags)
+ && !test_bit(IPOIB_MCAST_FLAG_BUSY,
&next_mcast->flags)
+ && !test_bit(IPOIB_MCAST_FLAG_ATTACHED,
&next_mcast->flags))
+ break;
+ }
+ if (&next_mcast->list != &priv->multicast_list)
+ queue_delayed_work(ipoib_workqueue, &priv->mcast_task,
+ next_mcast->backoff * HZ);
+ }
spin_unlock_irq(&priv->lock);
mutex_unlock(&mcast_mutex);
@@ -570,13 +579,16 @@ void ipoib_mcast_join_task(struct work_struct *work)
break;
}
}
- spin_unlock_irq(&priv->lock);
if (&mcast->list == &priv->multicast_list) {
/* All done */
+ spin_unlock_irq(&priv->lock);
break;
}
+ list_move_tail(&mcast->list, &priv->multicast_list);
+ spin_unlock_irq(&priv->lock);
+
ipoib_mcast_join(dev, mcast, 1);
return;
}
___
general mailing list
[email protected]
http://lists.openfabrics.org/cgi-bin/mailman/listinfo/general
To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general
___
general mailing list
[email protected]
http://lists.openfabrics.org/cgi-bin/mailman/listinfo/general
To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general
