buglwIP - 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: 

Since this seems to be working, closing it as fixed :-)

Simon Goldschmidt <goldsimon>
Group administrator
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>
Group administrator
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...

Can you just confirm me this is the "tcp_output(pcb);" inside the tcp_slowtmr, during the "poll connection", which "flush" the last datas from the pcb ?

Frédéric Bernon <fbernon>
Group Member
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>
Group Member
Wed 25 Jul 2007 07:32:09 PM UTC, comment #29: 

OK, checked it in:
- tcp_close doesn't care for the return value of tcp_output
- therefore, do_close_internal can call tcp_output until this returns ERR_OK, in which case the connection is regarded as closed and conn->pcb is set to NULL.

Frédéric and others interested, would you please check if this is OK now?

Simon Goldschmidt <goldsimon>
Group administrator
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).

With that change (don't care for tcp_output return value), api_msg.c:do_close_internal() would get simpler!

I'll change it to that, we can change it back later if you others disagree ;-)


Implementing linger on that is a little bit trickier, since when calling later, the connection could already have been deallocated. I think that would require more changes to tcp_close()...

Simon Goldschmidt <goldsimon>
Group administrator
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>
Group Member
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 :

Since you have change in netconn_delete (api_lib.c) the code like that :

   msg.function = do_delconn;
   msg.msg.conn = conn;
-  TCPIP_APIMSG(&msg);
+  tcpip_apimsg(&msg);

You shouldn't let in do_delconn(api_msg.c) the TCPIP_APIMSG_ACK, but replace it by a sys_mbox_post(msg->conn->mbox, NULL). Else, of course, the application thread stay blocked on do_delconn...

Right?

Frédéric Bernon <fbernon>
Group Member
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 :-)

> - "enum tcp_state old_state = conn->pcb.tcp->state;

