On Mon, Jul 9, 2012 at 12:43 PM, Johannes Berg <johannes@xxxxxxxxxxxxxxxx> wrote: > On Mon, 2012-07-09 at 12:39 +0300, Arik Nemtsov wrote: > >> >> >> >> I noticed no synchronize_rcu() in the start/stop scan calls. Good/bad idea? >> >> >> > >> >> >> > Well, start() certainly wouldn't need it since you'd only get NULL :-) >> >> >> > >> >> >> > stop() in theory could use it, but it doesn't actually matter because as >> >> >> > long as the interface still exists the pointer is valid. We don't free >> >> >> > the interface in scan stop, so we don't need to make sure that the >> >> >> > pointer is cleared before we continue. And in the case that we *do* in >> >> >> > fact clear the interface (when it's going down) we have synchronize_rcu >> >> >> > already in those code paths due to say the interface list with RCU >> >> >> > protection. >> >> >> >> >> >> I meant protecting these (in patch 2/3): >> >> >> >> >> >> - local->sched_scanning, >> >> >> + rcu_dereference_protected(local->sched_scan_sdata, >> >> >> + lockdep_is_held(&local->mtx)), >> >> >> >> >> >> The check is obviously racy here, but it was racy before as well I guess. >> >> >> I'm not sure why something line test_bit(SCHED_SCANNING) wasn't used >> >> >> in these places. >> >> > >> >> > I don't think I understand what you're trying to say ... why is this >> >> > racy? We hold the mutex that we always hold when we assign the pointer. >> >> >> >> I mean this check in ieee80211_rx_h_passive_scan(): >> >> >> >> if (test_bit(SCAN_HW_SCANNING, &local->scanning) || >> >> test_bit(SCAN_SW_SCANNING, &local->scanning) || >> >> test_bit(SCAN_ONCHANNEL_SCANNING, &local->scanning) || >> >> local->sched_scanning) >> >> return ieee80211_scan_rx(rx->sdata, skb); >> >> >> >> since this is RCU, the pointer might be there a while longer after the >> >> scan finished.. >> > >> > Oh. I was looking at the code after patch 3 and this no longer >> > exists ;-) >> > >> > But then my first argument applies -- as long as the interface is there, >> > the pointer is OK, and when the interface is removed we need to remove >> > it from the RCU-managed interface list so need to synchronize_rcu() >> > already. No? >> >> The add/remove interface part is covered, yes. >> >> What happens when starting/stopping sched scan? The rcu pointer is >> removed in ieee80211_request_sched_scan_stop(), but we may still think >> we are sched scanning for a while inside >> ieee80211_rx_h_passive_scan(). >> >> Probably nothing too bad will happen though.. > > Yes, well... Say we stick synchronize_rcu() into sched_scan_stop(). What > would actually change? > We'd still think we're sched scanning (in the RX handler) for exactly > the same amount of time, except the sched scan stop code would now wait > for it, while doing nothing. It has nothing to do after the wait (since > it doesn't free the RCU data or anything). I was referring to using test_bit(SCHED_SCANNING). You're right syncronize_rcu() doesn't do anything here (I even realized my mistake somewhere along the email chain I think). Arik -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
