[ofa-general] [PATCH V2] IB/ipoib: refresh paths instead of fushing them on SM change event
Moni Shoua
monis at Voltaire.COM
Tue Jul 1 07:36:34 PDT 2008
Thanks Roland,
I refered to your comment below. I will resend V3 with hope that my answers
are acceptable and that there are no more issues left.
Roland Dreier wrote:
> I wanted to apply this but I see some locking issues;
>
> > @@ -409,6 +427,9 @@ static void path_rec_completion(int status,
> >
> > spin_lock_irqsave(&priv->lock, flags);
> >
> > + if (path->ah)
> > + ipoib_put_ah(path->ah);
>
> Look at what ipoib_free_ah() does if this drops the last reference to
> the ah (hint -- it takes priv->lock). Is there any guarantee that there
> are more references to this ah still held? If so there needs to be a
> comment explaining this (as other places in ipoib_main.c have).
if path->ah is not NULL then it means that this is not the first
visit in path_rec_completion() for this path and we are sure
that there is at least one more reference held by a neighbour
in this path.
I added this comment to the code.
>
> > list_for_each_entry_safe(neigh, tn, &path->neigh_list, list) {
> > + if (neigh->ah)
> > + old_ah = neigh->ah;
>
> is there any guarantee that this list has only one entry? If so why?
You are right, there isn't but it seems easy to fix. I don't need to remember all
old_ah's but only one that might be the last one that holds the reference and put it
outside the lock.
I changed this to
list_for_each_entry_safe(neigh, tn, &path->neigh_list, list) {
+ if (neigh->ah) {
+ /*
+ * We don't need to remember all old_ah's but
+ * only the last one that may hold the last
+ * reference
+ */
+ if (old_ah)
+ ipoib_put_ah(old_ah);
+ old_ah = neigh->ah;
+ }
>
> - R.
> _______________________________________________
> general mailing list
> general at lists.openfabrics.org
> http://lists.openfabrics.org/cgi-bin/mailman/listinfo/general
>
> To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general
>
More information about the general
mailing list