FreeRTOS+TCP : add memory statistics and dump packets #44

htibosch · 2020-03-24T15:37:40Z

See also an earlier equivalent PR 1828 on aws/amazon-freertos

Description

Sometimes it is difficult to get an overview of how much RAM is actually used by FreeRTOS+TCP.

When enabled, tcp_mem_stats.c will write CSV records to the standard logging. With the use of grep and sed, these lines can be taken out and copied into a spreadsheet.

The result is a sheet (with formulas), that shows all relevant ipconfig macros, their values and how many objects are declared, either statically or dynamically. The user can test several scenarios by changing the macro values.

When not enabled ( ipconfigUSE_TCP_MEM_STATS not defined ), the module is not compiled and no extra code is added.

I tested the module on several platforms, notably ESP32, Xilinx and WinSim.

There is also some documentation in tools/tcp_mem_stats.md

Also: added tcp_dump_packets.c : it can be included in the WinSim project. It will write selected network packets to a C source files. This source file can later be include in testing projects to test the TCP software.

Test Steps

tcp_mem_stats.c : I had it run for a few minutes in a aws_tests project. After vTCPMemStatClose() was called, I did the following on the logging:

tcp_dump_packets.c : I filled in the desired packet types and had it run until all packet types were collected. I compiled the output into a testing module.

grep ".*TCPMemStat," < output.log | sed "s/.*TCPMemStat,//" > output.csv

I opened the CSV file as a spreadsheet.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

htibosch · 2020-03-24T15:40:06Z

Here below an example of a spread sheet as produced by an STMF32F7xx, in an FTP-session.
I truncated records at the bottom in order to keep this post short.

Some important ipconfig items:
ipconfig item	Value	Comment
NETWORK_MTU	1200
TCP_MSS	1140
USE_TCP	1
USE_TCP_WIN	1
TCP_RX_BUFFER_LENGTH	2280	Plus 24 bytes
TCP_TX_BUFFER_LENGTH	2280
USE_DHCP	1

RAM that is always allocated either statically or on the heap:
ipconfig item	Value	PerUnit	Total
NUM_NETWORK_BUFFER_DESCRIPTORS	12	60	720	Descriptors only
TCP_WIN_SEG_COUNT	32	64	2048
EVENT_QUEUE_LENGTH	17	8	136
IP_TASK_STACK_SIZE_WORDS	300	4	1200
ARP_CACHE_ENTRIES	6	20	120
DNS_CACHE_ENTRIES	1	268	268
Network buffers in HEAP	12	1224	14688
Total			19180	Actual size fluctuates because BufferAllocation_2.c is used


The following allocations are done on the heap while running:
Create/Remove	Object	Size	Heap use	Heap-min	Heap-Cur
CREATE	UDP-Socket	124	124	2055372	2055372
CREATE	TCP-Socket	504	628	2044460	2044460
CREATE	TCP-Socket	504	1132	2027092	2027092
CREATE	UDP-Socket	124	1256	2026844	2026844
REMOVE	UDP-Socket	-124	1132	2026844	2026884
REMOVE	UDP-Socket	-124	1008	2026668	2027024
CREATE	TX-Buffer	2304	3312	1995768	1995768
CREATE	RX-Buffer	2304	5616	1992160	1992160
REMOVE	RX-Buffer	-2304	3312	1992160	2042564
REMOVE	TX-Buffer	-2304	1008	1992160	2044876
REMOVE	TCP-Socket	-504	504	1992160	2047228
Totals			5616	1992160	2055372
Maximum RAM usage:			24796

htibosch · 2020-03-24T15:50:08Z

And here is an example of the network packet dump in C:

