| Message ID | 20170522160946.2057-1-enric.balletbo@collabora.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Johannes Berg |
| Headers | show |
On Mon, 2017-05-22 at 18:09 +0200, Enric Balletbo i Serra wrote: > The IEEE80211_SCAN_RESULT_EXPIRE value was modified several times in > the > past. Initially was set at 10 seconds (2a51931192), then increased at > 15 > seconds (09f97e0fc4) and finally to 30 seconds (f9616e0f88) to cover > the > use case when a station is having heavy uplink traffic. On some > devices, > like Chromebooks, this value is decreased to 7 seconds to avoid stall > results, and other devices prefer set to 15 seconds. > > This simple patch tries to make the selection of this value a bit > more > flexible by being able to set the expire time at config stage. Most > users > can leave the default value set as 30 seconds, others can modify the > value > at config stage if they want lower or bigger values. I'm not really all that convinced that we really need this - userspace should just be using the flush thing more often, and then it doesn't really matter. However, maybe that doesn't really matter all that much. But, > +config CFG80211_SCAN_RESULT_EXPIRE > + int "Scan completion time" if CFG80211 That should have "if CFG80211 && EXPERT" or something like that - no need to prompt everyone for it. > + rdev->scan_result_expire = > CONFIG_CFG80211_SCAN_RESULT_EXPIRE; This is completely pointless - no need to go through runtime like that. > -#define IEEE80211_SCAN_RESULT_EXPIRE (30 * HZ) You can just use CONFIG_CFG80211_SCAN_RESULT_EXPIRE * HZ here. > + __cfg80211_bss_expire(rdev, jiffies - rdev- > >scan_result_expire); You also completely messed this up because it now depends on HZ. johannes
On Mon, 2017-05-22 at 18:09 +0200, Enric Balletbo i Serra wrote: > The IEEE80211_SCAN_RESULT_EXPIRE value was modified several times in > the > past. Initially was set at 10 seconds (2a51931192), then increased at > 15 > seconds (09f97e0fc4) and finally to 30 seconds (f9616e0f88) to cover > the > use case when a station is having heavy uplink traffic. On some > devices, > like Chromebooks, this value is decreased to 7 seconds to avoid stall > results, and other devices prefer set to 15 seconds. Couldn't userspace just look at NL80211_BSS_SEEN_MS_AGO to filter and create its own list? Given that the kernel provides the information userspace needs to figure out the age of a particular BSS, it doesn't seem like there needs to be a kernel tunable for this. Userspace can already avoid stale results. Also, different runtime situations might want different result ages, which wouldn't be possible if the kernel had a hardcoded maximum. Furthermore, different userspace apps might be reading the same scan list, and they might have different ideas about staleness. Or perhaps I misunderstand the problem, which could well be the case. Dan > This simple patch tries to make the selection of this value a bit > more > flexible by being able to set the expire time at config stage. Most > users > can leave the default value set as 30 seconds, others can modify the > value > at config stage if they want lower or bigger values. > > Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com> > --- > net/wireless/Kconfig | 11 +++++++++++ > net/wireless/core.c | 2 ++ > net/wireless/core.h | 1 + > net/wireless/scan.c | 6 ++---- > 4 files changed, 16 insertions(+), 4 deletions(-) > > diff --git a/net/wireless/Kconfig b/net/wireless/Kconfig > index 6c60612..2a54e28 100644 > --- a/net/wireless/Kconfig > +++ b/net/wireless/Kconfig > @@ -29,6 +29,17 @@ config CFG80211 > > When built as a module it will be called cfg80211. > > +config CFG80211_SCAN_RESULT_EXPIRE > + int "Scan completion time" if CFG80211 > + default 30 > + ---help--- > + This value is the time in seconds the mac80211 scan method > needs > + to finish. An expiration time to 30 seconds should be > sufficient > + for most cases but for some other cases we might be > interested in > + tweak this value. > + > + If unsure, leave the default of 30. > + > config NL80211_TESTMODE > bool "nl80211 testmode command" > depends on CFG80211 > diff --git a/net/wireless/core.c b/net/wireless/core.c > index 83ea164..2a5742c 100644 > --- a/net/wireless/core.c > +++ b/net/wireless/core.c > @@ -499,6 +499,8 @@ struct wiphy *wiphy_new_nm(const struct > cfg80211_ops *ops, int sizeof_priv, > > init_waitqueue_head(&rdev->dev_wait); > > + rdev->scan_result_expire = > CONFIG_CFG80211_SCAN_RESULT_EXPIRE; > + > /* > * Initialize wiphy parameters to IEEE 802.11 MIB default > values. > * Fragmentation and RTS threshold are disabled by default > with the > diff --git a/net/wireless/core.h b/net/wireless/core.h > index 6e809325..6a8a164 100644 > --- a/net/wireless/core.h > +++ b/net/wireless/core.h > @@ -72,6 +72,7 @@ struct cfg80211_registered_device { > struct rb_root bss_tree; > u32 bss_generation; > u32 bss_entries; > + unsigned int scan_result_expire; > struct cfg80211_scan_request *scan_req; /* protected by RTNL > */ > struct sk_buff *scan_msg; > struct list_head sched_scan_req_list; > diff --git a/net/wireless/scan.c b/net/wireless/scan.c > index 14d5f0c..f0b55a9 100644 > --- a/net/wireless/scan.c > +++ b/net/wireless/scan.c > @@ -70,8 +70,6 @@ module_param(bss_entries_limit, int, 0644); > MODULE_PARM_DESC(bss_entries_limit, > "limit to number of scan BSS entries (per wiphy, > default 1000)"); > > -#define IEEE80211_SCAN_RESULT_EXPIRE (30 * HZ) > - > static void bss_free(struct cfg80211_internal_bss *bss) > { > struct cfg80211_bss_ies *ies; > @@ -476,7 +474,7 @@ void cfg80211_bss_age(struct > cfg80211_registered_device *rdev, > > void cfg80211_bss_expire(struct cfg80211_registered_device *rdev) > { > - __cfg80211_bss_expire(rdev, jiffies - > IEEE80211_SCAN_RESULT_EXPIRE); > + __cfg80211_bss_expire(rdev, jiffies - rdev- > >scan_result_expire); > } > > const u8 *cfg80211_find_ie_match(u8 eid, const u8 *ies, int len, > @@ -737,7 +735,7 @@ struct cfg80211_bss *cfg80211_get_bss(struct > wiphy *wiphy, > if (!is_valid_ether_addr(bss->pub.bssid)) > continue; > /* Don't get expired BSS structs */ > - if (time_after(now, bss->ts + > IEEE80211_SCAN_RESULT_EXPIRE) && > + if (time_after(now, bss->ts + rdev- > >scan_result_expire) && > !atomic_read(&bss->hold)) > continue; > if (is_bss(&bss->pub, bssid, ssid, ssid_len)) {
> Couldn't userspace just look at NL80211_BSS_SEEN_MS_AGO to filter and > create its own list? Given that the kernel provides the information > userspace needs to figure out the age of a particular BSS, it doesn't > seem like there needs to be a kernel tunable for this. Userspace can > already avoid stale results. Yeah, I agree. It can also ask for a flush, so that old results are gone by the time the next scan returns. We don't have a flush operation without requesting a new scan, but I guess that could be added. > Also, different runtime situations might want different result ages, > which wouldn't be possible if the kernel had a hardcoded maximum. > Furthermore, different userspace apps might be reading the same scan > list, and they might have different ideas about staleness. > > Or perhaps I misunderstand the problem, which could well be the case. No, I think this is perfectly right - userspace should be able to deal with this given the tools we gave it, or if not, we should probably just give it more tools instead of hardcoding the kernel configuration. This value really just kinda needed to be an upper bound so that we don't start expiring entries while we're still scanning. johannes
On Mon, 2017-05-22 at 18:19 +0200, Johannes Berg wrote: > > I'm not really all that convinced that we really need this - > userspace should just be using the flush thing more often, and then > it doesn't really matter. > > However, maybe that doesn't really matter all that much. Actually, I changed my mind based on the discussion with Dan. This makes no sense, and since you've obviously never even tested this patch, you probably haven't thought about alternatives either. johannes
Hi, On 22/05/17 18:24, Johannes Berg wrote: > >> Couldn't userspace just look at NL80211_BSS_SEEN_MS_AGO to filter and >> create its own list? Given that the kernel provides the information >> userspace needs to figure out the age of a particular BSS, it doesn't >> seem like there needs to be a kernel tunable for this. Userspace can >> already avoid stale results. > > Yeah, I agree. It can also ask for a flush, so that old results are > gone by the time the next scan returns. We don't have a flush operation > without requesting a new scan, but I guess that could be added. > Ok, guess I understand what you're saying. Thanks for pointing me in the right direction. >> Also, different runtime situations might want different result ages, >> which wouldn't be possible if the kernel had a hardcoded maximum. >> Furthermore, different userspace apps might be reading the same scan >> list, and they might have different ideas about staleness. >> >> Or perhaps I misunderstand the problem, which could well be the case. > > No, I think this is perfectly right - userspace should be able to deal > with this given the tools we gave it, or if not, we should probably > just give it more tools instead of hardcoding the kernel configuration. > > This value really just kinda needed to be an upper bound so that we > don't start expiring entries while we're still scanning. > > johannes >
diff --git a/net/wireless/Kconfig b/net/wireless/Kconfig index 6c60612..2a54e28 100644 --- a/net/wireless/Kconfig +++ b/net/wireless/Kconfig @@ -29,6 +29,17 @@ config CFG80211 When built as a module it will be called cfg80211. +config CFG80211_SCAN_RESULT_EXPIRE + int "Scan completion time" if CFG80211 + default 30 + ---help--- + This value is the time in seconds the mac80211 scan method needs + to finish. An expiration time to 30 seconds should be sufficient + for most cases but for some other cases we might be interested in + tweak this value. + + If unsure, leave the default of 30. + config NL80211_TESTMODE bool "nl80211 testmode command" depends on CFG80211 diff --git a/net/wireless/core.c b/net/wireless/core.c index 83ea164..2a5742c 100644 --- a/net/wireless/core.c +++ b/net/wireless/core.c @@ -499,6 +499,8 @@ struct wiphy *wiphy_new_nm(const struct cfg80211_ops *ops, int sizeof_priv, init_waitqueue_head(&rdev->dev_wait); + rdev->scan_result_expire = CONFIG_CFG80211_SCAN_RESULT_EXPIRE; + /* * Initialize wiphy parameters to IEEE 802.11 MIB default values. * Fragmentation and RTS threshold are disabled by default with the diff --git a/net/wireless/core.h b/net/wireless/core.h index 6e809325..6a8a164 100644 --- a/net/wireless/core.h +++ b/net/wireless/core.h @@ -72,6 +72,7 @@ struct cfg80211_registered_device { struct rb_root bss_tree; u32 bss_generation; u32 bss_entries; + unsigned int scan_result_expire; struct cfg80211_scan_request *scan_req; /* protected by RTNL */ struct sk_buff *scan_msg; struct list_head sched_scan_req_list; diff --git a/net/wireless/scan.c b/net/wireless/scan.c index 14d5f0c..f0b55a9 100644 --- a/net/wireless/scan.c +++ b/net/wireless/scan.c @@ -70,8 +70,6 @@ module_param(bss_entries_limit, int, 0644); MODULE_PARM_DESC(bss_entries_limit, "limit to number of scan BSS entries (per wiphy, default 1000)"); -#define IEEE80211_SCAN_RESULT_EXPIRE (30 * HZ) - static void bss_free(struct cfg80211_internal_bss *bss) { struct cfg80211_bss_ies *ies; @@ -476,7 +474,7 @@ void cfg80211_bss_age(struct cfg80211_registered_device *rdev, void cfg80211_bss_expire(struct cfg80211_registered_device *rdev) { - __cfg80211_bss_expire(rdev, jiffies - IEEE80211_SCAN_RESULT_EXPIRE); + __cfg80211_bss_expire(rdev, jiffies - rdev->scan_result_expire); } const u8 *cfg80211_find_ie_match(u8 eid, const u8 *ies, int len, @@ -737,7 +735,7 @@ struct cfg80211_bss *cfg80211_get_bss(struct wiphy *wiphy, if (!is_valid_ether_addr(bss->pub.bssid)) continue; /* Don't get expired BSS structs */ - if (time_after(now, bss->ts + IEEE80211_SCAN_RESULT_EXPIRE) && + if (time_after(now, bss->ts + rdev->scan_result_expire) && !atomic_read(&bss->hold)) continue; if (is_bss(&bss->pub, bssid, ssid, ssid_len)) {
The IEEE80211_SCAN_RESULT_EXPIRE value was modified several times in the past. Initially was set at 10 seconds (2a51931192), then increased at 15 seconds (09f97e0fc4) and finally to 30 seconds (f9616e0f88) to cover the use case when a station is having heavy uplink traffic. On some devices, like Chromebooks, this value is decreased to 7 seconds to avoid stall results, and other devices prefer set to 15 seconds. This simple patch tries to make the selection of this value a bit more flexible by being able to set the expire time at config stage. Most users can leave the default value set as 30 seconds, others can modify the value at config stage if they want lower or bigger values. Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com> --- net/wireless/Kconfig | 11 +++++++++++ net/wireless/core.c | 2 ++ net/wireless/core.h | 1 + net/wireless/scan.c | 6 ++---- 4 files changed, 16 insertions(+), 4 deletions(-)