lwIP - A Lightweight TCP/IP stack - Bugs: bug #20021, conn->sem is only signaled...
You are not allowed to post comments on this tracker with your current authentication level.
bug #20021: conn->sem is only signaled one-way
Submitter: | Simon Goldschmidt <goldsimon> | ||
Submitted: | Thu 31 May 2007 04:43:18 PM UTC | ||
Category: | None | Severity: | 5 - Blocker |
Item Group: | Crash Error | Status: | Fixed |
Privacy: | Public | Assigned to: | goldsimon |
Open/Closed: | Closed | Planned Release: | None |
lwIP version: | None |
Jump to the original submission
Wed 22 Aug 2007 08:07:48 PM UTC, comment #33: |
Simon Goldschmidt <goldsimon> |
Thu 09 Aug 2007 02:15:51 PM UTC, comment #32: Confirm? For that I would have to look at the code (which unfortunately, I canT right now), but it's what I've built my code upon, and I remember seeing it like that, yes. But again, this is only from memory. I'll look it up when I'm home. |
Simon Goldschmidt <goldsimon> |
Thu 26 Jul 2007 06:35:50 PM UTC, comment #31: Like this, code seems good (and nice!). First tests seems ok. But need some tests to be sure. Close it if you want. If I see something, I will reopen it if necessary...
|
Frédéric Bernon <fbernon> |
Thu 26 Jul 2007 08:53:57 AM UTC, comment #30: If you haven't already, could you add a comment to tcp_close() to explain why we don't care about that return value? It's the sort of change that someone could easily revert accidentally in a year or two. |
Kieran Mansley <kieranm> |
Wed 25 Jul 2007 07:32:09 PM UTC, comment #29: OK, checked it in:
|
Simon Goldschmidt <goldsimon> |
Wed 25 Jul 2007 07:23:03 PM UTC, comment #28: Hm, I think we need to change tcp_close: it returns an error if tcp_output() returns an error at the end. But this would only be true for SO_LINGER, other connections would send in the background (through timers).
|
Simon Goldschmidt <goldsimon> |
Tue 24 Jul 2007 05:29:29 PM UTC, comment #27: I have check in the change, since I think that only you and me use LWIP_TCPIP_CORE_LOCKING=1 for the moment... |
Frédéric Bernon <fbernon> |
Tue 24 Jul 2007 04:34:30 PM UTC, comment #26: Since the last change, I got a problem with the "close" of a UDP socket with LWIP_TCPIP_CORE_LOCKING=1 :
|
Frédéric Bernon <fbernon> |
Mon 16 Jul 2007 05:12:04 PM UTC, comment #25: Hmm, I remember posting an answer to comment #21 some time but I can't find it now, so that probably didn't work :-)
if ((old_state == SYN_RCVD) || (old_state == ESTABLISHED) ||
|
Simon Goldschmidt <goldsimon> |
Fri 13 Jul 2007 09:53:18 PM UTC, comment #24: Yes, it's check in, but before close it, I would like to get your comments about comment #21...
|
Frédéric Bernon <fbernon> |
Fri 13 Jul 2007 04:47:44 PM UTC, comment #23: Frédéric, your last comment is checked in, isn't it? If so, can we close this item? It seems to work... |
Simon Goldschmidt <goldsimon> |
Wed 04 Jul 2007 09:29:02 AM UTC, comment #22: About socket layer: so, netconn_close is not called, we stay on a netconn_delete: ok, since this call do the job (and a smart linker will remove netconn_close/do_close from the code).
|
Frédéric Bernon <fbernon> |
Wed 04 Jul 2007 08:48:11 AM UTC, comment #21: Since the job done in netconn_close/do_close is the same as the NETCONN_TCP part of netconn_delete/do_delconn, the code is a redundant.
|
Frédéric Bernon <fbernon> |
Tue 03 Jul 2007 07:30:34 PM UTC, comment #20: Ok, since noone objected, I've checked it in. Please test and report if there still are problems! |
Simon Goldschmidt <goldsimon> |
Sun 01 Jul 2007 04:11:55 PM UTC, comment #19: I've put together a patch that removes conn->sem and implements a better closing mechanism calling an internal 'do_close_internal' function in api_msg.c from do_close/do_delconn and releasing the blocked application only when the connection is really closed. This removes unnecessary context switches, the do_close_internal is called again from sent_tcp/poll_tcp until the connection is really closed (application thread is then waked). err_tcp for such a conn also wakes the application thread.
|
Simon Goldschmidt <goldsimon> |
Thu 28 Jun 2007 10:58:47 AM UTC, comment #18: Anyone testing this? |
Simon Goldschmidt <goldsimon> |
Fri 22 Jun 2007 06:27:24 AM UTC, comment #17:
|
Simon Goldschmidt <goldsimon> |
Thu 21 Jun 2007 09:24:27 PM UTC, comment #16: For such change, I think it would be good to first see a patch file before decide to check in. I will test it and tell you if I meet (or not) any problems... |
Frédéric Bernon <fbernon> |
Thu 21 Jun 2007 06:26:05 PM UTC, comment #15: I'm ready with the change and I'll check it in. I'm open to comments on that. Especially LWIP_TCPIP_CORE_LOCKING code can be removed altogether if netconn_write uses tcpip_apimsg() instead of TCPIP_APIMSG().
|
Simon Goldschmidt <goldsimon> |
Tue 19 Jun 2007 04:16:19 PM UTC, comment #14: That means if we have free memory or not, we always switch tasks to tcpip_thread. As bad as it sounds, it can be faster than what we have now_ for data bigger than tcp_sndbuf(conn->pcb.tcp), we only have one task-change. As opposed to multiple (one task change [and back] per sendbuf-size). |
Simon Goldschmidt <goldsimon> |
Fri 01 Jun 2007 04:40:49 PM UTC, comment #13: The existing behaviour of netconn_write is to block and wait if out of mem or send window, so to preserve that, the calling thread would block in tcpip_apimsg waiting for the sys_arch_mbox_fetch (or whatever replaces it, from Frederic's work) to return. The tcpip thread would only wake it up when it really does complete. So I think you can guarantee someone is waiting because that's where they will be blocked.
|
Jonathan Larmour <jifl> |
Fri 01 Jun 2007 04:02:20 PM UTC, comment #12:
|
Simon Goldschmidt <goldsimon> |
Fri 01 Jun 2007 01:29:30 PM UTC, comment #11:
|
Jonathan Larmour <jifl> |
Fri 01 Jun 2007 01:24:41 PM UTC, comment #10:
|
Simon Goldschmidt <goldsimon> |
Fri 01 Jun 2007 12:44:44 PM UTC, comment #9:
|
Kieran Mansley <kieranm> |
Fri 01 Jun 2007 12:35:54 PM UTC, comment #8:
|
Jonathan Larmour <jifl> |
Fri 01 Jun 2007 11:51:14 AM UTC, comment #7:
|
Simon Goldschmidt <goldsimon> |
Fri 01 Jun 2007 11:36:05 AM UTC, comment #6: Just to clarify something from the original submission that I mentioned on the mailing list, the semaphore is not waited on only once. It is waited also in this part of netconn_write:
|
Jonathan Larmour <jifl> |
Fri 01 Jun 2007 10:13:33 AM UTC, comment #5: To be honest, I don't know. I have talk about "close" problems in http://savannah.nongnu.org/bugs/?19157, but I never gone in depth to study the tcp part (and I'm not very pleased of my patch, even if, experimentaly, it works). It "seems" better to "close" before "delete", but the CVS HEAD code doesn't do it...
|
Frédéric Bernon <fbernon> |
Fri 01 Jun 2007 09:38:38 AM UTC, comment #4:
|
Simon Goldschmidt <goldsimon> |
Fri 01 Jun 2007 08:38:34 AM UTC, comment #3: Note that netconn_close is not used at socket layer. But if you use it, you have to be sure that the tcp_close have success.
|
Frédéric Bernon <fbernon> |
Fri 01 Jun 2007 08:25:18 AM UTC, comment #2:
|
Simon Goldschmidt <goldsimon> |
Fri 01 Jun 2007 07:48:21 AM UTC, comment #1: Hmm, the state variable would then either need to be an atomic (which I don't think we really want to start requiring) or protected in some way. Perhaps we could use the semaphore itself in the other direction to signal that we want to be told.
|
Kieran Mansley <kieranm> |
Thu 31 May 2007 04:43:18 PM UTC, original submission:
In api_msg.c, sent_tcp() signals the conn->sem semaphore every time something is sent. But the sem is only waited for when ERR_MEM is returned (either if there is not enough memory or the TCP window is full).
|
Simon Goldschmidt <goldsimon> |
Depends on the following items: None found
Items that depend on this one: None found
There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.
Follow 7 latest changes.
Date | Changed by | Updated Field | Previous Value | => | Replaced by |
---|---|---|---|---|---|
2007-08-22 | goldsimon | Status | Ready For Test | Fixed | |
Open/Closed | Open | Closed | |||
2007-07-13 | kieranm | Planned Release | 1.3.0 | ||
2007-07-01 | goldsimon | Attached File | - | Added lwip_close.patch, #13218 | |
2007-06-21 | goldsimon | Status | In Progress | Ready For Test | |
2007-06-19 | goldsimon | Status | Confirmed | In Progress | |
Assigned to | None | goldsimon |
Powered by Savane 3.13-f8d8.
Corresponding source code
Since this seems to be working, closing it as fixed :-)