[v2,2/2] WNM: Use standard BSS selection and enable abridged bit handling

From: Benjamin Berg <benjamin.berg@intel.com>

Most of the logic to reject BSSs during transition has been moved into
wnm_is_bss_excluded. In addition to this, since commit 67bf89f55442
("WNM: Choose the best available BSS, not just the first one") we will
simply choose the BSS with the best throughput.

Overall, this matches the behaviour that the supplicant will use anyway
in wpa_supplicant_select_bss. The only bigger difference is that using
this will check all known BSSs instead of only the ones in the candidate
list. This means that with this change the abridged bit is handled
according to spec.

Another slight change is that this drops the logic to reject candidates
with a very low signal level. This does not seem to very relevant
anymore as we prefere stronger BSSs to begin with.

Signed-off-by: Benjamin Berg <benjamin.berg@intel.com>
---
 wpa_supplicant/events.c           |   2 +-
 wpa_supplicant/wnm_sta.c          | 129 +++---------------------------
 wpa_supplicant/wpa_supplicant_i.h |   5 ++
 3 files changed, 18 insertions(+), 118 deletions(-)

On 9/18/24 05:39, Benjamin Berg wrote:
> From: Benjamin Berg <benjamin.berg@intel.com>
> 
> Most of the logic to reject BSSs during transition has been moved into
> wnm_is_bss_excluded. In addition to this, since commit 67bf89f55442
> ("WNM: Choose the best available BSS, not just the first one") we will
> simply choose the BSS with the best throughput.

It looks like you are dropping the logic that will keep it from roaming
unless the change in estimated throughput is relatively large?

[snip]

> -	if ((!target->est_throughput && !bss_in_list->est_throughput) ||
> -	    (target->est_throughput > bss_in_list->est_throughput &&
> -	     target->est_throughput - bss_in_list->est_throughput >
> -	     bss_in_list->est_throughput >> 4)) {
> -		/* It is more than 100/16 percent better, so switch. */
> -		return target;
> -	}

Or is that somehow handled in the current code elsewhere?

Thanks,
Ben

On 9/18/24 09:12, Berg, Benjamin wrote:
> On Wed, 2024-09-18 at 07:08 -0700, Ben Greear wrote:
>> On 9/18/24 05:39, Benjamin Berg wrote:
>>> From: Benjamin Berg <benjamin.berg@intel.com>
>>>
>>> Most of the logic to reject BSSs during transition has been moved
>>> into
>>> wnm_is_bss_excluded. In addition to this, since commit 67bf89f55442
>>> ("WNM: Choose the best available BSS, not just the first one") we
>>> will
>>> simply choose the BSS with the best throughput.
>>
>> It looks like you are dropping the logic that will keep it from
>> roaming
>> unless the change in estimated throughput is relatively large?
>>
>> [snip]
>>
>>> -	if ((!target->est_throughput && !bss_in_list-
>>>> est_throughput) ||
>>> -	    (target->est_throughput > bss_in_list-
>>>> est_throughpubelieve it does not do anything since the commit I
>>> mentionedt &&
>>> -	     target->est_throughput - bss_in_list->est_throughput
>>>>
>>> -	     bss_in_list->est_throughput >> 4)) {
>>> -		/* It is more than 100/16 percent better, so
>>> switch. */
>>> -		return target;
>>> -	}
>>
>> Or is that somehow handled in the current code elsewhere?
> 
> No, I believe it does not do anything since the commit I mentioned.
> 
> If bss_in_list is set and it is better, then it will have already been
> sorted to the start by find_better_target.
> 
> If bss_in_list is not set, then obviously the code will not be reached
> and the found target is used.
> 
> And yes, that does mean that we are effectively ignoring the
> order/precedence value in the neighbor report (apart from a zero
> preference to mean "disallowed"). But, I do not think it is a change in
> behaviour at this point in time.

We need to dampen roams in the case where a user sits between two APs at nearly the same
RSSI (well, same estimated tput).  It should not roam back and forth every time a small environmental change
happens.  The code you are removing at least attempted to do that as far as I
can tell.

Thanks,
Ben

On Wed, 2024-09-18 at 07:08 -0700, Ben Greear wrote:
> On 9/18/24 05:39, Benjamin Berg wrote:
> > From: Benjamin Berg <benjamin.berg@intel.com>
> > 
> > Most of the logic to reject BSSs during transition has been moved into
> > wnm_is_bss_excluded. In addition to this, since commit 67bf89f55442
> > ("WNM: Choose the best available BSS, not just the first one") we
> > will simply choose the BSS with the best throughput.
> 
> It looks like you are dropping the logic that will keep it from roaming
> unless the change in estimated throughput is relatively large?
> 
> [snip]
> 
> > -	if ((!target->est_throughput && !bss_in_list->est_throughput) ||
> > -	    (target->est_throughput > bss_in_list->est_throught &&
> > -	     target->est_throughput - bss_in_list->est_throughput
> > > 
> > -	     bss_in_list->est_throughput >> 4)) {
> > -		/* It is more than 100/16 percent better, so switch. */
> > -		return target;
> > -	}
> 
> Or is that somehow handled in the current code elsewhere?

No, I believe it does not do anything since the commit I mentioned.

If bss_in_list is set and it is better, then it will have already been
sorted to the start by find_better_target.

