* [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
* 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).