Message ID | 20090130225113.GA13977@ioremap.net |
---|---|
State | Changes Requested, archived |
Delegated to: | David Miller |
Headers | show |
On Sat, 31 Jan 2009 01:51:14 +0300 Evgeniy Polyakov <zbr@ioremap.net> wrote: > On Fri, Jan 30, 2009 at 11:30:22PM +0100, Eric Dumazet (dada1@cosmosbay.com) wrote: > > > It should contain rough number of sockets, there is no need to be very > > > precise because of this hueristic. > > > > Denying there is a bug is... well... I dont know what to say. > > > > I wonder why we still use atomic_t all over the kernel. > > It is not a bug. It is not supposed to be precise. At all. > I implemented a simple heuristic on when diferent bind port selection > algorithm should start: roughly when number of opened sockets equals to > some predefined value (sysctl at the moment, but it could be 64k or > anything else), so if that number is loosely maintained and does not > precisely corresponds to the number of sockets, it is not a problem. > > You also saw 'again' lavel which has magic 5 number - it is another > heuristic - since lock is dropped atfer the bind bucket check, and we > selected it, it is possible that non-reuse socket will be added into the > bucket, so we will have to rerun the process again. I limited this to > the 5 attempts only, since it is better than what we have right now (I > never saw more than 2 attempts needed in the tests), when number of > bound sockets does not exceed 64k. > > How is any of this supposed to fix the bug? -- 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
My working hypothesis is: 1. Something about Evgeniy's patch makes IPV6 (actually IPV4 in IPV6) be preferred over plain IPV4. 2. Vino server (VNC) doesn't think ::ffff::127.0.0.1 is really the localhost 3. protocol gets screwed up after that. It is probably reproducible with other services that support IPV6. -- 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
On Fri, Jan 30, 2009 at 04:36:00PM -0800, Stephen Hemminger (shemminger@vyatta.com) wrote: > > It is not a bug. It is not supposed to be precise. At all. > > I implemented a simple heuristic on when diferent bind port selection > > algorithm should start: roughly when number of opened sockets equals to > > some predefined value (sysctl at the moment, but it could be 64k or > > anything else), so if that number is loosely maintained and does not > > precisely corresponds to the number of sockets, it is not a problem. > > > > You also saw 'again' lavel which has magic 5 number - it is another > > heuristic - since lock is dropped atfer the bind bucket check, and we > > selected it, it is possible that non-reuse socket will be added into the > > bucket, so we will have to rerun the process again. I limited this to > > the 5 attempts only, since it is better than what we have right now (I > > never saw more than 2 attempts needed in the tests), when number of > > bound sockets does not exceed 64k. > > > > > > How is any of this supposed to fix the bug? Nothing from above fixes the bug. It was an explaination of how things work. Patch is based on Eric's observation about unconditional (compared to old code) attempt to get the new socket bucket when code should just return.
On Fri, Jan 30, 2009 at 06:52:24PM -0800, Stephen Hemminger (shemminger@vyatta.com) wrote: > My working hypothesis is: > 1. Something about Evgeniy's patch makes IPV6 (actually IPV4 in IPV6) be > preferred over plain IPV4. > 2. Vino server (VNC) doesn't think ::ffff::127.0.0.1 is really the localhost > 3. protocol gets screwed up after that. > > It is probably reproducible with other services that support IPV6. getaddrinfo() returns list of addresses and IPv6 was the first one iirc. Previously it bailed out, but with my change it will try again without reason for doing this. With the patch I sent based on Eric's observation things should be fine.
Evgeniy Polyakov a écrit : > On Fri, Jan 30, 2009 at 06:52:24PM -0800, Stephen Hemminger (shemminger@vyatta.com) wrote: >> My working hypothesis is: >> 1. Something about Evgeniy's patch makes IPV6 (actually IPV4 in IPV6) be >> preferred over plain IPV4. >> 2. Vino server (VNC) doesn't think ::ffff::127.0.0.1 is really the localhost >> 3. protocol gets screwed up after that. >> >> It is probably reproducible with other services that support IPV6. > > getaddrinfo() returns list of addresses and IPv6 was the first one iirc. > Previously it bailed out, but with my change it will try again without > reason for doing this. With the patch I sent based on Eric's observation > things should be fine. > Problem is your patch is wrong Evgeniy, please think about it litle bit more and resubmit it. Take the time to run this $0.02 program, before and after your upcoming fix : $ cat size.c #include <net/inet_hashtables.h> extern int printf(const char *, ...); int main(int argc, char *argv[]) { printf("offsetof(struct inet_hashinfo, bsockets)=0x%x\n", offsetof(struct inet_hashinfo, bsockets)); return 0; } $ make size.o ; gcc -o size size.o ; ./size CHK include/linux/version.h CHK include/linux/utsrelease.h SYMLINK include/asm -> include/asm-x86 CALL scripts/checksyscalls.sh CC size.o offsetof(struct inet_hashinfo, bsockets)=0x18 offset of bsockets being 0x18 or 0x20 is same result : bad because in same cache line than ehash, ehash_locks, ehash_size, ehash_locks_mask, bhash, bhash_size, unless your cpu is a Pentium. Also, I suggest you change bsockets to something more appropriate, eg a percpu counter. Thank you. Eric -- 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
--- ./include/net/inet_hashtables.h~ 2009-01-19 22:19:11.000000000 +0300 +++ ./include/net/inet_hashtables.h 2009-01-31 01:48:21.000000000 +0300 @@ -134,7 +134,6 @@ struct inet_bind_hashbucket *bhash; unsigned int bhash_size; - int bsockets; struct kmem_cache *bind_bucket_cachep; @@ -148,6 +147,7 @@ * table where wildcard'd TCP sockets can exist. Hash function here * is just local port number. */ + int bsockets; struct inet_listen_hashbucket listening_hash[INET_LHTABLE_SIZE] ____cacheline_aligned_in_smp; --- ./net/ipv4/inet_connection_sock.c~ 2009-01-19 22:21:08.000000000 +0300 +++ ./net/ipv4/inet_connection_sock.c 2009-01-31 01:50:20.000000000 +0300 @@ -172,7 +172,8 @@ } else { ret = 1; if (inet_csk(sk)->icsk_af_ops->bind_conflict(sk, tb)) { - if (sk->sk_reuse && sk->sk_state != TCP_LISTEN && --attempts >= 0) { + if (sk->sk_reuse && sk->sk_state != TCP_LISTEN && + smallest_size != -1 && --attempts >= 0) { spin_unlock(&head->lock); goto again; }