Re: [PATCH 2/2] mac80211: use call_rcu() on sta deletion

[Johannes Berg <johannes@sipsolutions.net>]

On Wed, 2012-09-05 at 20:23 +0300, Eliad Peller wrote:

> +++ b/net/mac80211/iface.c
> @@ -774,6 +774,10 @@ static void ieee80211_do_stop(struct ieee80211_sub_if_data *sdata,
>  		 * frames at this very time on another CPU.
>  		 */
>  		synchronize_rcu();
> +
> +		/* flush any remaining cleanup work */
> +		flush_workqueue(local->workqueue);

I have a feeling we need rcu_barrier() here instead of synchronize_rcu()
now to guarantee that all the potentially pending call_rcu() functions
from the sta_info_flush() above have actually completed and queued the
work items that we want to flush here. Right? That's a bit unfortunate,
but I guess the best way of handling it and ifdown should be rare anyway
compared to everything else.

I think it'd also be good to make the comment more verbose and explain
which cleanup work.

> +static void free_sta_work(struct work_struct *wk)
> +{
> +	struct sta_info *sta = container_of(wk, struct sta_info, free_sta_wk);
> +	int ac, i;
> +	struct tid_ampdu_tx *tid_tx;
> +	struct ieee80211_sub_if_data *sdata = sta->sdata;
> +	struct ieee80211_local *local = sdata->local;
> +
> +	/*
> +	 * At this point, after we wait for an RCU grace period,
> +	 * neither mac80211 nor the driver can reference this
> +	 * sta struct any more except by still existing timers
> +	 * associated with this station that we clean up below.
> +	 */

Should probably rewrite the comment a bit to indicate that we have in
fact waited since we get called via call_rcu().

> +	cancel_work_sync(&sta->drv_unblock_wk);

This will deadlock now, but cancel_work() will be safe without the sync
since the workqueue is single-threaded.

johannes