Message ID | 4978EE03.9040207@cosmosbay.com |
---|---|
State | RFC, archived |
Delegated to: | David Miller |
Headers | show |
Hi Eric. On Thu, Jan 22, 2009 at 11:06:59PM +0100, Eric Dumazet (dada1@cosmosbay.com) wrote: > Hello Vitaly, thanks for this excellent report. > > Yes, current code is really not good when all ports are in use : > > We now have to scan 28232 [1] times long chains of 220 sockets. > Thats very long (but at least thread is preemptable) > > In the past (before patches), only one thread was allowed to run in kernel while scanning > udp port table (we had only one global lock udp_hash_lock protecting the whole udp table). > This thread was faster because it was not slowed down by other threads. > (But the rwlock we used was responsible for starvations of writers if many UDP frames > were received) I believe problem is in the port searching algorithm, when we have exponentially grow of the number of ports to check after random selection of the first one. This allows to have small chains but setup time will be very long. Not sure if bind chais should be that small actually. In the 64k patch, which allows to have more than 64k bound sockets per system I store rough amount of bound sockets and when it becomes larger than sysctl limit I just randomly select a bundle. This works for the bind(0) for the sockets with reuse option though. I posted a picture of the bind(0) time for the .28 kernel iirc. Or is this a different issue?
At Thu, 22 Jan 2009 23:06:59 +0100, Eric Dumazet wrote: > > err = bind(s, (const struct sockaddr*)&sa, sizeof(sa)); > > Bug here, if bind() returns -1 (all ports are in use) Yeah, there was assert(), but the program drops to problems very soon, I was lazy to handle this situation correctly and just removed it ;) > > Thanks! > > Hello Vitaly, thanks for this excellent report. > > Yes, current code is really not good when all ports are in use : > > We now have to scan 28232 [1] times long chains of 220 sockets. > Thats very long (but at least thread is preemptable) > > In the past (before patches), only one thread was allowed to run in kernel while scanning > udp port table (we had only one global lock udp_hash_lock protecting the whole udp table). Very true, my (older) kernel with udp_hash_lock just become totally unresponsive after running this test. .29-rc2 become jerky only, but still works. > This thread was faster because it was not slowed down by other threads. > (But the rwlock we used was responsible for starvations of writers if many UDP frames > were received) > > > > One way to solve the problem could be to use following : > > 1) Raising UDP_HTABLE_SIZE from 128 to 1024 to reduce average chain lengths. > > 2) In bind(0) algo, use rcu locking to find a possible usable port. All cpus can run in //, without > dirtying locks. Then lock the found chain and recheck port is available before using it. I think 2 is definitely better than 1, because 1 is not actually fixing anything, but postpones the problem slightly. > [1] replace 28232 by your actual /proc/sys/net/ipv4/ip_local_port_range values > 61000 - 32768 = 28232 > > I will try to code a patch before this week end. Cool! > Thanks > > Note : I tried to use a mutex to force only one thread in bind(0) code but got no real speedup. > But it should help if you have a SMP machine, since only one cpu will be busy in bind(0) > You saved my time, I was thinking about trying mutexes also. Thanks :) -- wbr, Vitaly -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Evgeniy Polyakov a écrit : > Hi Eric. > > On Thu, Jan 22, 2009 at 11:06:59PM +0100, Eric Dumazet (dada1@cosmosbay.com) wrote: >> Hello Vitaly, thanks for this excellent report. >> >> Yes, current code is really not good when all ports are in use : >> >> We now have to scan 28232 [1] times long chains of 220 sockets. >> Thats very long (but at least thread is preemptable) >> >> In the past (before patches), only one thread was allowed to run in kernel while scanning >> udp port table (we had only one global lock udp_hash_lock protecting the whole udp table). >> This thread was faster because it was not slowed down by other threads. >> (But the rwlock we used was responsible for starvations of writers if many UDP frames >> were received) > > I believe problem is in the port searching algorithm, when we > have exponentially grow of the number of ports to check after random > selection of the first one. This allows to have small chains but setup > time will be very long. Not sure if bind chais should be that small > actually. In the 64k patch, which allows to have more than 64k bound > sockets per system I store rough amount of bound sockets and when it > becomes larger than sysctl limit I just randomly select a bundle. > This works for the bind(0) for the sockets with reuse option though. > I posted a picture of the bind(0) time for the .28 kernel iirc. > > Or is this a different issue? > Well, this is not exactly the same issue, udp bind() code is slightly different than tcp. (Probably not so many machines use lot of udp sockets) Since UDP hash table is really small (128 slots), we can try to allocate UDP ports chains per chain, instead of port per port, to reduce number of chain lookups. In tcp, most machines have 64k slots for bind table so this wont help -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c index cf5ab05..a572407 100644 --- a/net/ipv4/udp.c +++ b/net/ipv4/udp.c @@ -155,6 +155,8 @@ int udp_lib_get_port(struct sock *sk, unsigned short snum, struct udp_hslot *hslot; struct udp_table *udptable = sk->sk_prot->h.udp_table; int error = 1; + static DEFINE_MUTEX(bind0_mutex); + int mutex_acquired = 0; struct net *net = sock_net(sk); if (!snum) { @@ -162,6 +164,8 @@ int udp_lib_get_port(struct sock *sk, unsigned short snum, unsigned rand; unsigned short first; + mutex_lock(&bind0_mutex); + mutex_acquired = 1; inet_get_local_port_range(&low, &high); remaining = (high - low) + 1; @@ -196,6 +200,8 @@ int udp_lib_get_port(struct sock *sk, unsigned short snum, fail_unlock: spin_unlock_bh(&hslot->lock); fail: + if (mutex_acquired) + mutex_unlock(&bind0_mutex); return error; }