LWIP mailbox size configuration

[pstolarz]

Description of defect

Header features/lwipstack/lwipopts.h enables LWIP mailboxes size configuration, which are there defined as follows:

#define TCPIP_MBOX_SIZE             MBED_CONF_LWIP_TCPIP_MBOX_SIZE
#define DEFAULT_TCP_RECVMBOX_SIZE   MBED_CONF_LWIP_DEFAULT_TCP_RECVMBOX_SIZE
#define DEFAULT_UDP_RECVMBOX_SIZE   8
#define DEFAULT_RAW_RECVMBOX_SIZE   8
#define DEFAULT_ACCEPTMBOX_SIZE     8

Moreover TCP/IP thread mailbox size (MBED_CONF_LWIP_TCPIP_MBOX_SIZE) and TCP receive mailbox size (MBED_CONF_LWIP_DEFAULT_TCP_RECVMBOX_SIZE) may be defined via features/lwipstack/mbed_lib.json, so they are part of mbed-os user configuration interface. (Why only these, not all ?)

The issue is: any configuration here is completely ignored (see sys_mbox_new() routine's queue_sz argument handling in features/lwipstack/lwip-sys/arch/lwip_sys_arch.c). Mailbox size is always set to MB_SIZE (configurable via MBED_CONF_LWIP_MBOX_SIZE) regardless of its type.

Target(s) affected by this defect ?

All

Toolchain(s) (name and version) displaying this defect ?

n/a

What version of Mbed-os are you using (tag or sha) ?

mbed-os-5.14.2

What version(s) of tools are you using. List all that apply (E.g. mbed-cli)

n/a

How is this defect reproduced ?

n/a

[ciarmcom]

Internal Jira reference: https://jira.arm.com/browse/MBOTRIAGE-2453

[kjbracey]

Configurability was added recently by a third party: #11517

I would have thought it did at least do something to work around his issue...

Looking at it, the allocation is indeed fixed at MBED_CONF_LWIP_MBOX_SIZE (MB_SIZE) for every mailbox, so 11517 should have just added that config option.

Do you have a need for different per-mbox size?

[pstolarz]

Configurability was added recently by a third party: #11517

@kjbracey-arm the issue described by #11517 should be solved by my fix we recently talked about. The symptoms resemble problems I had. @balajicyp could you check #11976 patch. It should solve your problems with corrupted TCP packages. In case you had problems with TCP transmission slowed down, try increase mailbox & netbuf mem-pool sizes (MBED_CONF_LWIP_MBOX_SIZE, MBED_CONF_LWIP_NUM_NETBUF) from 8 to 16 for example.

Do you have a need for different per-mbox size?

At least the common mailbox size configuration via MBED_CONF_LWIP_MBOX_SIZE should be sufficient. I'd recommend to hide other params as not providing any value for now. The problem with more fine grained mailboxes config (TCPIP thread, UDP/TCP receive mailboxes, PCBs etc.) is it will require proper config for underlying mem-pools which usually is not an easy task. I'd recommend hide complexity exposed by large number of complex params to only few ones but properly chosen.

[balajicyp]

Hi @pstolarz @kjbracey-arm

I have verified with the patch #11976 (i.e using changes in #12046 ) and it indeed fix my problem of receiving out of order TCP packets while playing mp3 ( using chunk data format).

I did not face problems with TCP transmission and with the patch alone and leaving the MBOX_SIZE at 8 I did not see any problems.

I agree that MBED_CONF_LWIP_MBOX_SIZE configuration should have been sufficient for #11517 along with that MBED_CONF_LWIP_MEMP_NUM_TCPIP_MSG_INPKT to be configurable.

Other configuration changes such as MBED_CONF_LWIP_TCPIP_MBOX_SIZE and MBED_CONF_LWIP_DEFAULT_TCP_RECVMBOX_SIZE are not needed.

Thanks
Balaji.

[pstolarz]

Glad to hear it solved your issue.

MBED_CONF_LWIP_MEMP_NUM_TCPIP_MSG_INPKT param configures MEMP_TCPIP_MSG_INPKT pool used internally by call synchronizing thread (tcpip_thread) as a pool for processing incoming TCP messages in a synchronous way. Did you experience no memory errors on this pool? I doubt it should be exposed as a config param.

TCP received frames are provided by net-driver to the IP stack via pbuf mem-pools (MEMP_PBUF, MEMP_PBUF_POOL) and UDP packets via netbuf mem-pool (MEMP_NETBUF) and appropriate configs for these pools should be configurable I believe.

[balajicyp]

@pstolarz

MBED_CONF_LWIP_MEMP_NUM_TCPIP_MSG_INPKT param configures MEMP_TCPIP_MSG_INPKT pool used internally by call synchronizing thread (tcpip_thread) as a pool for processing incoming TCP messages in a synchronous way. Did you experience no memory errors on this pool? I doubt it should be exposed as a config param.

[Balaji] The MEMP_NUM_TCPIP_MSG_INPKT packet pool was running out of memory during peak receiving burst of data, again this I saw when I enabled some debug prints. This MEMP_NUM_TCPIP_MSG_INPKT is assigned to MBED_CONF_LWIP_MEMP_NUM_TCPIP_MSG_INPKT. Since the default value configured 8 is too low it could be configurable so that it could be increased to 16 based on other changes such as TCP window size etc.

TCP received frames are provided by net-driver to the IP stack via pbuf mem-pools (MEMP_PBUF, MEMP_PBUF_POOL) and UDP packets via netbuf mem-pool (MEMP_NETBUF) and appropriate configs for these pools should be configurable I believe.
[Balaji] I am ok with these parameters being configurable.

[pstolarz]

[Balaji] The MEMP_NUM_TCPIP_MSG_INPKT packet pool was running out of memory during peak receiving burst of data, again this I saw when I enabled some debug prints.

It was caused by corrupted tcpip_thread's mailbox. As I understand no out-of-mem errors occurred after applying #11976 fix.

[michalpasztamobica]

Hello, thanks a lot for this valuable analysis, @pstolarz and @balajicyp ! I think it diverged into three slightly different topics...

1. The originally reported issue regarded MBED_CONF_LWIP_DEFAULT_TCP_RECVMBOX_SIZE and MBED_CONF_LWIP_TCPIP_MBOX_SIZE. The corresponding LWIP's internal definitions are indeed not being used (it looks like they are used in netconn_alloc(), but in fact the argument is ignored in sys_mbox_new, as @pstolarz noticed).
   I believe however that these two values might have been set to be configurable because of this macro. If user decreases the MB_SIZE, they might also need to decrease TCPIP_MBOX_SIZE and DEFAULT_TCP_RECVMBOX_SIZE (and any other argument passed to sys_mbox_new for that matter, however I checked that at the moment only these two are in this situation).
   What we can do is remove the check OR do something like this in lwipopts.h:

#define TCPIP_MBOX_SIZE             MBED_CONF_LWIP_MBOX_SIZE
#define DEFAULT_TCP_RECVMBOX_SIZE   MBED_CONF_LWIP_MBOX_SIZE

and then remove the two corresponding configs from mbed_lib.json. Does this make sense?

2. MEMP_NETBUF and MEMP_PBUF are in fact created with this macro being used here and using the MEMP_NUM_NETBUF/MEMP_NUM_PBUF defines. Those in turn are set in lwipopts.h based on MBED_CONF_LWIP_NUM_NETBUF/PBUF. It seems then, that these two are already configurable and no work is needed here?

3. MBED_CONF_LWIP_MEMP_NUM_TCPIP_MSG_INPKT <- I still don't have clarity here, but if @balajicyp confirms that his issue was resolved without the need to modify this setting I would tend to agree with @pstolarz that unnecessary complexity should better be hidden from users.

@kjbracey-arm , your feedback on this would be much appreciated. If I understood the scope of the task, I can prepare a PR with the agreed changes.

[balajicyp]

[Balaji] The MEMP_NUM_TCPIP_MSG_INPKT packet pool was running out of memory during peak receiving burst of data, again this I saw when I enabled some debug prints.

It was caused by corrupted tcpip_thread's mailbox. As I understand no out-of-mem errors occurred after applying #11976 fix.

Yes I have not seen any issues after applying the #11976 fix.

[balajicyp]

Hello, thanks a lot for this valuable analysis, @pstolarz and @balajicyp ! I think it diverged into three slightly different topics...

The originally reported issue regarded MBED_CONF_LWIP_DEFAULT_TCP_RECVMBOX_SIZE and MBED_CONF_LWIP_TCPIP_MBOX_SIZE. The corresponding LWIP's internal definitions are indeed not being used (it looks like they are used in netconn_alloc(), but in fact the argument is ignored in sys_mbox_new, as @pstolarz noticed).
I believe however that these two values might have been set to be configurable because of this macro. If user decreases the MB_SIZE, they might also need to decrease TCPIP_MBOX_SIZE and DEFAULT_TCP_RECVMBOX_SIZE (and any other argument passed to sys_mbox_new for that matter, however I checked that at the moment only these two are in this situation).
What we can do is remove the check OR do something like this in lwipopts.h:

#define TCPIP_MBOX_SIZE MBED_CONF_LWIP_MBOX_SIZE
#define DEFAULT_TCP_RECVMBOX_SIZE MBED_CONF_LWIP_MBOX_SIZE

and then remove the two corresponding configs from mbed_lib.json. Does this make sense?

MEMP_NETBUF and MEMP_PBUF are in fact created with this macro being used here and using the MEMP_NUM_NETBUF/MEMP_NUM_PBUF defines. Those in turn are set in lwipopts.h based on MBED_CONF_LWIP_NUM_NETBUF/PBUF. It seems then, that these two are already configurable and no work is needed here?

MBED_CONF_LWIP_MEMP_NUM_TCPIP_MSG_INPKT <- I still don't have clarity here, but if @balajicyp confirms that his issue was resolved without the need to modify this setting I would tend to agree with @pstolarz that unnecessary complexity should better be hidden from users.
>[Balaji] I think we can remove this configuration MBED_CONF_LWIP_MEMP_NUM_TCPIP_MSG_INPKT, I have only seen issues of running out of memory without #11976 fix when streaming audio._

@kjbracey-arm , your feedback on this would be much appreciated. If I understood the scope of the task, I can prepare a PR with the agreed changes.