Re: [B.A.T.M.A.N.] [PATCH] batman-adv: Increase purge timeout on DAT DHT candidates

[Marek Lindner <mareklindner@neomailbox.ch>]

[-- Attachment #1: Type: text/plain, Size: 1718 bytes --]

On Saturday, 12 January 2019 05:02:08 HKT Linus Lüssing wrote:
> Some old investigations and analysis seemed to indicate a potential
> reduction of 91.71% of unanswered ARP Requests (45min: 97.95%, 60min:
> 98.95%):

Does this reduction apply to this patch specifically or to the DHCPACK 
snooping or both ? Has this patch been tested ?

> 
> https://www.open-mesh.org/projects/batman-adv/wiki/DAT_DHCP_Snooping
> 
> This patch is rebased on top of:
> 
> "batman-adv: DHCP snooping for DAT"

That patch is now called "batman-adv: Snoop DHCPACKs for DAT" and has been 
merged ?


> @@ -152,7 +152,9 @@ static void batadv_dat_entry_put(struct batadv_dat_entry
> *dat_entry) static bool batadv_dat_to_purge(struct batadv_dat_entry
> *dat_entry) {
>  	return batadv_has_timed_out(dat_entry->last_update,
> -				    BATADV_DAT_ENTRY_TIMEOUT);
> +				    BATADV_DAT_ENTRY_TIMEOUT) &&
> +	       batadv_has_timed_out(dat_entry->last_dht_update,
> +				    BATADV_DAT_DHT_TIMEOUT);
>  }

This bit could be further simplified. Introducing 2 timeout fields is a bit 
misleading since there only are 2 cases:

* last_update is updated (or not) while last_dht_update is/remains 0
* last_update and last_dht_update have the same value

Why not turn last_dht_update into a bool and apply the timeout length based on 
that bool. Something like:

if (is_global_entry)
   return batadv_has_timed_out(dat_entry->last_update,
BATADV_DAT_DHT_TIMEOUT);
else
   return batadv_has_timed_out(dat_entry->last_update,
BATADV_DAT_ENTRY_TIMEOUT));

Furthermore, don't jiffies overflow at some point on some architectures ? 
Initializing a jiffies field with 0 appears error-prone.

Cheers,
Marek

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 488 bytes --]