tasklwIP - A Lightweight TCP/IP stack - Tasks: task #16111, Set checksum to zero optimized-out...

 
 

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

task #16111: Set checksum to zero optimized-out in case of using a HW Checksum Block

Submitter:  Zakaria Abu Al-Ailah <zakaria>
Submitted:  Fri 21 Jan 2022 03:02:38 PM UTC
   
 
Category:  IPv4 Should Start On:  Fri 21 Jan 2022 12:00:00 AM UTC
Should be Finished on:  Mon 31 Jan 2022 12:00:00 AM UTC Priority:  9 - Immediate
Status:  None Privacy:  Public
Assigned to:  None Percent Complete:  0%
Open/Closed:  Open Planned Release:  None
Effort:  0.00

Sat 22 Jan 2022 08:43:35 PM UTC, comment #2: 

Hi Simon,
I hope you are doing well.

Thank you for your prompt reply.

We have been using LwIP for a long time ago, and didn't face a single issue with it.

Actually, we started having this problem after starting to use our own HARDWARE CHECKSUM BLOCK, so the checksum is computed in Hardware in order to speed up the process and get higher throughput.

To do so, we defined 'LWIP_CHKSUM' in 'lwipopts.h' as follows:
#define LWIP_CHKSUM hw_cks

The function 'hw_cks' sets the input parameters ('dataptr' and 'len') in some registers and then starts the HW BLOCK, and when the output gets ready, it reads the checksum from some register and returns it.

In unit testing, the HW BLOCK works 100% properly, but in integration with LwIP, we figured out that ping-replies are sent out with wrong header checksum!

In this case, we checked the disassembly code, and found out that the compiler has completely removed the code of the instruction 'IPH_CHKSUM_SET(iphdr, 0)' (line 236).
That said, the problem is never related to delay storing or something related to the cache.

The compiler is doing so, since the same field '_chksum' is sets twice (line 236 and 239), and this field is not used in software between the two sets, but actually the HW BLOCK will pass on it (in memory) in order to compute the new checksum (for the ping-reply).

We added a dummy instruction inside the function 'hw_cks(const void *dataptr, int len)', e.g.
uint8_t val = ((uint8_t )dataptr + 10); /* read the first byte of '_chksum' */
We then checked the disassembly code, and found that the compiler didn't remove the code of 'IPH_CHKSUM_SET(iphdr, 0)' (in this case, it is aware that this field is used).
We also did a test (receive ping-requests from the host and send ping-replies out) and verified that everything is working fine.

I guess you should add 'volatile' to the field '_chksum', in order to explicitly tell the compiler that this field might be used in hardware.

Note: We are using FTO optimization. It is required in our case.

Regards,
Zakaria

Zakaria Abu Al-Ailah <zakaria>
Fri 21 Jan 2022 04:26:41 PM UTC, comment #1: 

Is this from reading the code or do you actually have a problem?

Is the compiler allowed to delay storing 'iphdr' fields even though 'iphdr' is passed to the function 'inet_chksum()'? Or is it the cache you have a problem with? In that case, your 'inet_chksum' function calling the hardware is responsible to invalidate cache.

Either way, I'm not aware of a problem with this code?

Simon Goldschmidt <goldsimon>
Group administrator
Fri 21 Jan 2022 03:02:38 PM UTC, original submission:  

Hi,

There is an issue in the following file:
  - File: lwip/src/core/ipv4/icmp.c
  - Line: 236 and 239

In case `LWIP_CHKSUM` is defined by the user in order to be computed by a HW block, the compiler is optimized-out the instruction `IPH_CHKSUM_SET(iphdr, 0);` which is in `icmp.c` file - line 236 (permalink: https://github.com/lwip-tcpip/lwip/blob/34e435c78611fbf21c49c5ddb6c395a097e24cc7/src/core/ipv4/icmp.c#L236).

The problem is the existence of the next instruction `IPH_CHKSUM_SET(iphdr, inet_chksum(iphdr, hlen));` (line 239).

This means the same field `iphdr->_chksum` is assigned twice as follows:
iphdr->_chksum = 0; // line 236
iphdr->_chksum = <checksum computed by HW>;  // line 239

Since the checksum is computed by HW, the compiler is not aware that the field `iphdr->_chksum` will be taken into account in the HW Block. Therefore the compiler is optimized-out the first assignment.

One solution for this problem is adding `volatile` to the field `_chksum` which is in `struct ip_hdr`.


Regards,
Zakaria Abu Al-Ailah

Zakaria Abu Al-Ailah <zakaria>

 

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

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by goldsimon (Posted a comment)
  • -email is unavailable- added by zakaria (Submitted the item)
  • -email is unavailable- added by zakaria
  •  

    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.

     

    Follows 1 latest change.

    Date Changed by Updated Field Previous Value => Replaced by
    2022-01-21 zakaria Carbon-Copy- Added -email is unavailable-

    Back to the top

    Powered by Savane 3.13-4b48.
    Corresponding source code