/* Packet_0004 */
uint8_t ucPacket_0004[ 66 ] =
{
    0x74, 0xb5, 0x7e, 0xf0, 0x47, 0xee, 0x00, 0x11, 0x22, 0x33, 0x44, 0x21, 0x08, 0x00, 0x45, 0x00,
    0x00, 0x34, 0x00, 0x00, 0x00, 0x00, 0x80, 0x06, 0x92, 0x48, 0xc0, 0xa8, 0x02, 0x0f, 0x12, 0xe0,
    0xd2, 0xe4, 0xcd, 0x1b, 0x22, 0xb3, 0xbe, 0x02, 0x89, 0x16, 0x00, 0x00, 0x00, 0x00, 0x80, 0x02,
    0x04, 0x74, 0x8c, 0x80, 0x00, 0x00, 0x02, 0x04, 0x04, 0x74, 0x01, 0x03, 0x03, 0x00, 0x01, 0x01,
    0x04, 0x02
};

DumpPacket_t xPacket_0004 =
{
    .pucData = ucPacket_0004,
    .uxLength = 66,
    .ulType = 0x10888, /* IN | FRAME_4 | TCP | SYN */
    .usSource = 52507,
    .usDestination = 8883,
};
/*-----------------------------------------------------------*/

It is a frame of type IPv4, and it is a SYN packet in a TCP conversation.

The file will also include an array of all available network packets:

DumpPacket_t *xPacketList[ dumpPACKET_COUNT ] =
{
    &xPacket_0000,
    &xPacket_0001,
    &xPacket_0002,
    &xPacket_0003,
    /* etc. */
};

So the testing software can search for certain types of packets.

tcp_dump_packets.h defines a complete list of all properties, like e.g. :

#define flag_ICMP4            0x00000001uL
#define flag_ICMP6            0x00000002uL
#define flag_UDP              0x00000004uL
#define flag_TCP              0x00000008uL
#define flag_DNS              0x00000010uL
/* etc */
#define flag_FRAME_6          0x00020000uL
#define flag_Unknown_FRAME    0x00040000uL

AniruddhaKanhere · 2020-03-25T20:27:50Z

