patchlwIP - A Lightweight TCP/IP stack - Patches: patch #7885, modification of api modules to...

 
 

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

patch #7885: modification of api modules to support FreeRTOS-MPU (NO_SYS = 0)

Submitter:  Artem Pisarenko <jblackarty>
Submitted:  Wed 14 Nov 2012 05:08:13 AM UTC
   
 
Category:  sockets/netconn Priority:  5 - Normal
Status:  Done Privacy:  Public
Assigned to:  goldsimon Open/Closed:  Closed
Planned Release:  None

Jump to the original submission

Fri 21 Feb 2014 08:05:40 AM UTC, comment #8: 

Good catch, I hope I fixed it now.

Simon Goldschmidt <goldsimon>
Group administrator
Fri 21 Feb 2014 05:53:30 AM UTC, comment #7: 

I'm glad to see my contribution accepted.

I compared what you modified and found an error in netconn_gethostbyname() function:

if (addr == NULL) {
  addr = IP_ADDR_ANY;
}

Firstly, condition will never be true because of LWIP_ERROR() check above. Secondly, if it even would be, 'addr' will point to constant variable, followed with

*addr = msg->addr;


Also I found my own oversight of 'name' argument. Only pointer passed via 'msg', but it may point to stack-located string. So whole string must be passed.

Artem Pisarenko <jblackarty>
Thu 20 Feb 2014 06:51:19 PM UTC, comment #6: 

Done, thanks for the patch.
Had to cleab it up a bit, though, so I hope it still all works. At least the things I tested did... :-)

Simon Goldschmidt <goldsimon>
Group administrator
Sat 24 Nov 2012 09:41:22 AM UTC, comment #5: 

Actually I was able to avoid mess of #ifdef-else-endif insertions. Instead, replacements ans small additions was made with macros which are defined conditionally (in most places). The resulting code turned out to be not so bloated as expected.

Artem Pisarenko <jblackarty>
Fri 23 Nov 2012 05:27:17 PM UTC, comment #4: 

I haven't had time to review this patch yet, but here are my comments:

- I agree that we should leave existing code (which uses stack-allocated variables) for performance reasons. This is meant to be a lightweight and efficient stack

- I also see Cortex and other MPU-enabled processors being sued increasingly so I think it is important to provide support for MPU

So I think using #defines is the way to go.

As we can tell, this is making the code bloated, so the idea I have is that, when we have official releases, we also provide some "cleaned-up" versions, such as lwip-1.5.0-NO-MPU.zip, or lwip-1.5.0-NO-IPV6.zip. We would need a preprocessor tool to eliminate unnecessary #define blocks.


Thanks again Artem for the contribution.
Cheers
Ivan

Ivan Delamer <idelamer>
Group Member
Fri 23 Nov 2012 01:34:57 PM UTC, comment #3: 

I made it configurable. See new patch attached (draft3).
It adds LWIP_MPU_COMPATIBLE option (default 0).
Also I removed extra MEMP_NUM_SOCKET_API option previously added by me.

I'm not sure about correctness of macro names I choosed (especially _REF/_DEREF may be interpreted conversely), so correct at your discretion.

I made rough tests of functionality affected (except lwip_getsockopt()/lwip_setsockopt()) for both disabled and enabled LWIP_MPU_COMPATIBLE option. Everything works stable for me.

(file #26962)

Artem Pisarenko <jblackarty>
Fri 23 Nov 2012 03:52:42 AM UTC, comment #2: 

But how exactly you propose to make it configurable ? Wrap all changes with preprocessor directives depending on option in lwipopts.h ? I'll try that but in my opinion it will make code more difficult to maintain and you might reject it. This will make my work meaningless.

Artem Pisarenko <jblackarty>
Thu 22 Nov 2012 05:18:31 PM UTC, comment #1: 

Thanks for the patch. It does make sense if you want MPU support enabled.

However, I don't think this will be true for all platforms and your patch does decrease performance on these platforms (while it does not gain anything there). In fact, the memp_malloc/memp_free-thing was how it was until 1.2.0 or so. We changed it to stack-allocation for performance reasons, then.

Being like that, I'm rather thinking about a config option for MPU support. I guess there might be other changes which would be nice for MPU systems but not necessary for MPU-less systems.

Simon Goldschmidt <goldsimon>
Group administrator
Wed 14 Nov 2012 05:08:13 AM UTC, original submission:  

The patch makes lwip support system architectures (ports) based on FreeRTOS-MPU (with memory protection unit) where tcpip thread must be run in user mode. (Modification support lwip usage in all cases but I didn't considered PPP. Maybe PPP module doesn't require modifications ever and will work.)

FreeRTOS-MPU doesn't allow user-mode tasks to get access to other threads' stacks. Current lwip code violates this rule because tcpip_thread accesses local variables located on stack of api function (which belongs to application thread) being passed by pointer via tcpip_msg. Based on static analysis of code (manual, no special instruments :)) I followed all references to local variables and moved them on heap/pool. This increases heap/pool usage but decreases stack usage in application and tcpip threads.

There are also other MPU constraints which isn't being meet by lwip design. They may be solved by means of port and application configuration. But this is topic of separate discussion. This patch only makes strictly required changes to lwip code itself.

"Release notes":
- LWIP_TCPIP_CORE_LOCKING option wasn't mentioned although I tried to consider it
- new MEMP_NUM_SOCKET_API option added
- struct dns_api_msg (in api_msg.h) changed
- struct api_msg_msg (in api_msg.h) changed
- struct netifapi_msg_msg (in netifapi.h) changed
- some definitions of socket structures from sockets.c moved to header

I made some basic tests which covering code I modified except lwip_getsockopt()/lwip_setsockopt() because I don't know sockets API so much. Everything seems to work stable for me.
I expect project maintainers provide me with support to pass modified code through all your internal tests and I'll report results.
I have STM3220G-EVAL board and from-scratch-made sys arch port and ethernet driver designed to work in mem-pools configuration of lwip.

You can read discussion on this topic was started on mailing list "[lwip-users] lwip and MPU (FreeRTOS-MPU)".

Artem Pisarenko <jblackarty>

 

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

Attached Files
file #26962:  lwip_mpu_redesign__draft3.patch added by jblackarty (45KiB - application/octet-stream)
file #26912:  lwip_mpu_redesign__draft2.patch added by jblackarty (39KiB - 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 idelamer (Posted a comment)
  • -email is unavailable- added by goldsimon (Posted a comment)
  • -email is unavailable- added by jblackarty (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 6 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2014-02-20 goldsimon StatusNone Done
        Assigned toNone goldsimon
        Open/ClosedOpen Closed
    2013-01-15 goldsimon CategoryNone sockets/netconn
    2012-11-23 jblackarty Attached File- Added lwip_mpu_redesign__draft3.patch, #26962
    2012-11-14 jblackarty Attached File- Added lwip_mpu_redesign__draft2.patch, #26912

    Back to the top

    Powered by Savane 3.13-d3ae.
    Corresponding source code