* [ECOS] uipc_socket.c @ 2005-06-15 22:39 Will Lentz 2005-06-16 7:06 ` [ECOS] uipc_socket.c (and cyg_tcp_maxidle) Hans Hübner 0 siblings, 1 reply; 9+ messages in thread From: Will Lentz @ 2005-06-15 22:39 UTC (permalink / raw) To: ecos-discuss; +Cc: Will Lentz Hi, I may have found a potential bug in packages/net/bsd_tcpip/current/src/sys/kern/uipc_socket.c (or I may be completely wrong :-). At the end of sodealloc(), the following code exists: zfreei(so->so_zone, so); wakeup(so->so_zone); The problem is that zfreei() changes so->so_zone. Shouldn't wakeup() be done on the original so->so_zone? I only noticed this problem by: 1- while(1) { sock = socket( AF_INET, SOCK_STREAM, IPPROTO_TCP ); connect( sock, ... ); close( sock ); } Eventually this pauses in socket() (in cyg_tsleep()) when you run out of eCos sockets. 2- After 2*MSL or so, cyg_wakeup() gets called with chan == 0x0. Why? The zfreei() call in sodealloc() changes so->so_zone to 0 before the wakeup() call. The following diff solves the problem for me by making the wakeup() work on the so_zone that was freed: --- uipc_socket.c Thu Jul 24 11:04:25 2003 +++ new_uipc_socket.c Wed Jun 15 14:54:20 2005 @@ -202,8 +202,12 @@ FREE(so->so_accf, M_ACCF); } #endif /* INET */ - zfreei(so->so_zone, so); - wakeup(so->so_zone); + { + struct vm_zone *tmp = so->so_zone; + + zfreei(so->so_zone, so); + wakeup(tmp); + } } Any ideas? Suggestions? Thanks, Will -- Before posting, please read the FAQ: http://ecos.sourceware.org/fom/ecos and search the list archive: http://ecos.sourceware.org/ml/ecos-discuss ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [ECOS] uipc_socket.c (and cyg_tcp_maxidle) 2005-06-15 22:39 [ECOS] uipc_socket.c Will Lentz @ 2005-06-16 7:06 ` Hans Hübner 2005-06-16 15:36 ` Will Lentz 2005-06-17 20:12 ` Andrew Lunn 0 siblings, 2 replies; 9+ messages in thread From: Hans Hübner @ 2005-06-16 7:06 UTC (permalink / raw) To: Will Lentz; +Cc: ecos-discuss On Wed, 15 Jun 2005, Will Lentz wrote: > I may have found a potential bug in > packages/net/bsd_tcpip/current/src/sys/kern/uipc_socket.c (or I may be > completely wrong :-). > > At the end of sodealloc(), the following code exists: > zfreei(so->so_zone, so); > wakeup(so->so_zone); > The problem is that zfreei() changes so->so_zone. Shouldn't wakeup() be > done on the original so->so_zone? I only noticed this problem by: > 1- while(1) { > sock = socket( AF_INET, SOCK_STREAM, IPPROTO_TCP ); > connect( sock, ... ); > close( sock ); > } > Eventually this pauses in socket() (in cyg_tsleep()) when you run out > of eCos sockets. > > 2- After 2*MSL or so, cyg_wakeup() gets called with chan == 0x0. Why? > The zfreei() call in sodealloc() changes so->so_zone to 0 before the > wakeup() call. If I read the source correctly, zfreei() is a function, not a macro, and it is passed the zone argument by value (as is default with C). Thus, zfreei() cannot change the so structure itself and the value of so->so_zone will be unmodified after the call. sleep() and wakeup() take void* as argument and do not interpret the value, so to them it is not of concern whether the passed pointer is actually valid. I looked into this because I had some problems with eCos and an embedded web server. Seemingly, the server worked fine but stopped serving after some 10 requests had been processed. Looking into this, I found that the number of sockets in our default configuration was very low (16). As TCP sockets stay allocated after the connection has closed until the tcp_maxidle period has expired, no new connections could be accepted until that period (which is in the minutes range in the default configuration) passed. Not having looked at the TCP specification, I decided that reducing the tcp_maxidle parameter to a very low value combined with an increase in the number of sockets would help me out: extern int cyg_tcp_maxidle; cyg_tcp_maxidle = 200; I am still not completely satisfied with the solution, because I don't really know whether this change will result in bad interactions with other TCP network partners. Also, I have found the numbers reported for TCP accept/established/close do not correspond and I suspect that there are in fact other bugs which get triggered by hitting the socket limit very often. I hope this is useful to some. Regards, Hans -- Before posting, please read the FAQ: http://ecos.sourceware.org/fom/ecos and search the list archive: http://ecos.sourceware.org/ml/ecos-discuss ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [ECOS] uipc_socket.c (and cyg_tcp_maxidle) 2005-06-16 7:06 ` [ECOS] uipc_socket.c (and cyg_tcp_maxidle) Hans Hübner @ 2005-06-16 15:36 ` Will Lentz 2005-06-16 15:51 ` Hans Hübner 2005-06-17 20:12 ` Andrew Lunn 1 sibling, 1 reply; 9+ messages in thread From: Will Lentz @ 2005-06-16 15:36 UTC (permalink / raw) To: Hans Hübner; +Cc: ecos-discuss Hi Hans, zfreei() is a function, and doesn't directly modify its first argument. But - it passes "so" as its second argument, and it modifies the value of so->so_zone through that. Looking at zfreei: { elem *p = (elem *)item; // item here is "so" log(LOG_MDEBUG, "zfreei to %s <= %p\n", zone->name, p); p->next = zone->pool; // p->next corresponds to so->so_zone in memory zone->pool = p; zone->free++; zone->alloc_frees++; } It's true that wakeup() doesn't care if it is passed a valid argument, but if it gets the wrong value then it doesn't wake up the correct people :-). Maybe zfreei() shouldn't be passed "so" as its second argument in sodealloc? Can anyone comment on why this is done? It looks fishy casting a "so" pointer to an "elem *" and then mucking with it... Every other example of zfreei() I can find passes the value returned from zalloci() as the second argument of zfreei(). Thanks, Will P.S. I also found that changing cyg_tcp_maxidle helps, but I still think there is some problem. On Thu, 2005-06-16 at 09:01 +0200, Hans Hübner wrote: > On Wed, 15 Jun 2005, Will Lentz wrote: > > > I may have found a potential bug in > > packages/net/bsd_tcpip/current/src/sys/kern/uipc_socket.c (or I may be > > completely wrong :-). > > > > At the end of sodealloc(), the following code exists: > > zfreei(so->so_zone, so); > > wakeup(so->so_zone); > > The problem is that zfreei() changes so->so_zone. Shouldn't wakeup() be > > done on the original so->so_zone? I only noticed this problem by: > > 1- while(1) { > > sock = socket( AF_INET, SOCK_STREAM, IPPROTO_TCP ); > > connect( sock, ... ); > > close( sock ); > > } > > Eventually this pauses in socket() (in cyg_tsleep()) when you run out > > of eCos sockets. > > > > 2- After 2*MSL or so, cyg_wakeup() gets called with chan == 0x0. Why? > > The zfreei() call in sodealloc() changes so->so_zone to 0 before the > > wakeup() call. > > If I read the source correctly, zfreei() is a function, not a macro, and it > is passed the zone argument by value (as is default with C). Thus, zfreei() > cannot change the so structure itself and the value of so->so_zone will be > unmodified after the call. sleep() and wakeup() take void* as argument and do > not interpret the value, so to them it is not of concern whether the passed > pointer is actually valid. > > I looked into this because I had some problems with eCos and an embedded web > server. Seemingly, the server worked fine but stopped serving after some 10 > requests had been processed. Looking into this, I found that the number of > sockets in our default configuration was very low (16). As TCP sockets stay > allocated after the connection has closed until the tcp_maxidle period has > expired, no new connections could be accepted until that period (which is in > the minutes range in the default configuration) passed. > > Not having looked at the TCP specification, I decided that reducing the > tcp_maxidle parameter to a very low value combined with an increase in the > number of sockets would help me out: > > extern int cyg_tcp_maxidle; > cyg_tcp_maxidle = 200; > > I am still not completely satisfied with the solution, because I don't really > know whether this change will result in bad interactions with other TCP > network partners. Also, I have found the numbers reported for TCP > accept/established/close do not correspond and I suspect that there are in > fact other bugs which get triggered by hitting the socket limit very often. > > I hope this is useful to some. > > Regards, > Hans -- Before posting, please read the FAQ: http://ecos.sourceware.org/fom/ecos and search the list archive: http://ecos.sourceware.org/ml/ecos-discuss ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [ECOS] uipc_socket.c (and cyg_tcp_maxidle) 2005-06-16 15:36 ` Will Lentz @ 2005-06-16 15:51 ` Hans Hübner 2005-06-16 16:06 ` Will Lentz 0 siblings, 1 reply; 9+ messages in thread From: Hans Hübner @ 2005-06-16 15:51 UTC (permalink / raw) To: Will Lentz; +Cc: ecos-discuss On Thu, 16 Jun 2005, Will Lentz wrote: > zfreei() is a function, and doesn't directly modify its first argument. > But - it passes "so" as its second argument, and it modifies the value > of so->so_zone through that. Looking at zfreei: Oops, it helps being awake while posting! Thanks for the patch, I'll apply it to my source tree immediately :) Cheers, Hans -- Before posting, please read the FAQ: http://ecos.sourceware.org/fom/ecos and search the list archive: http://ecos.sourceware.org/ml/ecos-discuss ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [ECOS] uipc_socket.c (and cyg_tcp_maxidle) 2005-06-16 15:51 ` Hans Hübner @ 2005-06-16 16:06 ` Will Lentz [not found] ` <20050616183534.N69813@web.m68k.de> 0 siblings, 1 reply; 9+ messages in thread From: Will Lentz @ 2005-06-16 16:06 UTC (permalink / raw) To: Hans Hübner; +Cc: ecos-discuss The patch does help me when I run out of sockets via the socket() call, but I'm wondering if the zfreei() in sodealloc() is getting the correct second argument. I'll dig into this deeper... Cheers, Will On Thu, 2005-06-16 at 17:46 +0200, Hans Hübner wrote: > On Thu, 16 Jun 2005, Will Lentz wrote: > > > zfreei() is a function, and doesn't directly modify its first argument. > > But - it passes "so" as its second argument, and it modifies the value > > of so->so_zone through that. Looking at zfreei: > > Oops, it helps being awake while posting! Thanks for the patch, I'll apply it > to my source tree immediately :) > > Cheers, > Hans -- Before posting, please read the FAQ: http://ecos.sourceware.org/fom/ecos and search the list archive: http://ecos.sourceware.org/ml/ecos-discuss ^ permalink raw reply [flat|nested] 9+ messages in thread
[parent not found: <20050616183534.N69813@web.m68k.de>]
* Re: [ECOS] uipc_socket.c (and cyg_tcp_maxidle) [not found] ` <20050616183534.N69813@web.m68k.de> @ 2005-06-16 23:45 ` Will Lentz 2005-06-17 7:15 ` Andrew Lunn 0 siblings, 1 reply; 9+ messages in thread From: Will Lentz @ 2005-06-16 23:45 UTC (permalink / raw) To: Hans Hübner; +Cc: ecos Hi Hans, No problem :-). I'm forwarding this on to the list so others know it helps. BTW - any comments from people? In uipc_socket.c soalloc() the zalloci() is assigned to "so", so it looks right to do the zfreei() on "so". Can my patch be applied to CVS? Thanks, Will On Thu, 2005-06-16 at 18:40 +0200, Hans Hübner wrote: > Hi Will, > > thanks for the patch, really! I only started with eCos 2 weeks ago and I > stumbled across this very problem soon, as the default number of sockets is so > low and the bug seems to be triggered by socket shortage. With the patch > applied, my application seems to run much more stable and also the TCP > counters now look plausible. You really made my day, thanks! > > -Hans > > On Thu, 16 Jun 2005, Will Lentz wrote: > > > The patch does help me when I run out of sockets via the socket() call, > > but I'm wondering if the zfreei() in sodealloc() is getting the correct > > second argument. I'll dig into this deeper... > > > > Cheers, > > Will > > > > On Thu, 2005-06-16 at 17:46 +0200, Hans Hübner wrote: > >> On Thu, 16 Jun 2005, Will Lentz wrote: > >> > >>> zfreei() is a function, and doesn't directly modify its first argument. > >>> But - it passes "so" as its second argument, and it modifies the value > >>> of so->so_zone through that. Looking at zfreei: > >> > >> Oops, it helps being awake while posting! Thanks for the patch, I'll apply it > >> to my source tree immediately :) > >> > >> Cheers, > >> Hans > > > > > > -- Before posting, please read the FAQ: http://ecos.sourceware.org/fom/ecos and search the list archive: http://ecos.sourceware.org/ml/ecos-discuss ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [ECOS] uipc_socket.c (and cyg_tcp_maxidle) 2005-06-16 23:45 ` Will Lentz @ 2005-06-17 7:15 ` Andrew Lunn 0 siblings, 0 replies; 9+ messages in thread From: Andrew Lunn @ 2005-06-17 7:15 UTC (permalink / raw) To: Will Lentz; +Cc: Hans H?bner, ecos On Thu, Jun 16, 2005 at 04:45:23PM -0700, Will Lentz wrote: > Hi Hans, > > No problem :-). I'm forwarding this on to the list so others know it > helps. > > BTW - any comments from people? In uipc_socket.c soalloc() the > zalloci() is assigned to "so", so it looks right to do the zfreei() on > "so". There has been problems like this in the past because of the missing userspace/kernel space transition which FreeBSD has but eCos does not have. Im out on a customer visit at the moment so don't have the oportunity to look at this now, but i will take a look once im back. This things are normally very easy to fix if its what i think it is. Andrew -- Before posting, please read the FAQ: http://ecos.sourceware.org/fom/ecos and search the list archive: http://ecos.sourceware.org/ml/ecos-discuss ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [ECOS] uipc_socket.c (and cyg_tcp_maxidle) 2005-06-16 7:06 ` [ECOS] uipc_socket.c (and cyg_tcp_maxidle) Hans Hübner 2005-06-16 15:36 ` Will Lentz @ 2005-06-17 20:12 ` Andrew Lunn 2005-06-17 21:00 ` Will Lentz 1 sibling, 1 reply; 9+ messages in thread From: Andrew Lunn @ 2005-06-17 20:12 UTC (permalink / raw) To: Hans H?bner; +Cc: Will Lentz, ecos-discuss [-- Attachment #1: Type: text/plain, Size: 1297 bytes --] On Thu, Jun 16, 2005 at 09:01:23AM +0200, Hans H?bner wrote: > On Wed, 15 Jun 2005, Will Lentz wrote: > > >I may have found a potential bug in > >packages/net/bsd_tcpip/current/src/sys/kern/uipc_socket.c (or I may be > >completely wrong :-). > > > >At the end of sodealloc(), the following code exists: > > zfreei(so->so_zone, so); > > wakeup(so->so_zone); > >The problem is that zfreei() changes so->so_zone. Shouldn't wakeup() be > >done on the original so->so_zone? I only noticed this problem by: > >1- while(1) { > > sock = socket( AF_INET, SOCK_STREAM, IPPROTO_TCP ); > > connect( sock, ... ); > > close( sock ); > > } > > Eventually this pauses in socket() (in cyg_tsleep()) when you run out > >of eCos sockets. > > > >2- After 2*MSL or so, cyg_wakeup() gets called with chan == 0x0. Why? > >The zfreei() call in sodealloc() changes so->so_zone to 0 before the > >wakeup() call. This is not quite correct. zfreei() does not change so->so_zone. What it does is return the memory for the so structure to the pool. The wakeup then uses the memory which has just been returned to the pool. There is a race condition. Once back into the pool the memory could be allocated to another thread before the call to wakeup is made. Attached is a patch to fix this. Andrew [-- Attachment #2: sodealloc.diff --] [-- Type: text/plain, Size: 1495 bytes --] Index: net/bsd_tcpip/current/ChangeLog =================================================================== RCS file: /cvs/ecos/ecos-opt/net/net/bsd_tcpip/current/ChangeLog,v retrieving revision 1.52 diff -u -r1.52 ChangeLog --- net/bsd_tcpip/current/ChangeLog 27 Mar 2005 18:18:13 -0000 1.52 +++ net/bsd_tcpip/current/ChangeLog 17 Jun 2005 20:08:29 -0000 @@ -1,3 +1,8 @@ +2005-06-17 Andrew Lunn <andrew.lunn@ascom.ch> + + * src/sys/kern/uipc_socket.c (sodealloc): Fixed a race condition + when freeing the socket memory. Problem reported by Will Lent. + 2005-03-27 Andrew Lunn <andrew.lunn@ascom.ch> * src/sys/net/if.c (ifioctl): Fixed a compiler warning about Index: net/bsd_tcpip/current/src/sys/kern/uipc_socket.c =================================================================== RCS file: /cvs/ecos/ecos-opt/net/net/bsd_tcpip/current/src/sys/kern/uipc_socket.c,v retrieving revision 1.3 diff -u -r1.3 uipc_socket.c --- net/bsd_tcpip/current/src/sys/kern/uipc_socket.c 24 Jul 2003 18:04:25 -0000 1.3 +++ net/bsd_tcpip/current/src/sys/kern/uipc_socket.c 17 Jun 2005 20:08:31 -0000 @@ -188,8 +188,10 @@ void sodealloc(so) struct socket *so; + { - + vm_zone_t zone; + so->so_gencnt = ++so_gencnt; #ifdef INET if (so->so_accf != NULL) { @@ -202,8 +204,9 @@ FREE(so->so_accf, M_ACCF); } #endif /* INET */ - zfreei(so->so_zone, so); - wakeup(so->so_zone); + zone = so->so_zone; + zfreei(zone, so); + wakeup(zone); } int [-- Attachment #3: Type: text/plain, Size: 148 bytes --] -- Before posting, please read the FAQ: http://ecos.sourceware.org/fom/ecos and search the list archive: http://ecos.sourceware.org/ml/ecos-discuss ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [ECOS] uipc_socket.c (and cyg_tcp_maxidle) 2005-06-17 20:12 ` Andrew Lunn @ 2005-06-17 21:00 ` Will Lentz 0 siblings, 0 replies; 9+ messages in thread From: Will Lentz @ 2005-06-17 21:00 UTC (permalink / raw) To: Andrew Lunn; +Cc: Hans H?bner, ecos Thanks! That works great :-) On Fri, 2005-06-17 at 22:09 +0200, Andrew Lunn wrote: > On Thu, Jun 16, 2005 at 09:01:23AM +0200, Hans H?bner wrote: > > On Wed, 15 Jun 2005, Will Lentz wrote: > > > > >I may have found a potential bug in > > >packages/net/bsd_tcpip/current/src/sys/kern/uipc_socket.c (or I may be > > >completely wrong :-). > > > > > >At the end of sodealloc(), the following code exists: > > > zfreei(so->so_zone, so); > > > wakeup(so->so_zone); > > >The problem is that zfreei() changes so->so_zone. Shouldn't wakeup() be > > >done on the original so->so_zone? I only noticed this problem by: > > >1- while(1) { > > > sock = socket( AF_INET, SOCK_STREAM, IPPROTO_TCP ); > > > connect( sock, ... ); > > > close( sock ); > > > } > > > Eventually this pauses in socket() (in cyg_tsleep()) when you run out > > >of eCos sockets. > > > > > >2- After 2*MSL or so, cyg_wakeup() gets called with chan == 0x0. Why? > > >The zfreei() call in sodealloc() changes so->so_zone to 0 before the > > >wakeup() call. > > This is not quite correct. zfreei() does not change so->so_zone. What > it does is return the memory for the so structure to the pool. The > wakeup then uses the memory which has just been returned to the > pool. There is a race condition. Once back into the pool the memory > could be allocated to another thread before the call to wakeup is > made. > > Attached is a patch to fix this. > > Andrew -- Before posting, please read the FAQ: http://ecos.sourceware.org/fom/ecos and search the list archive: http://ecos.sourceware.org/ml/ecos-discuss ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2005-06-17 21:00 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2005-06-15 22:39 [ECOS] uipc_socket.c Will Lentz 2005-06-16 7:06 ` [ECOS] uipc_socket.c (and cyg_tcp_maxidle) Hans Hübner 2005-06-16 15:36 ` Will Lentz 2005-06-16 15:51 ` Hans Hübner 2005-06-16 16:06 ` Will Lentz [not found] ` <20050616183534.N69813@web.m68k.de> 2005-06-16 23:45 ` Will Lentz 2005-06-17 7:15 ` Andrew Lunn 2005-06-17 20:12 ` Andrew Lunn 2005-06-17 21:00 ` Will Lentz
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).