Thanks Antonio!
On Mon, Feb 11, 2019 at 08:21:22AM +1000, Antonio Quartulli wrote:
Hi,
On 11/02/2019 01:09, Linus Lüssing wrote:
Furthermore, don't jiffies overflow at some point on some architectures ? Initializing a jiffies field with 0 appears error-prone.
Hm, good point. Assuming 32bit and 1 jiffy = 1ms it would overflow every 49 days (2^32/1000/60/60/24). The time_before() macros should accomodate for that (as long as the value to compare with is < 49/2 days apart?). However you are right, the 0 value would probably lead to faulty results for 49/2 days then...
Anyway, removing that new timeout thing should fix it, as last_update is always initialized with "jiffies".
+1
the problem is on last_dht_update that gets initializes with 0 (for locally cached entries). If you agree on removing it and using a bool (I like this idea too!), the overflow problem should be gone.
While updating the patch I noticed two more things in batadv_dat_snoop_incoming_arp_reply():
It seems that batadv_dat_entry_add() is only performed if the dat entry does not exist yet. Which kind of defeats the purpose of the DHCPACK snooping and extended timeout, I guess?
I'm thinking about moving the "goto out" here[0] to below the batadv_dat_entry_add(). Something like:
-------- bool batadv_dat_snoop_incoming_arp_reply(struct batadv_priv *bat_priv, struct sk_buff *skb, int hdr_size) { [...] dat_entry = batadv_dat_entry_hash_find(bat_priv, ip_src, vid); if (dat_entry && batadv_compare_eth(hw_src, dat_entry->mac_addr)) { batadv_dbg(BATADV_DBG_DAT, bat_priv, "Doubled ARP reply removed: ARP MSG = [src: %pM-%pI4 dst: %pM-%pI4]; dat_entry: %pM-%pI4\n", hw_src, &ip_src, hw_dst, &ip_dst, dat_entry->mac_addr, &dat_entry->ip); dropped = true; // Remove: // goto out; }
/* Update our internal cache with both the IP addresses the node got * within the ARP reply */ batadv_dat_entry_add(bat_priv, ip_src, hw_src, vid); batadv_dat_entry_add(bat_priv, ip_dst, hw_dst, vid);
// New: if (dropped) goto out; [...] --------
Secondly, I'm wondering about what to do with ARP Replies on batadv_dat_snoop_incoming_arp_reply() which did not come with a DHT-PUT.
With the v1 of this patch I would either update last_update or last_dht_update. Now I need to decide. Updating dat_entry->last_update and setting dat_entry->global = true for incoming ARP Replies which did not come via a DHT-PUT seems wrong. We only want long timeouts on the three DHT candidates because they are the ones that will be updated reliably from other nodes.
So, should I just avoid updating DAT entries via incoming ARP Replies that did not come via a DHT-PUT? Any other ideas?
Regards, Linus