| Message ID | 1553884287-23430-1-git-send-email-greearb@candelatech.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | supplicant: Tune auto-roaming based on scan results, allow configuring thresholds. | expand |
On Fri, Mar 29, 2019 at 11:31:27AM -0700, greearb@candelatech.com wrote: > While trying to force an .11r roam, I noticed that when I scanned, the station > went back to the original AP. The RSSI was around -25 for one and -35 for > the other, but the RSSI threshold was 5, so it flipped. > > I believe that the performance difference between RSSI 25 and 35 is negligible, > so I modified the algorithm to basically not auto-roam if both signals are > good. I'm not certain my new automatic thresholds are perfect, so maybe > they need more tuning. At the least, we should not auto roam between an RSSI 25 > and RSSI 35 AP based on RSSI. > > And, allow user overrides in case we want the thresholds even higher. > > In addition, add a note regarding the estimated bandwidth calculation, > as it does not appear to take NSS into account. I don't want to expose this type of very specific parameters in configuration file since this can result in expectation of those values being maintained in the future and that resulting in additional constraints on the implementation. If something needs to be configurable, I'd prefer to consider some higher level value to indicate how aggressive roaming should be and then automatically convert from that to internal parameters for the algorithm so that the design can be changed without having to touch the definition of the configuration parameter. > diff --git a/wpa_supplicant/events.c b/wpa_supplicant/events.c > @@ -1778,8 +1784,17 @@ static int wpa_supplicant_need_to_roam(struct wpa_supplicant *wpa_s, > min_diff = 3; > else if (current_bss->level < -70) > min_diff = 4; > - else > + else if (current_bss->level < -60) > min_diff = 5; > + else if (current_bss->level < -50) > + min_diff = 8; > + else if (current_bss->level < -40) > + min_diff = 10; > + else if (current_bss->level < -30) > + min_diff = 20; > + else > + min_diff = 30; I'm not sure this is really needed. There is now a cur_snr > GREAT_SNR check that will in practice skip roaming in cases where the signal level from the current BSS is sufficiently good. > diff --git a/wpa_supplicant/scan.c b/wpa_supplicant/scan.c > @@ -2383,6 +2383,7 @@ void scan_est_throughput(struct wpa_supplicant *wpa_s, > + /* TODO: Take number of spatial streams into account */ > /* TODO: channel utilization and AP load (e.g., from AP Beacon) */ I contribution to add such functionality and updates for HE related items in this area would be welcome..
On 01/02/2020 12:38 AM, Jouni Malinen wrote: > On Fri, Mar 29, 2019 at 11:31:27AM -0700, greearb@candelatech.com wrote: >> While trying to force an .11r roam, I noticed that when I scanned, the station >> went back to the original AP. The RSSI was around -25 for one and -35 for >> the other, but the RSSI threshold was 5, so it flipped. >> >> I believe that the performance difference between RSSI 25 and 35 is negligible, >> so I modified the algorithm to basically not auto-roam if both signals are >> good. I'm not certain my new automatic thresholds are perfect, so maybe >> they need more tuning. At the least, we should not auto roam between an RSSI 25 >> and RSSI 35 AP based on RSSI. >> >> And, allow user overrides in case we want the thresholds even higher. >> >> In addition, add a note regarding the estimated bandwidth calculation, >> as it does not appear to take NSS into account. > > I don't want to expose this type of very specific parameters in > configuration file since this can result in expectation of those values > being maintained in the future and that resulting in additional > constraints on the implementation. If something needs to be > configurable, I'd prefer to consider some higher level value to indicate > how aggressive roaming should be and then automatically convert from > that to internal parameters for the algorithm so that the design can be > changed without having to touch the definition of the configuration > parameter. I'm guessing the vast majority of users never touch the config file themselves...it is auto-generated by something at a higher level. Maybe let the higher level thing deal with the simplified setting value and let it tune the more specific values in supplicant? >> diff --git a/wpa_supplicant/events.c b/wpa_supplicant/events.c >> @@ -1778,8 +1784,17 @@ static int wpa_supplicant_need_to_roam(struct wpa_supplicant *wpa_s, >> min_diff = 3; >> else if (current_bss->level < -70) >> min_diff = 4; >> - else >> + else if (current_bss->level < -60) >> min_diff = 5; >> + else if (current_bss->level < -50) >> + min_diff = 8; >> + else if (current_bss->level < -40) >> + min_diff = 10; >> + else if (current_bss->level < -30) >> + min_diff = 20; >> + else >> + min_diff = 30; > > I'm not sure this is really needed. There is now a cur_snr > GREAT_SNR > check that will in practice skip roaming in cases where the signal level > from the current BSS is sufficiently good. Based on my current understanding of things, I'd change the logic above to keep the min_diff larger even at very weak signals since roaming is so disruptive to user experience, and RSSI is likely to fluctuate more than a few db. Does the current code keep the station from roaming back and forth when there are two APs at about -70? Thanks, Ben > >> diff --git a/wpa_supplicant/scan.c b/wpa_supplicant/scan.c >> @@ -2383,6 +2383,7 @@ void scan_est_throughput(struct wpa_supplicant *wpa_s, >> + /* TODO: Take number of spatial streams into account */ >> /* TODO: channel utilization and AP load (e.g., from AP Beacon) */ > > I contribution to add such functionality and updates for HE related > items in this area would be welcome.. >
On Thu, Jan 02, 2020 at 07:41:42AM -0800, Ben Greear wrote: > On 01/02/2020 12:38 AM, Jouni Malinen wrote: > > I don't want to expose this type of very specific parameters in > > configuration file since this can result in expectation of those values > > being maintained in the future and that resulting in additional > > constraints on the implementation. If something needs to be > > configurable, I'd prefer to consider some higher level value to indicate > > how aggressive roaming should be and then automatically convert from > > that to internal parameters for the algorithm so that the design can be > > changed without having to touch the definition of the configuration > > parameter. > > I'm guessing the vast majority of users never touch the config file > themselves...it is auto-generated by something at a higher level. Maybe > let the higher level thing deal with the simplified setting value > and let it tune the more specific values in supplicant? I'd consider that "higher level thing" the same as "users", i.e., it is whatever is generating the configuration for wpa_supplicant. I don't want to expose configuration parameters for low level details if those details may change completely based on how the internal wpa_supplicant implementation is changed. > > > diff --git a/wpa_supplicant/events.c b/wpa_supplicant/events.c > > > @@ -1778,8 +1784,17 @@ static int wpa_supplicant_need_to_roam(struct wpa_supplicant *wpa_s, > > > min_diff = 3; > > > else if (current_bss->level < -70) > > > min_diff = 4; > > > - else > > > + else if (current_bss->level < -60) > > > min_diff = 5; > > > + else if (current_bss->level < -50) > > > + min_diff = 8; > > > + else if (current_bss->level < -40) > > > + min_diff = 10; > > > + else if (current_bss->level < -30) > > > + min_diff = 20; > > > + else > > > + min_diff = 30; > > > > I'm not sure this is really needed. There is now a cur_snr > GREAT_SNR > > check that will in practice skip roaming in cases where the signal level > > from the current BSS is sufficiently good. > > Based on my current understanding of things, I'd change the logic above > to keep the min_diff larger even at very weak signals since roaming is so > disruptive to user experience, and RSSI is likely to > fluctuate more than a few db. This is proposing changes for cur_level >= -60. GREAT_SNR = 25 which would mean that these min_diff = 8, 10, 20, or 30 cases would not really have any impact to the current behavior since GREAT_SNR overrides (skip roam) anyway. > Does the current code keep the station from roaming back and forth when there > are two APs at about -70? Roaming back and forth should be significantly reduced. Roaming to the one of these APs that have "significantly better" throughput estimate should happen in one direction. Signal level based allowance for roaming would depend on the noise floor since -70 would be pretty close to GREAT_SNR, but not necessarily above it in environment where noise floor is high.
diff --git a/wpa_supplicant/config.c b/wpa_supplicant/config.c index 41d71ba..b9728ef 100644 --- a/wpa_supplicant/config.c +++ b/wpa_supplicant/config.c @@ -4174,6 +4174,8 @@ struct wpa_config * wpa_config_alloc_empty(const char *ctrl_interface, config->bss_max_count = DEFAULT_BSS_MAX_COUNT; config->bss_expiration_age = DEFAULT_BSS_EXPIRATION_AGE; config->bss_expiration_scan_count = DEFAULT_BSS_EXPIRATION_SCAN_COUNT; + config->reassoc_throughput_level_th = DEFAULT_REASSOC_THROUGHPUT_THRESHOLD; + config->reassoc_rssi_level_th = DEFAULT_REASSOC_RSSI_THRESHOLD; config->max_num_sta = DEFAULT_MAX_NUM_STA; config->ap_isolate = DEFAULT_AP_ISOLATE; config->access_network_type = DEFAULT_ACCESS_NETWORK_TYPE; @@ -4972,6 +4974,8 @@ static const struct global_parse_data global_fields[] = { { INT(pmf), 0 }, { FUNC(sae_groups), 0 }, { INT(dtim_period), 0 }, + { INT(reassoc_throughput_level_th), 0 }, + { INT(reassoc_rssi_level_th), 0 }, { INT(beacon_int), 0 }, { FUNC(ap_vendor_elements), 0 }, { FUNC(probe_req_ie), 0 }, diff --git a/wpa_supplicant/config.h b/wpa_supplicant/config.h index 7694132..46a6bc9 100644 --- a/wpa_supplicant/config.h +++ b/wpa_supplicant/config.h @@ -31,6 +31,8 @@ #define DEFAULT_BSS_MAX_COUNT 200 #define DEFAULT_BSS_EXPIRATION_AGE 180 #define DEFAULT_BSS_EXPIRATION_SCAN_COUNT 2 +#define DEFAULT_REASSOC_THROUGHPUT_THRESHOLD 5000 +#define DEFAULT_REASSOC_RSSI_THRESHOLD 0 #define DEFAULT_MIN_SCAN_GAP 0 #define DEFAULT_MAX_ASSOC_PER_SCAN 25 #define DEFAULT_DISABLE_ESS_ROAMING 0 @@ -1259,6 +1261,19 @@ struct wpa_config { */ int dtim_period; + + /** + * The throughput difference at which a station will roam based on + * scan results. Set to 5000 for default behaviour. + */ + int reassoc_throughput_level_th; + + /** + * The minimum rssi level difference that can cause a roam based on + * scan results. Set to zero for default behaviour. + */ + int reassoc_rssi_level_th; + /** * beacon_int - Default Beacon interval in TU * diff --git a/wpa_supplicant/events.c b/wpa_supplicant/events.c index 808c4b2..6153b9a 100644 --- a/wpa_supplicant/events.c +++ b/wpa_supplicant/events.c @@ -1728,10 +1728,12 @@ static int wpa_supplicant_need_to_roam(struct wpa_supplicant *wpa_s, wpa_dbg(wpa_s, MSG_DEBUG, "Considering within-ESS reassociation"); wpa_dbg(wpa_s, MSG_DEBUG, "Current BSS: " MACSTR - " freq=%d level=%d snr=%d est_throughput=%u", + " freq=%d level=%d snr=%d est_throughput=%u throughput_threshold=%d min_rssi_threshold=%d", MAC2STR(current_bss->bssid), current_bss->freq, current_bss->level, - current_bss->snr, current_bss->est_throughput); + current_bss->snr, current_bss->est_throughput, + wpa_s->conf->reassoc_throughput_level_th, + wpa_s->conf->reassoc_rssi_level_th); wpa_dbg(wpa_s, MSG_DEBUG, "Selected BSS: " MACSTR " freq=%d level=%d snr=%d est_throughput=%u", MAC2STR(selected->bssid), selected->freq, selected->level, @@ -1745,9 +1747,11 @@ static int wpa_supplicant_need_to_roam(struct wpa_supplicant *wpa_s, return 1; } - if (selected->est_throughput > current_bss->est_throughput + 5000) { + if (selected->est_throughput > current_bss->est_throughput + wpa_s->conf->reassoc_throughput_level_th) { wpa_dbg(wpa_s, MSG_DEBUG, - "Allow reassociation - selected BSS has better estimated throughput"); + "Allow reassociation - selected BSS has better estimated throughput: %d vs %d, trigger-level: %d", + selected->est_throughput, current_bss->est_throughput, + wpa_s->conf->reassoc_throughput_level_th); return 1; } @@ -1760,9 +1764,11 @@ static int wpa_supplicant_need_to_roam(struct wpa_supplicant *wpa_s, return 0; } - if (current_bss->est_throughput > selected->est_throughput + 5000) { + if (current_bss->est_throughput > selected->est_throughput + wpa_s->conf->reassoc_throughput_level_th) { wpa_dbg(wpa_s, MSG_DEBUG, - "Skip roam - Current BSS has better estimated throughput"); + "Skip roam - Current BSS has good enough estimated throughput: %d vs %d, trigger-level: %d", + current_bss->est_throughput, selected->est_throughput, + wpa_s->conf->reassoc_throughput_level_th); return 0; } @@ -1778,8 +1784,17 @@ static int wpa_supplicant_need_to_roam(struct wpa_supplicant *wpa_s, min_diff = 3; else if (current_bss->level < -70) min_diff = 4; - else + else if (current_bss->level < -60) min_diff = 5; + else if (current_bss->level < -50) + min_diff = 8; + else if (current_bss->level < -40) + min_diff = 10; + else if (current_bss->level < -30) + min_diff = 20; + else + min_diff = 30; + if (cur_est > sel_est * 1.5) min_diff += 10; else if (cur_est > sel_est * 1.2) @@ -1805,6 +1820,11 @@ static int wpa_supplicant_need_to_roam(struct wpa_supplicant *wpa_s, else min_diff = 0; } + + /* Let user override this so we can force roaming and still scan. */ + if (wpa_s->conf->reassoc_rssi_level_th > min_diff) + min_diff = wpa_s->conf->reassoc_rssi_level_th; + diff = abs(current_bss->level - selected->level); if (diff < min_diff) { wpa_dbg(wpa_s, MSG_DEBUG, diff --git a/wpa_supplicant/scan.c b/wpa_supplicant/scan.c index 1442823..4fbbf1a 100644 --- a/wpa_supplicant/scan.c +++ b/wpa_supplicant/scan.c @@ -2383,6 +2383,7 @@ void scan_est_throughput(struct wpa_supplicant *wpa_s, } } + /* TODO: Take number of spatial streams into account */ /* TODO: channel utilization and AP load (e.g., from AP Beacon) */ res->est_throughput = est; diff --git a/wpa_supplicant/wpa_supplicant.conf b/wpa_supplicant/wpa_supplicant.conf index 957838d..fd6b89e 100644 --- a/wpa_supplicant/wpa_supplicant.conf +++ b/wpa_supplicant/wpa_supplicant.conf @@ -1415,6 +1415,15 @@ fast_reauth=1 # Beacon interval (default: 100 TU) #beacon_int=100 +# RSSI threshold for making automatic roam decisions based on RSSI +# The supplicant tries to make a good decision on this already, so the +# setting below sets the lower-level of the threshold, not the upper. +#reassoc_rssi_level_th=0 + +# Estimated throughput threshold for making automatic roam decisions based on RSSI +# The default is 5000. +#reassoc_throughput_level_th=5000 + # WPS in AP mode # 0 = WPS enabled and configured (default) # 1 = WPS disabled