tasklwIP - A Lightweight TCP/IP stack - Tasks: task #7490, Add return value to sys_mbox_post

 
 

You are not allowed to post comments on this tracker with your current authentication level.

task #7490: Add return value to sys_mbox_post

Submitted by:  Simon Goldschmidt <goldsimon>
Submitted on:  Sun 25 Nov 2007 10:42:10 AM UTC  
 
Category:  None Should Start On:  Sun 25 Nov 2007 12:00:00 AM UTC
Should be Finished on:  Sun 25 Nov 2007 12:00:00 AM UTC Priority:  5 - Normal
Status:  Done Privacy:  Public
Percent Complete:  100% Assigned to:  Frédéric Bernon <fbernon>
Open/Closed:  Closed Planned Release:  1.3.0
Effort:  0.00

( Jump to the original submission)

Sun 13 Jan 2008 12:20:10 PM UTC, comment #43: 

I close this task, and open a new one for the design problem around the call of TCP_EVENT_RECV.

Frédéric Bernon <fbernon>
Project MemberIn charge of this item.
Sat 12 Jan 2008 05:54:39 PM UTC, comment #42: 

I'm agree, but I think it's a separate issue: we could close this one, and open another one for tcp_in.c changes.

(Perhaps add a pbuf* in the tcp_pcb where we could keep the data to pass to TCP_EVENT_RECV when this one return an error, and we retry to pass it to the next tcp_input call?)

Frédéric Bernon <fbernon>
Project MemberIn charge of this item.
Sat 12 Jan 2008 05:00:24 PM UTC, comment #41: 

> >check the good behavior for recv_tcp in api_msg.c
>
> Perhaps I'm wrong, but the err_t returns by recv_tcp is not
> checked in tcp_input (tcp_in.c, line 308), so, this is
> sys_mbox_post (and not "trypost") which have to be call


Hmm, but then the whole stack can lock up when one TCP netconn  recvmbox is full, which is what we wanted to fix with this check.

So either we have to solve this in tcp_in.c or set the netconn that missed a packet to a fatal error mode (and maybe already abort the pcb?) so that the connection is aborted.

Any thoughts about that?

Simon Goldschmidt <goldsimon>
Project Administrator
Sat 12 Jan 2008 03:40:51 PM UTC, comment #40: 

>add some sanity checks ?


I don't think we could do some real checks on MBOX_SIZE defines, so....

>complete documentation (sys_arch.txt, wiki...)


I think that sys_arch.txt is good, I will update the wiki.

To my point of view, and except if I miss something, I think we can close this task. Any objections ?

Frédéric Bernon <fbernon>
Project MemberIn charge of this item.
Sat 12 Jan 2008 03:35:16 PM UTC, comment #39: 

>rename netconn::sem per netconn::op_completed (note I thought it was more in the lwip coding style to keep objects type name in variables names - recvmbox, acceptmbox, etc...)


check in

>check the good behavior for recv_tcp in api_msg.c


Perhaps I'm wrong, but the err_t returns by recv_tcp is not checked in tcp_input (tcp_in.c, line 308), so, this is sys_mbox_post (and not "trypost") which have to be call

Frédéric Bernon <fbernon>
Project MemberIn charge of this item.
Sat 12 Jan 2008 01:58:35 PM UTC, comment #38: 

Although I'm sure GCC can optimize that, hopefully other people's compilers will be able to too.

Jonathan Larmour <jifl>
Project Member
Fri 11 Jan 2008 11:55:41 PM UTC, comment #37: 

I check in the patch with something like you have proposed. I hope it good for you...

Frédéric Bernon <fbernon>
Project MemberIn charge of this item.
Fri 11 Jan 2008 09:59:07 AM UTC, comment #36: 

I'm ok with the patch, although it would be easy to optimise the common case where all the values will be the same (especially true for the default case, and in cases where the underlying mbox does not support different sizes anyway). So add something like:

#if ((DEFAULT_RAW_RECVMBOX_SIZE == DEFAULT_UDP_RECVMBOX_SIZE) && \
  (DEFAULT_UDP_RECVMBOX_SIZE == DEFAULT_TCP_RECVMBOX_SIZE))
