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

[Moni Shoua]

Roland Dreier wrote:
> Please run your patches through scripts/checkpatch.pl and consider the
> output before sending them... it's easy to do and saves everyone time
> fixing trivial style mistakes.
> 
I did ran the script on the patch and besides one WARNING for 
a line that exceeds 80 columns which I believe is allowed sometimes.
> More substantive comments:
> 
>  > +static void __ipoib_ib_dev_flush(struct ipoib_dev_priv *priv, int level)
> 
> Instead of "int level" here, I would prefer to see something like
> 
> enum ipoib_flush_level {
> 	IPOIB_FLUSH_LIGHT,
> 	IPOIB_FLUSH_NORMAL,
> 	IPOIB_FLUSH_HEAVY,
> };
> 
> so that code like
> 
>  > -	if (pkey_event) {
>  > +	if (level == 2) {
> 
> remains at somewhat self-documenting.
OK
> 
>  > +static void path_refresh(struct net_device *dev, struct ipoib_path *path)
>  > +{
>  > +	struct ipoib_dev_priv *priv = netdev_priv(dev);
>  > +	ipoib_dbg(priv, "mark path LID 0x%04x GID " IPOIB_GID_FMT " invalid\n",
>  > +		  be16_to_cpu(path->pathrec.dlid), IPOIB_GID_ARG(path->pathrec.dgid));
>  > +	path->valid =  0;
>  > +}
> 
> I'm not sure this wrapper makes sense, because it is so trivial and it
> doesn't even do what the name suggests -- it doesn't refresh the path,
> it just marks it as invalid.
> 
I'll get rid of the wrapper. Please see below

> This looks broken for a couple of reasons:
> 
>  - What protects against paths being added to the path list while this
>    loop is running?  ipoib_flush_paths() moves the list to a new place
>    to avoid this problem.

Actually if I take out the spin_unlock() from inside of the loop I am
protected because adding a patch can only be started from the xmit
function that takes the tx_lock.
I let myself to give away spin_unlock because it was redundant from the start, we 
are only marking the path as invalid here.


>  - What happens if you mark a path as invalid and do
>    wait_for_completion(), and then ipoib_flush_paths() runs before
>    another path record query is done?  It seems that the second
>    wait_for_completion() will wait forever.
Again, you are right but wait_for_completion() is also not needed here so I let it go.

Finally,the function looks like this now (I also changed it's name to make
it more accurate)

+void ipoib_mark_paths_invalid(struct net_device *dev)
+{
+       struct ipoib_dev_priv *priv = netdev_priv(dev);
+       struct ipoib_path *path, *tp;
+       spin_lock_irq(&priv->tx_lock);
+       spin_lock(&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",
+                       be16_to_cpu(path->pathrec.dlid),
+                       IPOIB_GID_ARG(path->pathrec.dgid));
+               path->valid =  0;
+       }
+       spin_unlock(&priv->lock);
+       spin_unlock_irq(&priv->tx_lock);
+}
+

> 
>  - 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
>