| Message ID | 20180717175621.167407-1-matthewmwang@chromium.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | Improve roaming logic | expand |
Hi I'm experiencing aggressively roaming: > wpa_supplicant[9336]: wlp2s0: Current BSS: d8:38:fc:xx:xx:xx freq=2437 > level=-72 snr=17 est_throughput=48000 > wpa_supplicant[9336]: wlp2s0: Selected BSS: d8:38:fc:xx:xx:xx > freq=5260 level=-68 snr=24 est_throughput=65001 > wpa_supplicant[9336]: wlp2s0: Allow reassociation - selected BSS has > better estimated throughput A quick glance at the mailing list, I found this patch http://lists.infradead.org/pipermail/hostap/2018-July/038702.html, which I think is a better way to handle roaming. Was it forgotten or is there any particular reason it wasn't merged? - Kristian Klausen
On Tue, Jul 17, 2018 at 10:56:21AM -0700, Matthew Wang wrote: > Currently, wpa_supplicant roams too aggressively; the need_to_roam() > function will return early with a roaming decision if the difference > in signal level or throughput between the current and selected APs > is "sufficiently large." In particular, if the selected AP's > estimated throughput is more than 5k greater than the current AP's > estimated throughput, supplicant will decide to roam. Otherwise, if > the selected AP's signal level is less than the current AP's signal > level, or the selected AP's estimated throughput is at least 5k > less than the current AP's estimated throughput, supplicant will > skip the roam. These decisions are based only on one factor and can > lead to poor roaming choices (e.g. a roam should not happen if the > selected AP's estimated througput meets the threshold but the > current signal and throughput are already good, whereas a roam > should happen if the signal is slightly worse but the estimated > throughput is significantly better). > > This change standardizes the roaming heuristic and will hopefully > improve user WiFi experience. The change can be summarized as > follows: based on the current signal level, a certain roaming > difficulty is assigned. Based on the selected AP's estimated > throughput relative to the current AP's estimated throughput, the > difficulty is adjusted up or down. If the difference in signal > level meets the threshold, a roam happens. > > The hard-coded values were selected purely based on the previous > version of this function. They may eventually need to be fine-tuned > for optimal performance. Thanks. I applied some parts of this, but for others, I'd like to see more justification for the change, e.g., in a form of a debug log showing undesired roaming behavior. > diff --git a/wpa_supplicant/events.c b/wpa_supplicant/events.c > @@ -1683,27 +1683,8 @@ static int wpa_supplicant_need_to_roam(struct wpa_supplicant *wpa_s, > - if (selected->est_throughput > current_bss->est_throughput + 5000) { > - wpa_dbg(wpa_s, MSG_DEBUG, > - "Allow reassociation - selected BSS has better estimated throughput"); > - return 1; > - } I did not apply this one and this seems to be against the first part of the commit message as well, i.e., the roam should be allowed with worse signal strength if these is a significant improvement to the estimated throughput. It should also be noted that the estimated throughput values are not based on a single factor. > - if (current_bss->level < 0 && > - current_bss->level > selected->level + to_5ghz * 2) { > - wpa_dbg(wpa_s, MSG_DEBUG, "Skip roam - Current BSS has better " > - "signal level"); > - return 0; > - } I did not remove this condition completely, but I did change this to not apply if there is a significant improvement in the estimated throughput. > - if (current_bss->est_throughput > selected->est_throughput + 5000) { > - wpa_dbg(wpa_s, MSG_DEBUG, > - "Skip roam - Current BSS has better estimated throughput"); > - return 0; > - } I did not remove this either. It does not sound reasonable to roam to a BSS that is estimated to result in worse throughput. The exact definition of what exactly is significantly improve throughput is likely worth additional consideration, though, since the current implementation (5 kbps difference or 20% improvement) is not really based on any real study.
Thanks for the modifications. Most of the changes were just reasoned about e.g. we don't necessarily want to roam to an AP with est. throughput 5k higher because that's often not a significant difference. Take 138000 to 143000 for example. In any case, I don't have hard evidence that either version of the patch is better than the other, but I'll submit another patch if something comes up. On Thu, Jan 2, 2020 at 12:32 AM Jouni Malinen <j@w1.fi> wrote: > > On Tue, Jul 17, 2018 at 10:56:21AM -0700, Matthew Wang wrote: > > Currently, wpa_supplicant roams too aggressively; the need_to_roam() > > function will return early with a roaming decision if the difference > > in signal level or throughput between the current and selected APs > > is "sufficiently large." In particular, if the selected AP's > > estimated throughput is more than 5k greater than the current AP's > > estimated throughput, supplicant will decide to roam. Otherwise, if > > the selected AP's signal level is less than the current AP's signal > > level, or the selected AP's estimated throughput is at least 5k > > less than the current AP's estimated throughput, supplicant will > > skip the roam. These decisions are based only on one factor and can > > lead to poor roaming choices (e.g. a roam should not happen if the > > selected AP's estimated througput meets the threshold but the > > current signal and throughput are already good, whereas a roam > > should happen if the signal is slightly worse but the estimated > > throughput is significantly better). > > > > This change standardizes the roaming heuristic and will hopefully > > improve user WiFi experience. The change can be summarized as > > follows: based on the current signal level, a certain roaming > > difficulty is assigned. Based on the selected AP's estimated > > throughput relative to the current AP's estimated throughput, the > > difficulty is adjusted up or down. If the difference in signal > > level meets the threshold, a roam happens. > > > > The hard-coded values were selected purely based on the previous > > version of this function. They may eventually need to be fine-tuned > > for optimal performance. > > Thanks. I applied some parts of this, but for others, I'd like to see > more justification for the change, e.g., in a form of a debug log > showing undesired roaming behavior. > > > diff --git a/wpa_supplicant/events.c b/wpa_supplicant/events.c > > @@ -1683,27 +1683,8 @@ static int wpa_supplicant_need_to_roam(struct wpa_supplicant *wpa_s, > > > - if (selected->est_throughput > current_bss->est_throughput + 5000) { > > - wpa_dbg(wpa_s, MSG_DEBUG, > > - "Allow reassociation - selected BSS has better estimated throughput"); > > - return 1; > > - } > > I did not apply this one and this seems to be against the first part of > the commit message as well, i.e., the roam should be allowed with worse > signal strength if these is a significant improvement to the estimated > throughput. It should also be noted that the estimated throughput values > are not based on a single factor. > > > - if (current_bss->level < 0 && > > - current_bss->level > selected->level + to_5ghz * 2) { > > - wpa_dbg(wpa_s, MSG_DEBUG, "Skip roam - Current BSS has better " > > - "signal level"); > > - return 0; > > - } > > I did not remove this condition completely, but I did change this to not > apply if there is a significant improvement in the estimated throughput. > > > - if (current_bss->est_throughput > selected->est_throughput + 5000) { > > - wpa_dbg(wpa_s, MSG_DEBUG, > > - "Skip roam - Current BSS has better estimated throughput"); > > - return 0; > > - } > > I did not remove this either. It does not sound reasonable to roam to a > BSS that is estimated to result in worse throughput. > > The exact definition of what exactly is significantly improve throughput > is likely worth additional consideration, though, since the current > implementation (5 kbps difference or 20% improvement) is not really > based on any real study. > > -- > Jouni Malinen PGP id EFC895FA
diff --git a/wpa_supplicant/events.c b/wpa_supplicant/events.c index 66d9f88db..a095ee02f 100644 --- a/wpa_supplicant/events.c +++ b/wpa_supplicant/events.c @@ -1683,27 +1683,8 @@ static int wpa_supplicant_need_to_roam(struct wpa_supplicant *wpa_s, return 1; } - if (selected->est_throughput > current_bss->est_throughput + 5000) { - wpa_dbg(wpa_s, MSG_DEBUG, - "Allow reassociation - selected BSS has better estimated throughput"); - return 1; - } - to_5ghz = selected->freq > 4000 && current_bss->freq < 4000; - if (current_bss->level < 0 && - current_bss->level > selected->level + to_5ghz * 2) { - wpa_dbg(wpa_s, MSG_DEBUG, "Skip roam - Current BSS has better " - "signal level"); - return 0; - } - - if (current_bss->est_throughput > selected->est_throughput + 5000) { - wpa_dbg(wpa_s, MSG_DEBUG, - "Skip roam - Current BSS has better estimated throughput"); - return 0; - } - cur_est = current_bss->est_throughput; sel_est = selected->est_throughput; min_diff = 2; @@ -1718,32 +1699,28 @@ static int wpa_supplicant_need_to_roam(struct wpa_supplicant *wpa_s, min_diff = 4; else min_diff = 5; - if (cur_est > sel_est * 1.5) - min_diff += 10; - else if (cur_est > sel_est * 1.2) - min_diff += 5; - else if (cur_est > sel_est * 1.1) - min_diff += 2; - else if (cur_est > sel_est) - min_diff++; - } - if (to_5ghz) { - int reduce = 2; - - /* Make it easier to move to 5 GHz band */ - if (sel_est > cur_est * 1.5) - reduce = 5; - else if (sel_est > cur_est * 1.2) - reduce = 4; - else if (sel_est > cur_est * 1.1) - reduce = 3; - - if (min_diff > reduce) - min_diff -= reduce; - else - min_diff = 0; } - diff = abs(current_bss->level - selected->level); + + if (cur_est > sel_est * 1.5) + min_diff += 10; + else if (cur_est > sel_est * 1.2) + min_diff += 5; + else if (cur_est > sel_est * 1.1) + min_diff += 2; + else if (cur_est > sel_est) + min_diff++; + else if (sel_est > cur_est * 1.5) + min_diff -= 10; + else if (sel_est > cur_est * 1.2) + min_diff -= 5; + else if (sel_est > cur_est * 1.1) + min_diff -= 2; + else if (sel_est > cur_est) + min_diff--; + + if (to_5ghz) + min_diff -= 2; + diff = selected->level - current_bss->level; if (diff < min_diff) { wpa_dbg(wpa_s, MSG_DEBUG, "Skip roam - too small difference in signal level (%d < %d)",
Currently, wpa_supplicant roams too aggressively; the need_to_roam() function will return early with a roaming decision if the difference in signal level or throughput between the current and selected APs is "sufficiently large." In particular, if the selected AP's estimated throughput is more than 5k greater than the current AP's estimated throughput, supplicant will decide to roam. Otherwise, if the selected AP's signal level is less than the current AP's signal level, or the selected AP's estimated throughput is at least 5k less than the current AP's estimated throughput, supplicant will skip the roam. These decisions are based only on one factor and can lead to poor roaming choices (e.g. a roam should not happen if the selected AP's estimated througput meets the threshold but the current signal and throughput are already good, whereas a roam should happen if the signal is slightly worse but the estimated throughput is significantly better). This change standardizes the roaming heuristic and will hopefully improve user WiFi experience. The change can be summarized as follows: based on the current signal level, a certain roaming difficulty is assigned. Based on the selected AP's estimated throughput relative to the current AP's estimated throughput, the difficulty is adjusted up or down. If the difference in signal level meets the threshold, a roam happens. The hard-coded values were selected purely based on the previous version of this function. They may eventually need to be fine-tuned for optimal performance. Signed-off-by: Matthew Wang <matthewmwang@chromium.org> --- wpa_supplicant/events.c | 65 +++++++++++++---------------------------- 1 file changed, 21 insertions(+), 44 deletions(-)