Assert fail using zero copy buffers porting FreeRTOS-Plus-TCP V3.1.0 to V4.2.2

Libraries
1

Greetings,
I successfully ported our app from FreeRTOS-Plus-TCP V3.1.0 to V4.2.2. Our app is working as it did running V3.1.0 however it is necessary to comment out the line:

vPreCheckConfigs( )

in

BaseType_t FreeRTOS_IPInit_Multi( void )

The following two asserts fail:

configASSERT( ipBUFFER_PADDING >= 10U );
configASSERT( ( ( ( ipSIZE_OF_ETH_HEADER ) + ( ipBUFFER_PADDING ) ) % 4U ) == 0U );

We have been using an implementation of zero copy buffers all the way back to when we started using the stack (Build 160112 in 2016). Our configuration has:

ipconfigPACKET_FILLER_SIZE = 0
ipBUFFER_PADDING = (8U + ipconfigPACKET_FILLER_SIZE)

As far as I can tell we aren’t now or were prior suffering performance issues or failure related to unaligned access.

I hate to modify library code to use it. Is the check necessary?
Curious to hear your thoughts.

Thanks!

2

Hi @blanco-ether,

Is there any particular reason for you to custom define ipconfigPACKET_FILLER_SIZE as 0 in your FreeRTOSIPConfig.h?

The reason for the default definition of ipconfigPACKET_FILLER_SIZE as 2 in the FreeRTOSIPConfigDefaults.h is to force the IP header of network packets to start at a 32-bit aligned address (ethernet header is considered to be 14 bytes; adding 2 makes it have the required alignment) so that in most platforms it makes the code more efficient and avoids failures when used with caching enabled.

3

Apologies for the late reply but I had to spend time retracing the logic behind our implementation.

| Offset | Alignment | Length | Comments

32 64 32 64 32 64
0 0 4 8 4 8 uchar_8 * pointer; Points to the ‘NetworkBufferDescriptor_t’.
4 8 4 8 6 6 uchar_8 filler[6]; // To give the +2 byte offset.
—
10 14 4+2 8+2 6 6 uchar_8 dest_mac[6]; // Destination address . <---- processor MAC requires this offset to be 32bit aligned
16 20 4 8 6 6 uchar_8 src_mac[6]; // Source address.
22 26 4+2 4+2 2 2 uchar16_t ethertype;
24 28 4 4 ~ ~ << IP-header >> // word-aligned, either 4 or 8 bytes. <---- processor MAC requirement prevents this offset from ever being 32bit aligned

Maybe I’m missing something or implemented no copy buffers in an unanticipated manner. I haven’t worked with any MAC controllers other than the one in our processor so I do’t have a reference as to how other MAC controllers implement receiving Ethernet packets.

4

If it’s a hardware requirement to have the ethernet header start at a 32 bit aligned address, then we would need to remove that assert/make it optional, as it wouldn’t be possible to have a valid ipconfigPACKET_FILLER_SIZE/ipBUFFER_PADDING value that will satisfy both the requirements (32 bit alignment requirement for protocol headers).

@htibosch do you have any thoughts on this?

5

Maybe few applications that use FreeRTOS-Plus-TCP take advantage of no copy buffers as it is a significant effort to implement. I can’t imagine a MAC on any processor that wouldn’t have an (4/8) alignment requirement for it’s receive and transmit buffers however I could imagine a MAC having an option to automatically offset (pad) the beginning of a packet within a word boundary when receiving. This is only speculation of course as I haven’t written any other MAC drivers.

6

Some ethernet hardware provides support for TX/RX offsetting (example, XEMACPS_NWCFG_RXOFFS_MASK for Xilinx Zynq); an example usage is the Zynq network interface that’s part of the FreeRTOS Plus TCP repository. [refer - FreeRTOS-Plus-TCP/source/portable/NetworkInterface/Zynq/x_emacpsif_dma.c at V4.3.3 · FreeRTOS/FreeRTOS-Plus-TCP · GitHub]

In the above linked code for Xilinx EMACs, writing 0x8000 for XEMACPS_NWCFG_RXOFFS_MASK configures a 2-byte (in sync with the value of ipconfigPACKET_FILLER_SIZE which is set as 2) receive buffer offset. This effectively tells the EMAC to store incoming Ethernet frames starting 2 bytes after the address it’s given for the receive buffer. This makes sure we pass 32 bit aligned addresses as the receive buffer, at the same time ensuring the protocol headers are starting at a 32 bit aligned address. You could check if your hardware supports this feature.

7

I had surmised that such a feature was possible and checked for availability at the time of my previous post. I checked the MAC registers again in our device when you confirmed that Zynq MAC controller offers that feature. Unfortunately the feature does not exist in the MAC in our device.

