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

[Eliad Peller <eliad@wizery.com>]

On Thu, Sep 6, 2012 at 6:41 PM, Johannes Berg <johannes@sipsolutions.net> wrote:
> 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 wrongly assumed synchronize_rcu() takes care of call_rcu() callbacks
as well. thanks for clarifying this.

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

>> +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().
>
sure.

>> +     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.
>
i guess that because we run on an ordered workqueue we can't really
deadlock, but sure, i'll change it.

thanks for the review.
Eliad.