if ((old_state == SYN_RCVD) || (old_state == ESTABLISHED) ||
(old_state == CLOSE_WAIT)) {"
Why do you use a local variable and a "if" in this case, and not a switch like for "err" some lines below?

Dunno. I can change that if you like.

> Even if it's no explicitly in our coding style, default is always the last clause in our switchs. Why isn't it the case here


Note the /*fall through*/ comment. 'default' needs nearly the same action as ERR_MEM and ERR_OK, so it has to be above them to get small code. But I'll change that (see the comment below)

> and why putting () around err_t case (like "case (ERR_OK):" ?


Dunno, it's our coding style at work. It's hard to prevent mixing the styles every time... I'll change that, too.

>  feel dangerous to determine if FIN is sent or not which "err" return value


I'll think of something for that.

Simon Goldschmidt <goldsimon>
Group administrator
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>
Group Member
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>
Group administrator
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).

I see a little problem: netconn_delete is called inside the sys_sem_wait(socksem)/sys_sem_signal(socksem).

I think it could be a problem because any new socket can't be created during the close. Comments in lwip_close say it's done because "/* We cannot allow multiple closes of the same socket. */". Since others API file like lwip_send can call by another thread in the same time, I think this protection is not enough, and give more problem.

I propose to change the code like this :

int
lwip_close(int s)
{
  struct lwip_socket *sock;

  LWIP_DEBUGF(SOCKETS_DEBUG, ("lwip_close(%d)\n", s));

  sock = get_socket(s);
  if (!sock) {
    return -1;
  }

  netconn_delete(sock->conn);

  sys_sem_wait(socksem);
  if (sock->lastdata) {
    netbuf_delete(sock->lastdata);
  }
  sock->lastdata   = NULL;
  sock->lastoffset = 0;
  sock->conn       = NULL;
  sock_set_errno(sock, 0);
  sys_sem_signal(socksem);
  return 0;
}

Comments?

Frédéric Bernon <fbernon>
Group Member
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.

Some remarks about do_close_internal:

- "enum tcp_state old_state = conn->pcb.tcp->state;
   if ((old_state == SYN_RCVD) || (old_state == ESTABLISHED) ||
      (old_state == CLOSE_WAIT)) {"
  Why do you use a local variable and a "if" in this case, and not a switch like for "err" some lines below?

- Even if it's no explicitly in our coding style, default is always the last clause in our switchs. Why isn't it the case here, and why putting () around err_t case (like "case (ERR_OK):" ?

- I feel dangerous to determine if FIN is sent or not which "err" return value: ERR_BUF & ERR_MEM. You should be sure that any future code will not change or add a error return value. More, if the MAC is "full", there is no "official" value to return (we have talk about that there is some weeks). Of course, to do something better, you should "break" the tcp_close. Or perhaps check if the pcb->state is now FIN_WAIT_1?


Frédéric Bernon <fbernon>
Group Member
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>
Group administrator
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.

I'll apply it like that if noone objects ;-)

(file #13218)

Simon Goldschmidt <goldsimon>
Group administrator
Thu 28 Jun 2007 10:58:47 AM UTC, comment #18: 

Anyone testing this?

Simon Goldschmidt <goldsimon>
Group administrator
Fri 22 Jun 2007 06:27:24 AM UTC, comment #17: 


> For such change, I think it would be good to first see a patch file before decide to check in.


I thought it's what we have discussed here (aside from the LWIP_TCPIP_CORE_LOCKING code)? And I think it's better to have a modification than to stay with the current code. Since noone has been commenting on this for nearly one month, I decided to check it in like that.

Simon Goldschmidt <goldsimon>
Group administrator
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>
Group Member
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().

In a next phase I'd change the close handling so that conn->sem can be completely removed.

Simon Goldschmidt <goldsimon>
Group administrator
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>
Group administrator
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.

That seems the best approach that would avoid race conditions, to me, but there are probably others.

NB I'm going on vacation this weekend, so won't be able to contribute more to this discussion after tonight.... I just say that in case you direct something to me and I don't answer :).


Jonathan Larmour <jifl>
Group Member
Fri 01 Jun 2007 04:02:20 PM UTC, comment #12: 


>No, the code on the api_msg side would change - the tcpip thread would not block.


So if you move the send buffer test into api_msg (which is processed in tcpip_thread), and there is no free buffer (or no mem), would you set a state 'someones_waiting' and signal the conn->sem? That would take away the race condition of what I've suggested (only signal conn->sem if someone is waiting for it).

Simon Goldschmidt <goldsimon>
Group administrator
Fri 01 Jun 2007 01:29:30 PM UTC, comment #11: 


> Might this cause a new set of problems where the tcpip_thread
> gets blocked waiting for memory to do operation A, while
> operation B that could otherwise go ahead isn't able to get into
> the tcpip_thread? The benefits might outweigh this downside
> though. Probably do in fact if there's an existing race that
> would also be solved.


No, the code on the api_msg side would change - the tcpip thread would not block. It would carry on, even if out of memory. It would release the waiting thread in sent_tcp. It does not wait itself.


Jonathan Larmour <jifl>
Group Member
Fri 01 Jun 2007 01:24:41 PM UTC, comment #10: 


>Might this cause a new set of problems where the tcpip_thread gets blocked waiting for memory to do operation A, while operation B that could otherwise go ahead isn't able to get into the tcpip_thread? The benefits might outweigh this downside though. Probably do in fact if there's an existing race that would also be solved.


I don't know if the benefits outweigh that. That's a thing I wouldn't like seeing integrated. That would mean one full TCP window could block all other connections.

In fact, the tcpip_thread can't even process incoming segments and thus would block forever...


Simon Goldschmidt <goldsimon>
Group administrator
Fri 01 Jun 2007 12:44:44 PM UTC, comment #9: 


> Alternatively, you can move the current send buffer test into
> the tcp/ip thread (so the caller thread effectively always
> waits, and its up to the tcpip thread to whether it resumes).
> We had already talked about this in fact, because the call to
> tcp_sndbuf is a race condition already.


Might this cause a new set of problems where the tcpip_thread gets blocked waiting for memory to do operation A, while operation B that could otherwise go ahead isn't able to get into the tcpip_thread?  The benefits might outweigh this downside though.  Probably do in fact if there's an existing race that would also be solved.


Kieran Mansley <kieranm>
Group Member
Fri 01 Jun 2007 12:35:54 PM UTC, comment #8: 


> Is it really good pratice to signal a binary semaphore more
> than once?


As per my post on the mailing list, for any systems where posting more than once is disallowed, we could insist the porter makes it allowed. Within the port-specific code it is easy for them to prevent it happening, whereas it would be hard for lwIP to maintain an abstraction that supported the possibility.

>>Alternatively, you can move the current send buffer test into
>>the tcp/ip thread (so the caller thread effectively always
>>waits, and its up to the tcpip thread to whether it resumes).
> That seems like the best solution. We also would need one
> semaphore less per connection. Only non-blocking mode would be
> slower since it would call into tcpip_thread although there is
> no space. But that's what we have to do with our current
> (multithreaded) implementation...


Certainly I think this would be the better approach (I should have said that!).

Jonathan Larmour <jifl>
Group Member
Fri 01 Jun 2007 11:51:14 AM UTC, comment #7: 


>I think it would be easiest to set down that lwIP expects binary semaphores only.


Is it really good pratice to signal a binary semaphore more than once?

>Alternatively, you can move the current send buffer test into the tcp/ip thread (so the caller thread effectively always waits, and its up to the tcpip thread to whether it resumes).


That seems like the best solution. We also would need one semaphore less per connection. Only non-blocking mode would be slower since it would call into tcpip_thread although there is no space. But that's what we have to do with our current (multithreaded) implementation...

Simon Goldschmidt <goldsimon>
Group administrator
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:
      while ((sndbuf = tcp_sndbuf(conn->pcb.tcp)) == 0) {
        sys_sem_wait(conn->sem);
        if (conn->err != ERR_OK) {
          goto ret;
        }

From what I can see the code here (and in sent_tcp) is treating the semaphore as a binary semaphore not a counting semaphore.

I think there would be race conditions with the suggested solution of something that tries to indicate that someone's waiting.

I think it would be easiest to set down that lwIP expects binary semaphores only.

Alternatively, you can move the current send buffer test into the tcp/ip thread (so the caller thread effectively always waits, and its up to the tcpip thread to whether it resumes). We had already talked about this in fact, because the call to tcp_sndbuf is a race condition already.


Jonathan Larmour <jifl>
Group Member
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>
Group Member
Fri 01 Jun 2007 09:38:38 AM UTC, comment #4: 


>Note that netconn_close is not used at socket layer.


That's funny. Does netconn_delete (used in lwip_close) ensure the tcp_pcb is put into time wait state? And if, why is netconn_close different from netconn_delete?

Simon Goldschmidt <goldsimon>
Group administrator
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.

If during the close, there is a ERR_MEM, to avoid to consumne cycles to retry, you wait the next "poll_tcp"...




Frédéric Bernon <fbernon>
Group Member
Fri 01 Jun 2007 08:25:18 AM UTC, comment #2: 


>Is it waiting until memory becomes available again?


Mostly, I think. Although I don't know why it is waited for in netconn_close()...

Simon Goldschmidt <goldsimon>
Group administrator
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.

Getting a better understanding of what this semaphore is being used to achieve would be good.  It may be that there's a better way of doing it altogether, and trying to fix up the semaphore is a waste of time.  Is it waiting until memory becomes available again?

Kieran Mansley <kieranm>
Group Member
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).
That way, if you only send small data and have enough memory, the semaphore can be signalled into heaven ;-)

Proposed change: include a state variable like "someone's waiting" and only signal the semaphore when someone is waiting for more data to show.

Simon Goldschmidt <goldsimon>
Group administrator

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attached Files
file #13218:  lwip_close.patch added by goldsimon (11KiB - text/x-patch)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by jifl (Posted a comment)
  • -email is unavailable- added by fbernon (Posted a comment)
  • -email is unavailable- added by kieranm (Posted a comment)
  • -email is unavailable- added by goldsimon (Submitted the item)
  •  

    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 StatusReady For Test Fixed
        Open/ClosedOpen 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 StatusIn Progress Ready For Test
    2007-06-19 goldsimon StatusConfirmed In Progress
        Assigned toNone goldsimon

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code