[ofa-general] [PATCH V2] IB/ipoib: refresh paths instead of fushing them on SM change event

[Moni Shoua]

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
>