If bss_in_list is not set, then obviously the code will not be reached
and the found target is used.

And yes, that does mean that we are effectively ignoring the
order/precedence value in the neighbor report (apart from a zero
preference to mean "disallowed"). But, I do not think it is a change in
behaviour at this point in time.

Benjamin

On Wed, 2024-09-18 at 09:17 -0700, Ben Greear wrote:
> On 9/18/24 09:12, Berg, Benjamin wrote:
> > On Wed, 2024-09-18 at 07:08 -0700, Ben Greear wrote:
> > > On 9/18/24 05:39, Benjamin Berg wrote:
> > > 
> > > [SNIP]
> > > Or is that somehow handled in the current code elsewhere?
> > 
> > No, I believe it does not do anything since the commit I mentioned.
> > 
> > If bss_in_list is set and it is better, then it will have already been
> > sorted to the start by find_better_target.
> > 
> > If bss_in_list is not set, then obviously the code will not be reached
> > and the found target is used.
> > 
> > And yes, that does mean that we are effectively ignoring the
> > order/precedence value in the neighbor report (apart from a zero
> > preference to mean "disallowed"). But, I do not think it is a change in
> > behaviour at this point in time.
> 
> We need to dampen roams in the case where a user sits between two APs at nearly the same
> RSSI (well, same estimated tput).  It should not roam back and forth every time a small environmental change
> happens.  The code you are removing at least attempted to do that as far as I
> can tell.

Hmm, right, I misunderstood. Yes, it prevents the roam if estimated
throughput is not better by a margin.

So, we have this code path (slightly updated by this patch) in
wnm_scan_process:

	if (pre_scan_check) {
		struct os_reltime age;

		if (!bss)
			return 0;

		os_reltime_age(&bss->last_update, &age);
		if (age.sec >= 10)
			return 0;

#ifndef CONFIG_NO_ROAMING
		if (current_bss && bss != current_bss &&
		    wpa_supplicant_need_to_roam_within_ess(wpa_s, current_bss,
							   bss))
			return 0;
#endif /* CONFIG_NO_ROAMING */
	}

We could do something similar if we are allowed to stay on current_bss.

Basically we could just call wpa_supplicant_need_to_roam_within_ess
outside of the "pre_scan_check" if we are allowed to stay with the
current BSS. If that code decides that we should not roam, we simply
select current_bss as our target and are done.

Does that seem reasonable to you? We could even just have the same
100/16 logic there … but, is that actually better?

Benjamin

On 9/18/24 09:53, Benjamin Berg wrote:
> On Wed, 2024-09-18 at 09:17 -0700, Ben Greear wrote:
>> On 9/18/24 09:12, Berg, Benjamin wrote:
>>> On Wed, 2024-09-18 at 07:08 -0700, Ben Greear wrote:
>>>> On 9/18/24 05:39, Benjamin Berg wrote:
>>>>
>>>> [SNIP]
>>>> Or is that somehow handled in the current code elsewhere?
>>>
>>> No, I believe it does not do anything since the commit I mentioned.
>>>
>>> If bss_in_list is set and it is better, then it will have already been
>>> sorted to the start by find_better_target.
>>>
>>> If bss_in_list is not set, then obviously the code will not be reached
>>> and the found target is used.
>>>
>>> And yes, that does mean that we are effectively ignoring the
>>> order/precedence value in the neighbor report (apart from a zero
>>> preference to mean "disallowed"). But, I do not think it is a change in
>>> behaviour at this point in time.
>>
>> We need to dampen roams in the case where a user sits between two APs at nearly the same
>> RSSI (well, same estimated tput).  It should not roam back and forth every time a small environmental change
>> happens.  The code you are removing at least attempted to do that as far as I
>> can tell.
> 
> Hmm, right, I misunderstood. Yes, it prevents the roam if estimated
> throughput is not better by a margin.
> 
> So, we have this code path (slightly updated by this patch) in
> wnm_scan_process:
> 
> 	if (pre_scan_check) {
> 		struct os_reltime age;
> 
> 		if (!bss)
> 			return 0;
> 
> 		os_reltime_age(&bss->last_update, &age);
> 		if (age.sec >= 10)
> 			return 0;
> 
> #ifndef CONFIG_NO_ROAMING
> 		if (current_bss && bss != current_bss &&
> 		    wpa_supplicant_need_to_roam_within_ess(wpa_s, current_bss,
> 							   bss))
> 			return 0;
> #endif /* CONFIG_NO_ROAMING */
> 	}
> 
> We could do something similar if we are allowed to stay on current_bss.
> 
> Basically we could just call wpa_supplicant_need_to_roam_within_ess
> outside of the "pre_scan_check" if we are allowed to stay with the
> current BSS. If that code decides that we should not roam, we simply
> select current_bss as our target and are done.
> 
> Does that seem reasonable to you? We could even just have the same
> 100/16 logic there … but, is that actually better?

I'm sorry, but I'm too busy with other things right now to do a good job of thinking
through your proposal.  If we _must_ (re)connect for some reason, we should take the 'best'
AP, but if we are already associated, there should be some friction to roaming.  As long
a you are satisfied you are considering and meeting these goals, then probably you
are fine.

Thanks,
Ben

> 
> Benjamin
>