FreeRTOS-Plus/Source/FreeRTOS-Plus-TCP/FreeRTOS_Sockets.c

 		}
 		else
 		{
+			if( xIsInputStream != 0 )


Can we do if( xIsInputStream != pdFALSE )? Just for consistency and 0 is technically a magic number.

Yes sure, pdFALSE is better.
Also, now I combined the macro calls with existing code:

- if( xIsInputStream != 0 ) + if( xIsInputStream != pdFALSE ) { + iptraceMEM_STATS_CREATE( tcpRX_STREAM_BUFFER, pxBuffer, uxSize ); pxSocket->u.xTCP.rxStream = pxBuffer; } else { + iptraceMEM_STATS_CREATE( tcpTX_STREAM_BUFFER, pxBuffer, uxSize ); pxSocket->u.xTCP.txStream = pxBuffer; }

Can you have a look at this?

AniruddhaKanhere

Some changes are trivial but one change regarding vTCPMemStatRemove is important

AniruddhaKanhere · 2020-03-25T21:05:33Z

FreeRTOS-Plus/Source/FreeRTOS-Plus-TCP/tools/tcp_dump_packets.c

+static void vAddProtocolTags( uint8_t *pucEthernetBuffer, BaseType_t xIPType );
+static void vDetermineMessageType( uint8_t *pucBuffer, BaseType_t xIncoming );
+static void vActualDump( uint8_t *pucBuffer, size_t uxLength, BaseType_t xIncoming );
+static void vAddType( uint32_t ulFlags, const char *pcFlagName );


Not to nitpick but shouldn't all these functions begin with a 'prv' since they are all private functions?
Note: I would not hold a merge for this

You are right: v is used for public functions returning void, prv is used for static (private) functions.
I changed the names to prvAddProtocolTags etc.

AniruddhaKanhere · 2020-03-25T23:19:27Z

FreeRTOS-Plus/Source/FreeRTOS-Plus-TCP/include/tcp_mem_stats.h

+
+	void vTCPMemStatCreate( TCP_MEMORY_t xMemType, void *pxObject, size_t uxSize );
+
+	void vTCPMemStatRemove( void *pxObject );


Did you mean to declare vTCPMemStatsDelete here?
Because vTCPMemStatsRemove is neither defined nor used anywhere. Line 37 seems correct since it uses a function defined in tcp_mem_stats.c.

Oops yes, you are correct. In an earlier version, I called them "create/remove", which are not really opposites.
Now it is "create/delete". I changed the header file, thank.

AniruddhaKanhere · 2020-03-27T21:31:19Z

FreeRTOS-Plus/Source/FreeRTOS-Plus-TCP/FreeRTOS_Sockets.c

-			if( xIsInputStream != 0 )
+			if( xIsInputStream != pdFALSE )
 			{
+				iptraceMEM_STATS_CREATE( tcpRX_STREAM_BUFFER, pxBuffer, uxSize );


Hi Hein,
I just have one question, does the memset on line 2986 memset( pxBuffer, '\0', sizeof( *pxBuffer ) - sizeof( pxBuffer->ucArray ) ); affect the way the iptraceMEM_STATS_CREATE( tcpRX_STREAM_BUFFER, pxBuffer, uxSize ); behaves?

Just making sure that moving iptraceMEM_STATS_CREATE( tcpRX_STREAM_BUFFER, pxBuffer, uxSize ); from before memset to current location doesn't have any unintended effects.
Similarly for the else case...

Except that, all seems to be good. :)

Good to notice this change in code, but no, the call to memset() and setting pxBuffer->LENGTH have no relation with iptraceMEM_STATS_CREATE().
The latter will only log and register the pointer along with the type and the size.
Thanks.

lundinc2 · 2020-03-30T21:47:46Z

FreeRTOS-Plus/Source/FreeRTOS-Plus-TCP/tools/tcp_dump_packets.c

+/*-----------------------------------------------------------*/
+
+/* The Windows thread that actually writes the network packets to a C source and header file. */
+static DWORD WINAPI prvWritePackets( LPVOID lpParameter );


Could we use some abstraction so this code can be reused on other platforms? I am going to approve, but this could be useful for the Linux port

I suppose we may as well shelf that for later, as the Linux port does not have networking added to it yet.

We can add that later, as soon as the Linux port also has TCP.

And there is also FreeRTOS+FAT: the module tcp_dump_packets.c could also run on any device that has a disk ( SD/MMC, or a RAM-disk ).
In that case, mentioned function would become a normal task:

static void prvWritePackets( void *pvParameter );

I have thought of adding some +FAT code, but the problem is that there are no AWS projects yet that use FreeRTOS+FAT.
( and once you have a disk, it would also be nice to be able to produce regular PCAP files ).

The tcp_mem_stats.c module, on the contrary, can run on any device that has logging.

lundinc2

I have some concerns about using the windows' APIs

lundinc2 · 2020-03-30T21:48:24Z

@htibosch also you will need to rebase once this is approved, this repo has much stricter branch rules.

dcgaws · 2020-03-30T22:47:27Z

FreeRTOS-Plus/Source/FreeRTOS-Plus-TCP/FreeRTOS_Sockets.c

 		}
 		else
 		{
+			if( xProtocol == FREERTOS_IPPROTO_UDP )


Rather than add these new branches, we could replace the various malloc calls in this module with a macro (e.g. pvPortSocketsMalloc). The macro could accept whatever additional call site-specific parameters are required for stats. Would there be disadvantages with that approach?

@dcgaws: I have thought of doing so, but the macro pvPortMallocSocket() only takes one parameter: the size. I think we'de like to see in the logging what type of socket was created, UDP or TCP.
Likewise, pvPortMallocLarge() only takes a parameter uxSize. That function creates the stream buffers of TCP.
A socket-set is allocated with the standard FreeRTOS function pvPortMalloc(), which is too generic.

The new macro's like iptraceMEM_STATS_CREATE() get their default values in IPTraceMacroDefaults.h. Users can define them for their own use ( in FreeRTOSIPConfig.h ), or use the new modules.

dcgaws · 2020-03-30T22:49:28Z

FreeRTOS-Plus/Source/FreeRTOS-Plus-TCP/include/tcp_dump_packets.h

@@ -0,0 +1,61 @@
+/*


Could packet dumping please be separated into its own PR? (At least, based on the assumption that it's independent of the mem stats changes, above.)

@dcgaws:
I combined the two because they want to change a common file: include/IPTraceMacroDefaults.h.
Also they want to create a new directory /tools, which sits next to source and include.
And a last reason is time: I saved a lot of time by testing them in one go and writing a single PR.

AniruddhaKanhere · 2020-04-17T18:30:32Z

/bot run checks

alfred2g · 2020-06-02T22:48:41Z

/bot run checks

alfred2g · 2020-06-02T23:15:16Z

Hi @htibosch,
Could you please rebase and push again, there are some conflicts

htibosch · 2020-06-03T01:27:23Z

Hi @htibosch,
Could you please rebase and push again, there are some conflicts

Yes sure, I will. Closing this PR in order to create a new one.
Hein

* Create manifest.yml * Remove trailing whitespace * Update manifest.yml * Corrected a typo * Uncrustify C files * Uncrustify header files * Update ci.yml * Update ci.yml * Uncrustify with version 0.67 * Spelling corrections

FreeRTOS+TCP : add memory statistics and dump packets

69b0acc

htibosch mentioned this pull request Mar 24, 2020

FreeRTOS+TCP : add memory statistics ( tcp_mem_stats.c ) aws/amazon-freertos#1828

Closed

2 tasks

lundinc2 self-assigned this Mar 24, 2020

lundinc2 self-requested a review March 24, 2020 21:24

AniruddhaKanhere reviewed Mar 25, 2020

View reviewed changes

AniruddhaKanhere requested changes Mar 25, 2020

View reviewed changes

Corrections after Aniruddha's review

dd1a1a1

AniruddhaKanhere reviewed Mar 27, 2020

View reviewed changes

lundinc2 reviewed Mar 30, 2020

View reviewed changes

lundinc2 approved these changes Mar 30, 2020

View reviewed changes

dcgaws suggested changes Mar 30, 2020

View reviewed changes

AniruddhaKanhere approved these changes Mar 31, 2020

View reviewed changes

htibosch mentioned this pull request Apr 8, 2020

Use LwIP as default networking stack on ESP32 aws/amazon-freertos#1886

Merged

2 tasks

AniruddhaKanhere requested a review from dcgaws April 14, 2020 21:39

alfred2g closed this Jun 2, 2020

alfred2g reopened this Jun 2, 2020

htibosch closed this Jun 3, 2020

htibosch mentioned this pull request Jun 3, 2020

FreeRTOS+TCP : add memory statistics and dump packets, v3 #83

Merged


		void vTCPMemStatCreate( TCP_MEMORY_t xMemType, void *pxObject, size_t uxSize );

		void vTCPMemStatRemove( void *pxObject );

FreeRTOS+TCP : add memory statistics and dump packets #44

FreeRTOS+TCP : add memory statistics and dump packets #44

Uh oh!

Conversation

htibosch commented Mar 24, 2020

Description

Test Steps

Uh oh!

htibosch commented Mar 24, 2020

Uh oh!

htibosch commented Mar 24, 2020

Uh oh!

AniruddhaKanhere Mar 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AniruddhaKanhere left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

htibosch Mar 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lundinc2 left a comment

Choose a reason for hiding this comment

Uh oh!

lundinc2 commented Mar 30, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AniruddhaKanhere commented Apr 17, 2020

Uh oh!

alfred2g commented Jun 2, 2020

Uh oh!

alfred2g commented Jun 2, 2020

Uh oh!

htibosch commented Jun 3, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

AniruddhaKanhere Mar 25, 2020 •

edited

Loading

htibosch Mar 26, 2020 •

edited

Loading