Re: [PATCH 09/22] IB/ipoib: fix IPOIB_MCAST_RUN flag usage

[Erez Shitrit <erezsh-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org>]

On 2/12/2015 3:43 AM, Doug Ledford wrote:
> The usage of IPOIB_MCAST_RUN as a flag is inconsistent.  In some places
> it is used to mean "our device is administratively allowed to send
> multicast joins/leaves/packets" and in other places it means "our
> multicast join task thread is currently running and will process your
> request if you put it on the queue".  However, this latter meaning is in
> fact flawed as there is a race condition between the join task testing
> the mcast list and finding it empty of remaining work, dropping the
> mcast mutex and also the priv->lock spinlock, and clearing the
> IPOIB_MCAST_RUN flag.  Further, there are numerous locations that use
> the flag in the former fashion, and when all tasks complete and the task
> thread clears the RUN flag, all of those other locations will fail to
> ever again queue any work.  This results in the interface coming up fine
> initially, but having problems adding new multicast groups after the
> first round of groups have all been added and the RUN flag is cleared by
> the join task thread when it thinks it is done.  To resolve this issue,
> convert all locations in the code to treat the RUN flag as an indicator
> that the multicast portion of this interface is in fact administratively
> up and joins/leaves/sends can be performed.  There is no harm (other
> than a slight performance penalty) to never clearing this flag and using
> it in this fashion as it simply means that a few places that used to
> micro-optimize how often this task was queued on a work queue will now
> queue the task a few extra times.  We can address that suboptimal
> behavior in future patches.
>
> Signed-off-by: Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>   drivers/infiniband/ulp/ipoib/ipoib_multicast.c | 11 +++++------
>   1 file changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/infiniband/ulp/ipoib/ipoib_multicast.c b/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
> index bc50dd0d0e4..91b8fe118ec 100644
> --- a/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
> +++ b/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
> @@ -630,8 +630,6 @@ void ipoib_mcast_join_task(struct work_struct *work)
>   	}
>   
>   	ipoib_dbg_mcast(priv, "successfully joined all multicast groups\n");
> -
> -	clear_bit(IPOIB_MCAST_RUN, &priv->flags);
>   }
>   
>   int ipoib_mcast_start_thread(struct net_device *dev)
> @@ -641,8 +639,8 @@ int ipoib_mcast_start_thread(struct net_device *dev)
>   	ipoib_dbg_mcast(priv, "starting multicast thread\n");
>   
>   	mutex_lock(&mcast_mutex);
> -	if (!test_and_set_bit(IPOIB_MCAST_RUN, &priv->flags))
> -		queue_delayed_work(priv->wq, &priv->mcast_task, 0);
> +	set_bit(IPOIB_MCAST_RUN, &priv->flags);

Hi Doug,
Can you use IPOIB_FLAG_OPER_UP instead of IPOIB_MCAST_RUN, in all these 
places and get rid of that RUN bit?

> +	queue_delayed_work(priv->wq, &priv->mcast_task, 0);
>   	mutex_unlock(&mcast_mutex);
>   
>   	return 0;
> @@ -725,7 +723,7 @@ void ipoib_mcast_send(struct net_device *dev, u8 *daddr, struct sk_buff *skb)
>   		memcpy(mcast->mcmember.mgid.raw, mgid, sizeof (union ib_gid));
>   		__ipoib_mcast_add(dev, mcast);
>   		list_add_tail(&mcast->list, &priv->multicast_list);
> -		if (!test_and_set_bit(IPOIB_MCAST_RUN, &priv->flags))
> +		if (test_bit(IPOIB_MCAST_RUN, &priv->flags))
>   			queue_delayed_work(priv->wq, &priv->mcast_task, 0);
>   	}
>   
> @@ -951,7 +949,8 @@ void ipoib_mcast_restart_task(struct work_struct *work)
>   	/*
>   	 * Restart our join task if needed
>   	 */
> -	ipoib_mcast_start_thread(dev);
> +	if (test_bit(IPOIB_MCAST_RUN, &priv->flags))
> +		queue_delayed_work(priv->wq, &priv->mcast_task, 0);
>   	rtnl_unlock();
>   }
>   

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html