netdev - Re: [patch v1, kernel version 3.2.1] net/ipv4/ip

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 16 Jan 2012 14:42:12 +0100
From:	Štefan Gula <steweg@...il.com>
To:	David Laight <David.Laight@...lab.com>
Cc:	Eric Dumazet <eric.dumazet@...il.com>,
	Alexey Kuznetsov <kuznet@....inr.ac.ru>,
	"David S. Miller" <davem@...emloft.net>,
	James Morris <jmorris@...ei.org>,
	Hideaki YOSHIFUJI <yoshfuji@...ux-ipv6.org>,
	Patrick McHardy <kaber@...sh.net>, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [patch v1, kernel version 3.2.1] net/ipv4/ip_gre: Ethernet
 multipoint GRE over IP

2012/1/16 David Laight <David.Laight@...lab.com>:
>> > +   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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html