Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

LwIP: make TCPIP_THREAD_PRIO configurable #10980

Merged
merged 1 commit into from Aug 5, 2019

Conversation

@vmedcy
Copy link
Contributor

commented Jul 5, 2019

Description

Allow targets override TCPIP_THREAD_PRIO from features/lwipstack/mbed_lib.json.
Some PSoC 6 applications require TCPIP_THREAD_PRIO=osPriorityHigh to operate correctly.

Pull request type

[ ] Fix
[X] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

Release Notes

@ciarmcom ciarmcom requested review from ARMmbed/mbed-os-maintainers Jul 5, 2019
@ciarmcom

This comment has been minimized.

Copy link
Member

commented Jul 5, 2019

@vmedcy, thank you for your changes.
@ARMmbed/mbed-os-ipcore @ARMmbed/mbed-os-maintainers please review.

Copy link
Contributor

left a comment

I'm a bit wary that if you find yourself needing to change priority you may have worse problems - I usually find thread priority isn't the answer. But may as well let people fiddle.

Just editorial suggestions.

@@ -120,6 +120,10 @@
"help": "Stack size for lwip TCPIP thread",
"value": 1200
},
"tcpip-thread-priority": {
"help": "priority of lwip TCPIP thread",
"value": 24

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Jul 8, 2019

Contributor

I'd prefer this to be set by name - make it "osPriorityNormal". 24 is a bit vague.

Strings work fine here - a JSON string becomes C token(s). (If you wanted a C string you'd need to do "\"string\"").

@@ -120,6 +120,10 @@
"help": "Stack size for lwip TCPIP thread",
"value": 1200
},
"tcpip-thread-priority": {
"help": "priority of lwip TCPIP thread",

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Jul 8, 2019

Contributor

"Priority of..." (capital)

@artokin artokin added needs: work and removed needs: review labels Jul 8, 2019
@@ -111,7 +111,11 @@
#define TCPIP_THREAD_STACKSIZE LWIP_ALIGN_UP(MBED_CONF_LWIP_TCPIP_THREAD_STACKSIZE, 8)
#endif

#ifdef MBED_CONF_LWIP_TCPIP_THREAD_PRIORITY

This comment has been minimized.

Copy link
@bulislaw

bulislaw Jul 9, 2019

Member

Could we add a comment with the default value.

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Jul 9, 2019

Contributor

Actually, I'd kind of prefer not to have a default value here. This isn't a case where we need null in the JSON to mean "do something sensible".

We could just put the "osPriorityNormal" in the default JSON file, and have no #ifdef here.

This comment has been minimized.

Copy link
@balajicyp

balajicyp Jul 16, 2019

Contributor

Yes I am fine with "osPriorityNormal" in the default mbed_lib.json, with no #ifdef in header file.

balajicyp added a commit to balajicyp/mbed-os that referenced this pull request Jul 16, 2019
changes done based on review for
ARMmbed#10980
@balajicyp balajicyp referenced this pull request Jul 16, 2019
@vmedcy vmedcy force-pushed the vmedcy:tcpip-thread-priority branch from 42ec114 to 69782a4 Jul 17, 2019
@SeppoTakalo

This comment has been minimized.

Copy link
Contributor

commented Aug 1, 2019

I would like to have better understanding why this fixes any problems.

Having need to touch priorities to get some platforms to pass tests is a symptom of unstable platforms. Not a proper fix.

Network stack interfaces in Mbed OS effectively operate in event-based system. Unless other drivers consume 100% of CPU, then there should be eventually enough time to pass data through network stack. Having a need to raise network stack priority to high sounds like platform has busy-loops running on normal priority threads.
If those busy-loops are fixed, this need should go away, and system would throttle normally.

@balajicyp

This comment has been minimized.

Copy link
Contributor

commented Aug 1, 2019

Hi Seppo,

Cypress needs the LwIP TCP IP Thread priority configurable to set it to osPriorityHigh for some high throughput applicaitons such as iperf ( where the Throughput is sent at high rate of the order 40Mbps) from remote PC to Device.

We have several threads most importantly we have internal thread which gets signalled when SDIO IRQ is triggerred this is responsible for reading SDIO bus data from WLAN chip and this thread allocates its memory for the packet using pbuf ( LWIP buffer), this thread runs at osPriorityHigh, the LWIP threads gets data (UDP traffic) and it keeps the data in pbuf ( RAW pool) until the application reads the data, now if the application is slower running at lower priority then we see the LWIP TCP IP priroity running at < osPriorityHigh running out of its memory pool buffers.

Thanks
Balaji

@SeppoTakalo

This comment has been minimized.

Copy link
Contributor

commented Aug 2, 2019

Ah, so effectively you are just optimising the throughput of the system.

@SeppoTakalo

This comment has been minimized.

Copy link
Contributor

commented Aug 2, 2019

I'll start the CI tests.

@mbed-ci

This comment has been minimized.

Copy link

commented Aug 2, 2019

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 1
Build artifacts

@SeppoTakalo SeppoTakalo merged commit 937e791 into ARMmbed:master Aug 5, 2019
26 checks passed
26 checks passed
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/build-ARM Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage RTOS ROM(+0 bytes) RAM(+0 bytes)
Details
jenkins-ci/exporter Success
Details
jenkins-ci/greentea-test Success
Details
jenkins-ci/mbed2-build-ARM Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Success!
Details
travis-ci/docs Success!
Details
travis-ci/doxy-spellcheck Success!
Details
travis-ci/events Success! Runtime is 8692 cycles.
Details
travis-ci/gitattributestest Success!
Details
travis-ci/include_check Success!
Details
travis-ci/licence_check Success!
Details
travis-ci/littlefs Success! Code size is 8448B.
Details
travis-ci/psa-autogen Success!
Details
travis-ci/tools-py2.7 Success!
Details
travis-ci/tools-py3.5 Success!
Details
travis-ci/tools-py3.6 Success!
Details
travis-ci/tools-py3.7 Success!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.