Hi Marek,
Thanks for your feedback!
On Sun, Feb 10, 2019 at 07:59:59PM +0800, Marek Lindner wrote:
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 ?
The DHCPACK snooping part should reduce the broadcasted ARP Requests that were answered. Which is the 12.881% DAT BCAST part in the link.
For the unanswered ARP Requests I do not expect any reduction from the DHCPACK snooping part. If the client device is gone than there will be no more DHCPACKs to refresh entries either.
This patch however targets the unanswered ARP Requests. So even if the client is gone, we will then still respond with the value stored in the DHT, without falling back to broadcasting.
I haven't tested these two patches in the network I had performed the initial measurements and calculations in back then. But the DHCPACKs patch was applied and tested on gateways at Freifunk Darmstadt. And for this patch here I had tested in VMs that the DHT-PUT entry stayed for longer than the previous 5 minutes.
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 ?
Correct.
@@ -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));
Good idea, thanks!
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".
Regards, Linus