Re: [PATCH 2/7] knownnetworks: add known_network_add_connected_frequency

[Denis Kenzior <denkenz@gmail.com>]

Hi James,

On 12/20/23 07:11, James Prestwood wrote:
> This should be called when BSS is connected/roamed to which will
> insert the frequency ahead of entries which were only seen in scan
> results.
> 
> Providing this and the 'seen' variant sets up the frequency list
> into two adjacent sections:
>    [Most recently used]...[Most recently seen]
> 

So why not just keep two lists?

> Doing this will allow scanning to be more optimized and limit the
> number of frequencies while prefering BSS's that have been connected
> to prior.
> 
> Note that this list format is not synced to the file system.
> Frequencies will be synced in the order of this list but not
> containing the 'connected' bit. When IWD restarts this information
> will be lost, but the list is still sorted on disk in a better state
> than it was prior.
> ---
>   src/knownnetworks.c | 59 ++++++++++++++++++++++++++++++++++++++++++++-
>   src/knownnetworks.h |  3 +++
>   2 files changed, 61 insertions(+), 1 deletion(-)
> 
> diff --git a/src/knownnetworks.c b/src/knownnetworks.c
> index eac6c66b..e44109fd 100644
> --- a/src/knownnetworks.c
> +++ b/src/knownnetworks.c
> @@ -562,15 +562,70 @@ static bool known_frequency_match(const void *a, const void *b)
>   	return known_freq->frequency == *frequency;
>   }
>   
> +static int known_frequency_compare(const void *a, const void *b,
> +					void *user_data)
> +{
> +	const struct known_frequency *ka = a;
> +
> +	/*
> +	 * Only meant to be used to insert 'seen' frequencies. Any existing
> +	 * entry that has 'connected' set should preceed an entry that was

precede?

> +	 * only seen.
> +	 */
> +	if (ka->connected)
> +		return -1; > +
> +	/*
> +	 * Otherwise, insert immediately after the last 'connected' entry,
> +	 * i.e. the most recently seen
> +	 */
> +	return 1;
> +}
> +
>   /*
>    * Adds a frequency to the 'known' set of frequencies that this network
> - * operates on.  The list is sorted according to most-recently seen
> + * operates on. Frequencies added here will follow frequencies which have been
> + * connected to and inserted according to most-recently seen
>    */
>   int known_network_add_seen_frequency(struct network_info *info,
>   					uint32_t frequency)
>   {
>   	struct known_frequency *known_freq;
>   
> +	if (!info->known_frequencies)
> +		info->known_frequencies = l_queue_new();
> +

I'm not thrilled about the implementation...

> +	known_freq = l_queue_find(info->known_frequencies,
> +					known_frequency_match, &frequency);

You walk the entire queue once, then walk it again to remove the known_freq 
object, then walk it one more time for the insert.

> +	/*
> +	 * If no match, create a new one.
> +	 * If connected to frequency before leave that entry in place
> +	 */
> +	if (!known_freq) {
> +		known_freq = l_new(struct known_frequency, 1);
> +		known_freq->frequency = frequency;
> +	} else if (known_freq->connected)
> +		return 0;

So 'seeing' a frequency that has been connected to before doesn't change its 
sort order?

> +
> +	l_queue_remove(info->known_frequencies, known_freq);
> +
> +	/* insert the entry immediately after the last 'connected' entry */
> +	l_queue_insert(info->known_frequencies, known_freq,
> +			known_frequency_compare, NULL);
> +
> +	return 0;
> +}
> +
> +/*
> + * Adds a frequency to the known set of frequencies that this network operates
> + * on. Frequencies added here are assumed to be most recently used and inserted
> + * at the head of the queue.
> + */
> +int known_network_add_connected_frequency(struct network_info *info,
> +						uint32_t frequency)
> +{
> +	struct known_frequency *known_freq;
> +
>   	if (!info->known_frequencies)
>   		info->known_frequencies = l_queue_new();
>   
> @@ -581,6 +636,8 @@ int known_network_add_seen_frequency(struct network_info *info,
>   		known_freq->frequency = frequency;
>   	}
>   
> +	known_freq->connected = true;
> +
>   	l_queue_push_head(info->known_frequencies, known_freq);
>   
>   	return 0;

Overall I'm getting the feeling that the linked-list is the wrong data structure 
for all this.  If the intent is to maintain a bounded shortlist of frequencies 
to scan for a given network, then that's what we should do instead.

Regards,
-Denis