From: James Prestwood <prestwoj@gmail.com>
To: Denis Kenzior <denkenz@gmail.com>, iwd@lists.linux.dev
Subject: Re: [PATCH v2 2/3] station: fall back to reassociation under certain FT failures
Date: Thu, 31 Aug 2023 04:40:17 -0700 [thread overview]
Message-ID: <d3e32c55-6b86-40f6-94b8-045656a66c0f@gmail.com> (raw)
In-Reply-To: <14b92465-07b9-7f76-556f-0bd79dc2a6f7@gmail.com>
Hi Denis,
On 8/30/23 7:34 PM, Denis Kenzior wrote:
> Hi James,
>
> On 8/29/23 09:51, James Prestwood wrote:
>> The auth/action status is now tracked in ft.c. If an AP rejects the
>> FT attempt with "Invalid PMKID" we can now assume this AP is either
>> mis-configured for FT or is lagging behind getting the proper keys
>> from neighboring APs (e.g. was just rebooted).
>>
>> If we see this condition IWD can now fall back to reassociation in
>> an attempt to still roam to the best candidate. The fallback decision
>> is still rank based: if a BSS fails FT it is marked as such, its
>> ranking is re-computed removing the FT factor and it is inserted back
>> into the queue.
>>
>> The motivation behind this isn't necessarily to always force a roam,
>> but instead to handle two cases where IWD can either make a bad roam
>> decision or get 'stuck' and never roam:
>>
>> 1. If there is one good roam candidate and other bad ones. For
>> example say BSS A is experiencing this FT key pull issue:
>> Current BSS: -85dbm
>> BSS A: -55dbm
>> BSS B: -80dbm
>> The current logic would fail A, and roam to B. In this case
>> reassociation would have likely succeeded so it makes more sense
>> to reassociate to A as a fallback.
>>
>> 2. If there is only one candidate, but its failing FT. IWD will
>> never try anything other than FT and repeatedly fail.
>>
>> Both of the above have been seen on real network deployments and
>> result in either poor performance (1) or eventually lead to a full
>> disconnect due to never roaming (2).
>> ---
>> src/station.c | 124 ++++++++++++++++++++++++++++++++------------------
>> 1 file changed, 80 insertions(+), 44 deletions(-)
>>
>> v2:
>> * Rather than always falling back to reassociate recompute the
>> rank and insert into the queue. This allows IWD to try similarly
>> ranked BSS's with FT before falling back to reassociation.
>>
>
> <snip>
>
>> diff --git a/src/station.c b/src/station.c
>> index 2473de2a..59191baa 100644
>> --- a/src/station.c
>> +++ b/src/station.c
>> @@ -73,6 +73,7 @@ static bool supports_arp_evict_nocarrier;
>> static bool supports_ndisc_evict_nocarrier;
>> static struct watchlist event_watches;
>> static uint32_t known_networks_watch;
>> +static const double RANK_FT_FACTOR = 1.3;
>> struct station {
>> enum station_state state;
>> @@ -147,15 +148,49 @@ struct roam_bss {
>> uint8_t addr[6];
>> uint16_t rank;
>
> I'd store the original (non-FT factored rank here). This lets you get
> rid of the double division which I think might be expensive on some
> platforms.
>
>> int32_t signal_strength;
>> + bool reassoc: 1;
>
> I'd name this something like ft_failed.
>
>> };
>> -static struct roam_bss *roam_bss_from_scan_bss(const struct scan_bss
>> *bss)
>
> <snip>
>
>> +static struct roam_bss *roam_bss_from_scan_bss(struct handshake_state
>> *hs,
>> + const struct scan_bss *bss)
>> {
>> struct roam_bss *rbss = l_new(struct roam_bss, 1);
>> memcpy(rbss->addr, bss->addr, 6);
>> rbss->rank = bss->rank;
>> rbss->signal_strength = bss->signal_strength;
>> + rbss->reassoc = !station_can_fast_transition(hs, bss);
>
> Why would you do this for every candidate up-front? If the first one
> succeeds, no need to do this for the rest. Also, strictly speaking the
> scan results might have changed in the meantime. You should be checking
> this against the fresh bss obtained in station_transition_start.
>
>> return rbss;
>> }
>
> <snip>
>
>> @@ -2264,19 +2272,43 @@ static void station_transition_start(struct
>> station *station);
>> static bool station_ft_work_ready(struct wiphy_radio_work_item *item)
>> {
>> struct station *station = l_container_of(item, struct station,
>> ft_work);
>> - struct roam_bss *rbss = l_queue_pop_head(station->roam_bss_list);
>> - struct scan_bss *bss = network_bss_find_by_addr(
>> - station->connected_network, rbss->addr);
>> + _auto_(l_free) struct roam_bss *rbss = l_queue_pop_head(
>> + station->roam_bss_list);
>> + struct scan_bss *bss;
>> int ret;
>> - l_free(rbss);
>> -
>> /* Very unlikely, but the BSS could have gone away */
>> + bss = network_bss_find_by_addr(station->connected_network,
>> rbss->addr);
>> if (!bss)
>> goto try_next;
>> ret = ft_associate(netdev_get_ifindex(station->netdev), bss->addr);
>> - if (ret == -ENOENT) {
>
> Okay, but it feels like the resulting if statement is starting to become
> messy after these changes. Maybe using a switch/case would be better?
I'll see what I can do, but with both ret < 0 and ret > 0 checks things
don't immediately seem better with a case statement. Maybe I'll contain
this re-insert into its own function.
>
>> + if (ret > 0) {
>> + if (ret != MMPDU_STATUS_CODE_INVALID_PMKID) {
>> + station_debug_event(station, "ft-roam-failed");
>> + goto try_next;
>> + }
>> +
>> + /*
>> + * Re-insert removing FT from the ranking. If the BSS is still
>> + * the best reassociation will be used, otherwise we'll try
>> + * more FT candidates that are better ranked
>> + */
>> + rbss->rank /= RANK_FT_FACTOR;
>> + rbss->reassoc = true;
>
> Here's where saving the original rank would be better. You can simply
> assign the original rank to the current one and avoid the division.
So this actually points something I overlooked. The rbss->rank does not
actually take into account FT. RANK_FT_FACTOR is added onto the
scan_bss->rank locally and only used to compare against the current BSS.
So actually if FT failed we would be ranking those BSS's lower than
other non-FT BSS's by dividing.
So instead I think I need to store rbss->ft_rank and use both that and
rbss->rank for insertion:
>
>> +
>> + l_debug("Re-inserting BSS "MAC" forcing reassociation, rank:
>> %u",
>> + MAC_STR(rbss->addr), rbss->rank);
>> +
>> + l_queue_insert(station->roam_bss_list, rbss,
>> + roam_bss_rank_compare, NULL);
>> +
>> + station_debug_event(station, "ft-fallback-to-reassoc");
>> +
>> + station_transition_start(station);
>> + l_steal_ptr(rbss);
>> + return true;
>> + } else if (ret == -ENOENT) {
>> station_debug_event(station, "ft-roam-failed");
>> try_next:
>> station_transition_start(station);
>
> <snip>
>
>> @@ -2490,7 +2526,6 @@ static bool station_roam_scan_notify(int err,
>> struct l_queue *bss_list,
>> struct scan_bss *current_bss = station->connected_bss;
>> struct scan_bss *bss;
>> double cur_bss_rank = 0.0;
>> - static const double RANK_FT_FACTOR = 1.3;
>
> You probably don't need to move this if you save the original rank.
>
>> uint16_t mdid;
>> enum security orig_security, security;
>
> <snip>
>
> Regards,
> -Denis
next prev parent reply other threads:[~2023-08-31 11:40 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-08-29 14:51 [PATCH v2 1/3] ft: track FT auth/action response status James Prestwood
2023-08-29 14:51 ` [PATCH v2 2/3] station: fall back to reassociation under certain FT failures James Prestwood
2023-08-31 2:34 ` Denis Kenzior
2023-08-31 11:40 ` James Prestwood [this message]
2023-08-29 14:51 ` [PATCH v2 3/3] auto-t: add fallback to reassociate test James Prestwood
2023-08-29 14:53 ` James Prestwood
2023-08-31 2:09 ` [PATCH v2 1/3] ft: track FT auth/action response status Denis Kenzior
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=d3e32c55-6b86-40f6-94b8-045656a66c0f@gmail.com \
--to=prestwoj@gmail.com \
--cc=denkenz@gmail.com \
--cc=iwd@lists.linux.dev \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).