***SPAM*** Re: [ofa-general] [PATCH] IB/IPoIB: Decrease the time that invalid paths stay useless

[Yossi Etigin]

Looks OK to me

Moni Shoua wrote:
> Yossi Etigin wrote:
>>> @@ -360,12 +360,15 @@ void ipoib_mark_paths_invalid(struct net_device
>>> *dev)
>>>      spin_lock_irq(&priv->lock);
>>>  
>>>      list_for_each_entry_safe(path, tp, &priv->path_list, list) {
>>> -        ipoib_dbg(priv, "mark path LID 0x%04x GID " IPOIB_GID_FMT "
>>> invalid\n",
>>> +        ipoib_dbg(priv, "mark path LID 0x%04x GID " IPOIB_GID_FMT "
>>> stale\n",
>>>              be16_to_cpu(path->pathrec.dlid),
>>>              IPOIB_GID_ARG(path->pathrec.dgid));
>>> -        path->valid =  0;
>>> +        path->stale =  1;
>>>      }
>>>  
>>> +    if (!list_empty(&priv->path_list))
>>> +        queue_delayed_work(ipoib_workqueue, &priv->path_refresh_task,
>>> +                    round_jiffies_relative(HZ));
>>>      spin_unlock_irq(&priv->lock);
>>>  }
>>>  
>> What if there is already an outstanding path query on one
>> of the paths you mark stale? ipoib_refresh_paths() will issue another
>> query, making it two
>> queries on the same path. Then, if you bring the device
>> down (call ipoib_flush_paths()) it will wait for completion
>> of one query, causing a crash.
>> _______________________________________________
>> 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
>>
> Thanks. You're right. I think that the change to the patch below should fix what you pointed at
> Do you agree?
> 
> @@ -551,9 +557,29 @@ static int path_rec_start(struct net_device *dev,
>                 return path->query_id;
>         }
> 
> +       path->stale =  0;
>         return 0;
>  }
> 
> +void ipoib_refresh_paths(struct work_struct *work)
> +{
> +       struct ipoib_dev_priv *priv =
> +               container_of(work, struct ipoib_dev_priv, path_refresh_task.work);
> +       struct net_device *dev = priv->dev;
> +       struct ipoib_path *path, *tp;
> +
> +       spin_lock_irq(&priv->lock);
> +       list_for_each_entry_safe(path, tp, &priv->path_list, list) {
> +               ipoib_dbg(priv, "restart path LID 0x%04x GID " IPOIB_GID_FMT "\n",
> +                       be16_to_cpu(path->pathrec.dlid),
> +                       IPOIB_GID_ARG(path->pathrec.dgid));
> +               if (path->stale && !path->query)                 <<<<<<<<<<<<<<<<  CHANGE IS HERE
> +                       path_rec_start(dev, path);
> +       }
> +
> +       spin_unlock_irq(&priv->lock);
> +}
> +
>  static void neigh_add_path(struct sk_buff *skb, struct net_device *dev)
>  {
>         struct ipoib_dev_priv *priv = netdev_priv(dev);
> 
>