Re: [patch v1, kernel version 3.2.1] net/ipv4/ip_gre: Ethernet multipoint GRE over IP

2012/1/16 David Laight [off-list ref]:

quoted

quoted

+   for (i = 0; i < GRETAP_BR_HASH_SIZE; i++) {
+           struct ipgre_tap_bridge_entry *entry;
+           struct hlist_node *h, *n;
+           hlist_for_each_entry_safe(entry, h, n,
+                   &tunnel->hash[i], hlist)
+           {
+                   unsigned long this_timer;
+                   this_timer = entry->ageing_timer + delay;
+                   if (time_before_eq(this_timer, jiffies))
+                           ipgre_tap_bridge_delete(entry);
+                   else if (time_before(this_timer, next_timer))
+                           next_timer = this_timer;
+           }
+   }
+   spin_unlock_bh(&tunnel->hash_lock);
+   mod_timer(&tunnel->gc_timer, round_jiffies(next_timer + HZ/4));

wow... why setup a 250 ms timer, if entries are valid 300 seconds ?

Isn't that code trying to wakeup 250ms after the first expiry of any
of its items?

Do you even need a timer at all?

It may be enough to just put a timestamp into each entry
and tidy up when scanning one of the hash chains in (say)
the 'add item' path - when you need write access anyway.

A hash table might not be the best structure either!
The hash lookup is still o(n) for n >> table_size.
It also may be likely that you'll do repeated lookups for a small
number of items - in which case using a hash to cache recent lookups
might be useful (maybe with some type of balanced tree structure)

       David

That part of code is modified from original linux bridge code, as I
wanted to avoid of developing something that was already developed.
The timer is actually needed to network


-- 
Stefan Gula