/* All values identical, so just choose one of them */
# define RECVMBOX_SIZE DEFAULT_TCP_RECVMBOX_SIZE
#else
# define RECVMBOX_SIZE size
#endif

And then surround the added elements of the patch with:
#if (RECVMBOX_SIZE == size)
...
#endif

Thoughts?

Jonathan Larmour <jifl>
Project Member
Thu 10 Jan 2008 10:13:19 PM UTC, comment #35: 

To do for this task:

- rename netconn::sem per netconn::op_completed (note I thought it was more in the lwip coding style to keep objects type name in variables names - recvmbox, acceptmbox, etc...)

- check the good behavior for recv_tcp in api_msg.c

- add some sanity checks ?

- complete documentation (sys_arch.txt, wiki...)

Frédéric Bernon <fbernon>
Project MemberIn charge of this item.
Thu 10 Jan 2008 10:10:07 PM UTC, comment #34: 

>op_completed as sem name


I check in with just "sem" (the patch file #14778), I will rename in the week end.

>RECVMBOX_SIZE


proposed patch file attached. Note it increase a little bit the code size, so...

(file #14780)

Frédéric Bernon <fbernon>
Project MemberIn charge of this item.
Thu 10 Jan 2008 07:43:07 AM UTC, comment #33: 

I like the idea of op_completed as sem name, it kind of tells the user what it does...

> Any objections to have a RECVMBOX_SIZE per netconn type ?


Not from me, if doesn't increase the size too much, I think, and it has some benefits. Although making TCP recvmboxes big enough to never overflow can't really be achieved: the mbox would have to take 32K pbufs for a 32K receive window, so it would have to be 128K big...

Simon Goldschmidt <goldsimon>
Project Administrator
Thu 10 Jan 2008 03:27:34 AM UTC, comment #32: 

Both those patches look fine to me.

I think "acksem" might make an unwitting reader think it has something to do with TCP ACKs, even though it clearly doesn't.

Just keeping it as "sem" would be ok with me. If you did want to change it may I suggest something like "op_completed". So code would sys_sem_wait(...->op_completed ...) and sys_sem_signal(...->op_completed) which sort of reads correctly. But I have no strong opinion, just trying to throw ideas around.

Jonathan Larmour <jifl>
Project Member
Wed 09 Jan 2008 11:31:55 PM UTC, comment #31: 

>replace netconn::mbox per a sys_sem_t


I attach a patch file for that. I thought to that netconn::acksem could be better than netconn::sem. Comments?

(file #14778)

Frédéric Bernon <fbernon>
Project MemberIn charge of this item.
Wed 09 Jan 2008 10:20:40 PM UTC, comment #30: 

>a unblocking tcpip_callback


I attach a patch file for that

>For the recv_tcp, I thought we could replace DEFAULT_RECVMBOX_SIZE per DEFAULT_TCP_RECVMBOX_SIZE, DEFAULT_UDP_RECVMBOX_SIZE and DEFAULT_RAW_RECVMBOX_SIZE. Like this, we could define tcp::recvmbox large enought to never block, and other to optimize sizes.


Any objections to have a RECVMBOX_SIZE per netconn type ?

(file #14776)

Frédéric Bernon <fbernon>
Project MemberIn charge of this item.
Sun 06 Jan 2008 07:47:55 PM UTC, comment #29: 

To do for this task:

- a unblocking tcpip_callback

- replace netconn::mbox per a sys_sem_t

- check the good behavior for recv_tcp in api_msg.c

- add some sanity checks ?

- complete documentation (sys_arch.txt, wiki...)

For the recv_tcp, I thought we could replace DEFAULT_RECVMBOX_SIZE per DEFAULT_TCP_RECVMBOX_SIZE, DEFAULT_UDP_RECVMBOX_SIZE and DEFAULT_RAW_RECVMBOX_SIZE. Like this, we could define tcp::recvmbox large enought to never block, and other to optimize sizes.

Frédéric Bernon <fbernon>
Project MemberIn charge of this item.
Sun 06 Jan 2008 07:40:30 PM UTC, comment #28: 

(from private emails)

Frédéric BERNON wrote:

> I'm not sure to understand why you set conn::pcb.tcp to NULL before each
> call to netconn_free. Why don't you set it inside netconn_free ?


Simon answers:
My idea was to make sure that the pcb is deleted outside the
netconn_free function (since it is called from API thread also). Because
of that, I included the assert for pcb==NULL to make sure future
developers think of freeing the pdb before calling netconn_free (the
name doesn't really tell that the pcb is not freed). Of course it is not
really needed now, but I think it helps to avoid errors in the future.

Frédéric Bernon <fbernon>
Project MemberIn charge of this item.
Sat 05 Jan 2008 09:11:35 PM UTC, comment #27: 

Ok, it's check in. Simon, feel free to work on accept_function by adding something like a "netconn_delete".

Frédéric Bernon <fbernon>
Project MemberIn charge of this item.
Sat 05 Jan 2008 08:49:23 PM UTC, comment #26: 

>tcp_callback: I really don't have a strong opinion on this, you can just add the block/nonblock argument if you like.


I will check in as blocking in a first time (to let you work on the deleting netconn if you can)

>First we have to look through the code if it really is that way (so it wouldn't work)


A fast look to TCP_EVENT_RECV (tcp_in.c line 308) calls seems to indicate that the return value is not "directly" checked, so, I don't know if the data is acknowledged somewhere else.

In the patch I will check in, I will restore the recv_tcp...

Frédéric Bernon <fbernon>
Project MemberIn charge of this item.
Sat 05 Jan 2008 07:50:52 PM UTC, comment #25: 

tcp_callback: I really don't have a strong opinion on this, you can just add the block/nonblock argument if you like.

deleting netconns: do you check in the rest of the patch? If you have the time, you can add it, if not, check it in with the @todo like in the patch and I'll add it.

> yes, I thought about that, but I try to follow suggestions from
> comment #9. But I think you're right, and we should not use post > in this recv_tcp.


I know, this came to my mind just today... First we have to look through the code if it really is that way (so it wouldn't work), then we have to decide for an alternative: Asserting would be a bad idea, but considering the goal of tcp (reliable data stream), the only option here would be to abort the connection. But then, the NULL pointer marking the conncetion as closed can't be posted to the mbox (since it is full) - and setting conn->err to a fatal error might not be thread safe...

Simon Goldschmidt <goldsimon>
Project Administrator
Sat 05 Jan 2008 07:23:39 PM UTC, comment #24: 

>to sys_arch.txt: To prevent people porting lwIP to get confused about the 'size' argument, I'd add something like 'Elements stored in mboxes are pointers.' to the description of sys_mbox_new() so that people can calculate the actual memory size needed. Also, 0 = max or 0 = default should be mentioned somewhere, I think (since TCPIP_MBOX_SIZE is 0 by default).


ok, I change it (when I will attach a new patch file, tell me if it 's better)

>I would rather include a new function (tcpip_trycallback or something), that way, it is not linked when not used.


I'm not sure: since tcpip_callback is used as blocking in part of lwip like sockopt or ppp, and if you need tcpip_trycallback in your driver, I'm afraid the code size increase would be larger than add a "tcpip_callback_with_block(void (*f)(void ctx), void ctx, u8_t block)", and add a "#define tcpip_callback(f,c) tcpip_callback_with_block(f,c,1)"

>Resetting the pcb callbacks, calling tcp_close (or tcp_abort), freeing conn->mbox, conn->recvmbox and conn should be done. This can be done in an extra function to be combined with do_close_internal() and netconn_delete() (draining the mboxes doesn't hurt).


ok, I will try to add it (or if you have time...)

>About using sys_mbox_trypost in api_msg.c:recv_tcp(): Does this really work? We should take a look at tcp_in.c to see if retransmitted data is given to the application again on receiving a second time... after all I think tcp might already have ACKed that data... ??


yes, I thought about that, but I try to follow suggestions from comment #9. But I think you're right, and we should not use post in this recv_tcp.

Frédéric Bernon <fbernon>
Project MemberIn charge of this item.
Sat 05 Jan 2008 11:17:33 AM UTC, comment #23: 

to sys_arch.txt: To prevent people porting lwIP to get confused about the 'size' argument, I'd add something like 'Elements stored in mboxes are pointers.' to the description of sys_mbox_new() so that people can calculate the actual memory size needed. Also, 0 = max or 0 = default should be mentioned somewhere, I think (since TCPIP_MBOX_SIZE is 0 by default).

to 1/:
I would rather include a new function (tcpip_trycallback or something), that way, it is not linked when not used.

to 4/:
Resetting the pcb callbacks, calling tcp_close (or tcp_abort), freeing conn->mbox, conn->recvmbox and conn should be done. This can be done in an extra function to be combined with do_close_internal() and netconn_delete() (draining the mboxes doesn't hurt).

About using sys_mbox_trypost in api_msg.c:recv_tcp(): Does this really work? We should take a look at tcp_in.c to see if retransmitted data is given to the application again on receiving a second time... after all I think tcp might already have ACKed that data... ??

Other than that, I think the patch is fine.

Simon Goldschmidt <goldsimon>
Project Administrator
Sat 05 Jan 2008 02:12:20 AM UTC, comment #22: 

I attach a first partial patch file:

"size" parameter is added to sys_mbox_new, err_t sys_mbox_trypost(sys_mbox_t mbox, void *msg) is declared and documented, and sys_mbox_trypost is used, but some points need to be checked :

1/ If tcpip_callback is non-blocking, we will got problems with lwip_get/set sockopt, and with PPP, so, perhaps we could add a "block" parameter to this function if someone need to used is non-blocking (which is not the case in CVS code)

2/ tcpip_input used "trypost", other in tcpip.c used "post"

3/ calls in err_tcp are blocking: I didn't check (yet), but on error, these "NULL" messages could be important. Have to investigate

4/ in accept_function, if "trypost" failed, we have to close the new pcb/netconn. Something like a "netconn_free" could be useful. I put a @todo to not forget it

5/ netconn::mbox have to be change with a semaphore (have to use sys_arch_sem_wait and sys_sem_signal)

(file #14748)

Frédéric Bernon <fbernon>
Project MemberIn charge of this item.
Wed 12 Dec 2007 08:27:19 PM UTC, comment #21: 

> - if we do a "non-blocking+retry" in get/setsockopt with
> tcpip_callback on errors ? Same for netconn_gethostbyname? Or is
> it to the application thread to do it ?


Blocking and try again? So return ERR_MEM when the mailbox is full? I still think the mailbox will be too full most of the time when receiving too fast, this could confuse ported socket applications that presume an error then... I'd prefer blocking.

> - if we replace netconn::mbox by a sys_sem_t ?


That would be good.

> - if we add a "size" parameter to sys_mbox_new to let the user
> tune its mbox (we a DEFAULT_MBOX_SIZE for netconn::mbox,
> DEFAULT_RECVMBOX_SIZE for netconn::recvmbox & acceptmbox, and
> DEFAULT_TCPIPMBOX_SIZE ) ?


That would also be a good idea, I think! Since we trypost to recvmbox and acceptmbox, this can greatly help to reduce the memory footprint (especially combined with SO_RECVBUF, which could free the mbox and reallocate it with a smaller size - as long as the socket is still unused).

Simon Goldschmidt <goldsimon>
Project Administrator
Wed 12 Dec 2007 06:11:53 PM UTC, comment #20: 

Just to be sure, can you tell me :

- if we do a "non-blocking+retry" in get/setsockopt with tcpip_callback on errors ? Same for netconn_gethostbyname? Or is it  to the application thread to do it ?

- if we replace netconn::mbox by a sys_sem_t ?

- if we add a "size" parameter to sys_mbox_new to let the user tune its mbox (we a DEFAULT_MBOX_SIZE for netconn::mbox, DEFAULT_RECVMBOX_SIZE for netconn::recvmbox & acceptmbox, and DEFAULT_TCPIPMBOX_SIZE ) ?

Frédéric Bernon <fbernon>
Project MemberIn charge of this item.
Tue 11 Dec 2007 07:41:51 PM UTC, comment #19: 

> I think it's best just to make it non-blocking with error code if blocked, and let the caller decide.


Probably best.

Given the consensus we seem to have on this: why wait until after 1.3.0?

Simon Goldschmidt <goldsimon>
Project Administrator
Mon 10 Dec 2007 07:24:22 PM UTC, comment #18: 

I agree for blocking versus non-blocking, although arguably tcpip_input may as well free the pbuf itself.

tcpip_callback is indeed a bit of a dilemma. Personally I'd use it more from interrupt context than otherwise, but it's possible to think of situations when it needs to be reliable i.e. blocking. We could make two versions, but to be honest, I think it's best just to make it non-blocking with error code if blocked, and let the caller decide. Worst case they just loop polling it, or loop sleeping and polling it.

If we find people wanting a blocking version in future it's easy to add.

Jonathan Larmour <jifl>
Project Member
Mon 10 Dec 2007 07:14:35 PM UTC, comment #17: 

Well, it would mainly be:

non-blocking (try) for all calls in api_msg.c (plus TCPIP_APIMSG_ACK) an sockets.c.

In tcpip.c, both versions would be used:
- non-blocking for tcpip_input (return error value -> driver must free pbuf)
- and blocking for tcpip_netifapi, tcpip_apimsg, tcpip_timeout

only tcpip_callback is a real question: when blocking, it can't be used from interrupt context!

Simon Goldschmidt <goldsimon>
Project Administrator
Mon 10 Dec 2007 06:42:48 PM UTC, comment #16: 

(Re #15):

At first I wanted to disagree with Jonathan and Frederic, because in the two RTOS's I could think of, you could block only on the read side of a message queue... But it looks like in Linux there is functionality of blocking on the send side as well.

So, there is precedent for that view. Although, it seems that it will require sys_arch to allocate an mbox and a semaphore for those RTOS's that cannot block a send on a message queue.

I think it would be a good exercise to look at all the mbox_post's in the lwIP code and see whether there is a consistent thing to do on a failed post. Should it be block? Should it be drop? Should it be sleep+spin until success?

Jared Grubb <jgrubb>
Project Member
Fri 07 Dec 2007 01:54:45 PM UTC, comment #15: 

I just realised that while I said it's up to the caller to decide what to do if the call fails, blocking is difficult. Unless you always accompany an mbox by a semaphore, there's no way for the caller to block waiting for space to appear in the mbox.

So I think as per Frederic's comment #10, having two functions, sys_mbox_post (which blocks) and sys_mbox_trypost (which doesn't, and instead returns an error code as needed), and the caller can choose which behaviour they want. Since it is likely the implementations of these functions are mostly the same code (either in sys_arch or the OS) this shouldn't be that bad for code space.

Jonathan Larmour <jifl>
Project Member
Tue 27 Nov 2007 12:55:34 PM UTC, comment #14: 

Someone asked my opinion...

 - I don't think it should block, and so the sensible course is to return an error to the caller, and they can then decide to block if necessary.

 - I'm neutral on whether we should have a size argument, but either way I think it's a separate issue and we should do it later if we need it.

 - I think this API change (adding a return value) should be delayed till after 1.3.0

Kieran Mansley <kieranm>
Project Member
Tue 27 Nov 2007 07:40:09 AM UTC, comment #13: 

> netconn::acceptmbox (4-5 most of time)


A good point to implement the listen backlog?

> I think it's simpler just to say it never blocks.


I think so, too. So simply add a return value :-)

But after 1.3.0, for me.

Simon Goldschmidt <goldsimon>
Project Administrator
Tue 27 Nov 2007 04:44:58 AM UTC, comment #12: 

>Whether there is a size advised on creation or not is a separate issue, although I think it would be good (as long as it is not a mandatory size - some OS mailboxes are not configurable).


Yes, I think more to a way to configure these queues with different sizes:

netconn::acceptmbox (4-5 most of time)
netconn::recvmbox (large if some fast protocols are used)
tcpip.c::mbox (large enought to avoid any "blocked" states)

>Whether for 1.3 or not, I have no strong opinion, except that I know we're already suggesting changes for after 1.3 that will break ports. So I think if we ever want 1.3 to get out, we may just have to accept that 1.4 will break ports again.


Yes, it's true

Frédéric Bernon <fbernon>
Project MemberIn charge of this item.
Mon 26 Nov 2007 10:43:14 PM UTC, comment #11: 

I think it's simpler just to say it never blocks. What Jared said in comment #7 makes the most sense to me.

Whether there is a size advised on creation or not is a separate issue, although I think it would be good (as long as it is not a mandatory size - some OS mailboxes are not configurable).

Whether for 1.3 or not, I have no strong opinion, except that I know we're already suggesting changes for after 1.3 that will break ports. So I think if we ever want 1.3 to get out, we may just have to accept that 1.4 will break ports again.

Jonathan Larmour <jifl>
Project Member
Mon 26 Nov 2007 08:43:12 PM UTC, comment #10: 

>- multiple places in api_msg.c: conn->mbox (used like semaphore -> we could convert it to a semaphore!) should not block, since tcpip_thread would be blocked


Agree, and it reduce a little bit the need of a "size" parameter for sys_mbox_new. But if we change sys_mbox_new, I really would like to add this size parameter, in the same idea that we have add "name" and "stacksize" to sys_thread_new (port maintainer could decide to ignore them).

>We would need a flag (block or don't block) and a return value for that. Or we only take the return value and never block...


Why not another function (in the same spirit we have add sys_arch_mbox_tryfetch)? For a non-blocking function, error codes could be ERR_OK if it post the message, or something like ERR_WOULDBLOCK or ERR_RETRY ?

But, is it something for 1.3.0, or post-1.3.0 ?

Frédéric Bernon <fbernon>
Project MemberIn charge of this item.
Mon 26 Nov 2007 07:33:18 PM UTC, comment #9: 

I've put together the places where sys_mbox_post is used:

- multiple places in api_msg.c: conn->mbox (used like semaphore -> we could convert it to a semaphore!) should not block, since tcpip_thread would be blocked

- recv_raw(), recv_udp(), recv_tcp(): conn->recvmbox: may not block since tcpip_thread would be blocked
- err_tcp(), accept_function(): conn->acceptmbox: may not block since tcpip_thread would be blocked

- tcpip_input(): post work to tcpip_thread may not block since maybe called from interrupt

tcpip_callback(), tcpip_timeout(), tcpip_apimsg(), tcpip_netifapi(): post work to tcpip_thread (may wait since called from application thread?)

So in fact those are 6 places where sys_mbox_post may not block and 4 where it may block (if we convert conn->mbox to a sem). I think those 6 places don't make the code that big... And we would gain a lot of safety, in my opinion...

We would need a flag (block or don't block) and a return value for that. Or we only take the return value and never block...

Jonathan, Kieran, what is your opinion about this?

Simon Goldschmidt <goldsimon>
Project Administrator
Mon 26 Nov 2007 05:20:39 PM UTC, comment #8: 

In our case, the prevention through TCP window is not safe because we use lwIP for VNC trafic, which typically uses small packets (region refresh requests, particularly when moving a window with permanent refresh).  I could very well just drop the packet myself from the interrupt handler, or disable the interrupt while the box is somehow full, but there's currently no clean way to do this from our code (the mbox is static).

Samuel Thibault <sthibaul>
Mon 26 Nov 2007 05:20:17 PM UTC, comment #7: 

The "poster" of the data is the only one who can know what SHOULD happen if the mbox post fails -- and the correct behavior will depend on which mbox it is. Therefore, I don't think the problem can be solved inside the sys_arch file.

Simon's idea of returning an error code is probably the safest option.

Jared Grubb <jgrubb>
Project Member
Mon 26 Nov 2007 05:09:07 PM UTC, comment #6: 

> I actually have a problem with this: I get the network packets
> from an interrupt handler, which is not allowed to wait, and I've
> currently no way to let the interrupt handler know that it
> shouldn't call tcpip_input but just drop the packet.


If not solving this with giving sys_mbox_post a return value, we could always implement a counter that sets a limit on the packet-input mbox. That way, sys_mbox_post can assert if the mbox is full.

I don't think waiting until there is free space in the mbox is the correct implmementation: What if a UDP/TCP recv mbox is full but the application has different things to do than receiving data? This would block the whole tcpip_thread. So asserting should be the correct thing to do, but that can be left out in a release build! :-(

Simon Goldschmidt <goldsimon>
Project Administrator
Mon 26 Nov 2007 04:58:34 PM UTC, comment #5: 
Frédéric Bernon <fbernon>
Project MemberIn charge of this item.
Mon 26 Nov 2007 04:38:35 PM UTC, comment #4: 

I actually have a problem with this: I get the network packets from an interrupt handler, which is not allowed to wait, and I've currently no way to let the interrupt handler know that it shouldn't call tcpip_input but just drop the packet.

Samuel Thibault <sthibaul>
Sun 25 Nov 2007 12:34:01 PM UTC, comment #3: 

>The problem that remains is that tcpip_input() can fail which results in memory leaks


It's coherent with the current paradigm: sys_mbox_post should never failed, and if the mbox is full, it should retry (inside sys_mbox_post implementation).

I really thought it should be done after 1.3.0 to do a sys_arch redesign where it's need. In a previous chat (I can't remember where it was), I remember that someone (Jonathan perhaps) ask to also redesign sys_sem_xxx functions.

Frédéric Bernon <fbernon>
Project MemberIn charge of this item.
Sun 25 Nov 2007 12:19:02 PM UTC, comment #2: 

Just thought this is a rather small change, so could go in to 1.3.0...

Having a size parameter should be good to reduce memory usage of the stack (UDP mboxes can be smaller than TCP mboxes with a big recv window).

The problem that remains is that tcpip_input() can fail which results in memory leaks. Of course, this can be solved by a counter also (like SO_RCVBUF) but that adds code size, too!

On the other hand, solving this with a counter has the advantage that some space in the mbox is left for application tasks communicating with tcpip_thread. The mbox being full in that case locks up the stack anyway...

Simon Goldschmidt <goldsimon>
Project Administrator
Sun 25 Nov 2007 12:10:36 PM UTC, comment #1: 

I'm not agree, and should be done if we give a "size" parameter to sys_mbox_new (having a same static size for conn::mbox,  conn::recvmbox, and tcpip.c::mbox is an error). SO_RCVBUF already give a first solution to this problem.

But in a general way, I think we should do all the sys_arch changes in the same times (mutex, ticks...). So, I think it should be done in "post 1.3.0". In all cases, there will be other port breakages, and we can't do all of them in the same release.

Frédéric Bernon <fbernon>
Project MemberIn charge of this item.
Sun 25 Nov 2007 10:42:10 AM UTC, original submission:  

Various bug reports (and my own experience) have shown that a return value for sys_mbox_post might be useful to show whether the mailbox was full.

I have read on lwIP that (at least one user) has implemented mboxes to wait for free space when they are full. In most places, this locks up the stack.

Also, if the tcpip-mbox is full, input packets will leak sice noone knows whether they could be put on the mbox or not. And inside the mbox function, you don't know what type of element you have so you cannot free it safely.

To sum it up, I would give sys_mbox_post a true/false return value and assert in most places, free a pbuf in other places. And I would do it now (pre-1.3.0) since it is not a big change for us but requires ports to be updated.

Simon Goldschmidt <goldsimon>
Project Administrator

 

Attached Files
file #14780:  RECVMBOX_SIZE.patch added by fbernon (4KiB - application/octet-stream)
file #14778:  netconn_sem.patch added by fbernon (9KiB - application/octet-stream)
file #14776:  tcpip_callback.patch added by fbernon (2KiB - application/octet-stream)
file #14748:  sys_mbox_trypost.patch added by fbernon (7KiB - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

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

    Do you think this task is very important?
    If so, you can add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

     

     

    Follow 16 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2008-01-13 fbernon Open/ClosedOpen => Closed
    2008-01-13 fbernon StatusNone => Done
        Percent Complete80% => 100%
        Assigned toNone => fbernon
    2008-01-12 fbernon Percent Complete70% => 80%
    2008-01-10 fbernon Attached File- => Added RECVMBOX_SIZE.patch, #14780
    2008-01-09 fbernon Attached File- => Added netconn_sem.patch, #14778
        Percent Complete60% => 70%
    2008-01-09 fbernon Attached File- => Added tcpip_callback.patch, #14776
        Percent Complete50% => 60%
    2008-01-06 fbernon Percent Complete40% => 50%
    2008-01-05 fbernon Percent Complete20% => 40%
    2008-01-05 fbernon Attached File- => Added sys_mbox_trypost.patch, #14748
        Percent Complete0% => 20%
        Planned ReleaseNone => 1.3.0
    2007-11-25 goldsimon Planned Release1.3.0 => None

    Back to the top


    Powered by Savane 3.5