8

In that case, we might need to make those asserts optional by keeping the assert as the default option and disabling it as a config option. Would you be interested in creating a Github PR with the changes?

Could define something like, for example, ipconfigPORT_SUPPRESS_WARNING and keep the asserts inside the new macro, also disable it by default so that it can be overridden by a definition in the FreeRTOSIPConfig.h; thus, modifying library code isn’t required.

9

I made the modifications you suggested but am holding off on a pull request to get your thoughts about a point for removing the asserts entirely. The default settings in place in FreeRTOSIPConfigDefaults.h and in FreeRTOS_IP.h make sure the settings are ideal so it seems that there’s no point in checking it. A user that modifies ipconfigPACKET_FILLER_SIZE is clearly on their own.

To further the point, ipconfigPACKET_FILLER_SIZE defined in FreeRTOSIPConfigDefaults.h defaults to the ideal value if it is not defined in an applications FreeRTOSIPConfig.h. The comments and checks imply that 0 is a valid value to set it to.

/*
 * ipconfigPACKET_FILLER_SIZE
 *
 * Type: size_t
 * Unit: bytes
 * Minimum: 0
 *
 * In most projects, network buffers are 32-bit aligned plus 16 bits.
 * The two extra bytes are called "filler bytes". They make sure that the
 * IP-header starts at a 32-bit aligned address. That makes the code
 * very efficient and easy to maintain. An 'uint32_t' can be assigned/
 * changed without having to worry about alignment.
 *
 * See ipconfigBUFFER_PADDING.
 */

#ifndef ipconfigPACKET_FILLER_SIZE
    #define ipconfigPACKET_FILLER_SIZE    ( 2 )
#endif

#if ( ipconfigPACKET_FILLER_SIZE < 0 )
    #error ipconfigPACKET_FILLER_SIZE must be at least 0
#endif

#if ( ipconfigPACKET_FILLER_SIZE > SIZE_MAX )
    #error ipconfigPACKET_FILLER_SIZE overflows a size_t
#endif

Comments for that configuration setting in FreeRTOS-Plus-TCP source at some point prior (pre AWS) are shown below. I copied them from FreeRTOS-Plus-TCP to my configuration file years ago when I ported the stack to our application. The default value was still 2 at the time but I defined it as 0 then as it is today.

Note that I’m not sure if the comments were for the original macro name, ipFILLER_SIZE and what version of FreeRTOS-Plus-TCP it was originally in. Possibly zero copy demo code, I’m not sure.

/* Advanced only: in order to access 32-bit fields in the IP packets with
32-bit memory instructions, all packets will be stored 32-bit-aligned, plus
16-bits.  This has to do with the contents of the IP-packets: all 32-bit fields
are 32-bit-aligned, plus 16-bit(!). */

My point is that putting a warning in comments for ipconfigPACKET_FILLER_SIZE and removing the asserts may be a better approach.

I also realized that I could have defined ipconfigBUFFER_PADDING as:

#define ipBUFFER_PADDING ( 12U + ipconfigPACKET_FILLER_SIZE )

to avoid the assert although the extra memory would be unused.

What are your thoughts?

10

Agree on that, but on the other side, there isn’t much hardware that doesn’t offer the ethernet buffer offsetting feature. (at least based on the questions posted here)
In those cases, this assert help to choose the right value for the ipconfigPACKET_FILLER_SIZE or ipconfigBUFFER_PADDING. Based on the hardware, the performance could be significantly impacted if the alignment is not correct. I believe keeping this behavior default and disabling it being optional makes the impact a bit clearer for the users.

I also realized that I could have defined ipconfigBUFFER_PADDING as:
#define ipBUFFER_PADDING ( 12U + ipconfigPACKET_FILLER_SIZE )
to avoid the assert although the extra memory would be unused.

If ipconfigPACKET_FILLER_SIZE is defined as 0, then it will still trigger the assert here:

configASSERT( ( ( ( ipSIZE_OF_ETH_HEADER ) + ( ipBUFFER_PADDING ) ) % 4U ) == 0U );

wouldn’t it?

11

Understood.

I created the pull request:
Add configuration option to prevent configASSERT checks on ipBUFFER_PADDING #1271

Your right, this would still fail:

configASSERT( ( ( ( ipSIZE_OF_ETH_HEADER ) + ( ipBUFFER_PADDING ) ) % 4U ) == 0U );

if ipBUFFER_PADDING defined as below when ipconfigPACKET_FILLER_SIZE is 0. No workaround other than commenting out the configASSERTs without a configuration option.

#define ipBUFFER_PADDING ( 12U + ipconfigPACKET_FILLER_SIZE )

1 Like
12

Thank you for creating the PR and contributing to FreeRTOS.