Hi,
So I applied patches 1 and 2, but have some comments/questions here.
> static struct sk_buff *
> __ieee80211_beacon_get(struct ieee80211_hw *hw,
> struct ieee80211_vif *vif,
> struct ieee80211_mutable_offsets *offs,
> - bool is_template)
> + bool is_template,
> + int ema_index)
> {
> struct ieee80211_local *local = hw_to_local(hw);
> struct beacon_data *beacon = NULL;
> @@ -4995,13 +5038,11 @@ __ieee80211_beacon_get(struct ieee80211_hw *hw,
> struct ieee80211_chanctx_conf *chanctx_conf;
> int csa_off_base = 0;
>
>
>
>
> - rcu_read_lock();
Why are you making this change that moves the RCU locking out? In two
out of three places, you now have to
rcu_read_lock();
__ieee80211_beacon_get(...);
rcu_read_unlock();
Not sure I see the point?
>
> if (!ieee80211_sdata_running(sdata) || !chanctx_conf)
> - goto out;
> + return NULL;
That also causes a lot of collateral changes that make it harder to read
than it needs to be.
Maybe split the locking change to a separate patch, if at all necessary?
(but see below)
> + rcu_read_lock();
> ...
> + ema = kmalloc(sizeof(*ema), GFP_KERNEL);
This is obviously wrong - you should add a few more debug options to
your test kernels :-)
> + if (!ema) {
> + ieee80211_beacon_free_ema_list(head);
> + cnt = 0;
> + goto out;
break would be enough, and you could save the 'out' label?
> + }
> +
> + ema->skb = __ieee80211_beacon_get(hw, vif, &ema->offs, true,
> + cnt);
> + if (!ema->skb) {
> + kfree(ema);
> + break;
> + }
> + list_add_tail(&ema->list, head);
> + cnt++;
> + }
> +out:
> + rcu_read_unlock();
This is also the only place that uses the rcu_read_lock()/unlock() not
directly around the __ieee80211_beacon_get(), but there's absolutely no
point in that?
I think what you intended here was correct, but it was done incorrectly.
Really what it seems you (should have) _wanted_ with the RCU locking
here is that the
beacon = rcu_dereference(ap->beacon);
is only done *once*, so that you can really get a snapshot of *all*
those MBSSID beacons, even if ap->beacon changed.
However, that's not what you implemented. By moving the rcu_read_lock()
outside, you've achieved nothing, since you still do a dereference
inside, and it can change.
It seems to me that to do this correctly you have to actually split
__ieee80211_beacon_get() and do the dereference *outside* of it, then
pass the 'beacon' pointer into it. Then, you can here do the dereference
outside of the loop too (and in fact can do a much nicer for loop rather
than the infinite loop).
As it is, you've penalized the other cases (they now need to do the
rcu_read_lock() manually), but haven't actually gained anything
(functionally, at least, you save a few cycles by having the RCU stuff
outside the loop, but that doesn't really count).
johannes
