lwIP - A Lightweight TCP/IP stack - Patches: patch #7060, Patch to optionally inline ip...
You are not allowed to post comments on this tracker with your current authentication level.
patch #7060: Patch to optionally inline ip checksum
Submitter: | Bill Auerbach <billauerbach> | ||
Submitted: | Mon 11 Jan 2010 02:59:25 PM UTC | ||
Category: | None | Priority: | 3 - Low |
Status: | Done | Privacy: | Public |
Assigned to: | goldsimon | Open/Closed: | Closed |
Planned Release: | 1.4.0 |
Jump to the original submission
Wed 28 Apr 2010 07:37:46 PM UTC, comment #16: |
Simon Goldschmidt <goldsimon> |
Fri 26 Mar 2010 04:44:35 PM UTC, comment #15: The improvement will be greater with non-caching processors or if the data falls out of the cache. The better the caching the lower the gain as your PC test indicates. I am working on an lwIP 8-bit port to a non-caching processor and will add those results.
|
Bill Auerbach <billauerbach> |
Fri 26 Mar 2010 04:34:34 PM UTC, comment #14: I measured this on my PC:
|
Simon Goldschmidt <goldsimon> |
Tue 16 Mar 2010 05:14:58 PM UTC, comment #13: Take the attached C code and paste it at the bottom of IP.c. Put a break point on the for loop body and call the function inlineHdrChecksumBenchmark in your code. I called this after lwIP is initialized but with only a timer running. A #define needs to be filled in with a function name that gets an mS running timer from the system. Faster systems need to bump up the loop count.
|
Bill Auerbach <billauerbach> |
Fri 05 Mar 2010 12:27:42 PM UTC, comment #12: Bill, any progress on the benchmarks? I'd love to get a positive feedback here ;-) |
Simon Goldschmidt <goldsimon> |
Fri 12 Feb 2010 06:30:55 PM UTC, comment #11: I agree. The best way to benchmark is to run a program that tests only this given function without any possible side-effects. Since we're testing this patch I would expect the rest of the system to see no effect at all because of this change.
|
Bill Auerbach <billauerbach> |
Fri 12 Feb 2010 06:24:26 PM UTC, comment #10: I don't have a doubt that your tests will be positive, given the nature of your platform (NIOS-II). However, it would be interesting why 'bobbyb' was a performance decrease of 5% - especially since the MicroBlaze (which I guess he is using given the fact that he tested on a xilinx virtex) has 32 general-purpose registers, just as the NIOS-II does.
|
Simon Goldschmidt <goldsimon> |
Fri 12 Feb 2010 05:09:26 PM UTC, comment #9: I'll make the test include both functions without requiring lwIP - we only care about testing the one function. What this doesn't take into account is if someone has optimized or used a custom inet_chksum which might lessen the effect. Still I don't see how it could ever be slower inlined. |
Bill Auerbach <billauerbach> |
Fri 12 Feb 2010 03:53:11 PM UTC, comment #8:
|
Simon Goldschmidt <goldsimon> |
Fri 12 Feb 2010 03:36:36 PM UTC, comment #7: I will follow this up with a single function program that will test only this change by timing 10000 calls to this function without the patch, and then with it. If for now we can leave the #if test in the code to include or exclude the change, it will make testing this easier. |
Bill Auerbach <billauerbach> |
Fri 12 Feb 2010 03:29:46 PM UTC, comment #6: I'm not against this going in, but it's a shame we don't at least understand why it caused a slowdown on that one system. I agree with Bill that getting others to test it would be great as then we might find that it does speed up most systems. |
Kieran Mansley <kieranm> |
Fri 12 Feb 2010 03:19:44 PM UTC, comment #5: I agree, but I saw it use only one register - for the accumulating checksum. If there is no register for this, then yes, this patch has a penalty in one extra write per item. The extra read cancels the read that would have resulted in the call to inet_chksum.
|
Bill Auerbach <billauerbach> |
Fri 12 Feb 2010 03:14:40 PM UTC, comment #4: Well, as I said before, if the processor is at is limit regarding registers, any code change might result in slow-down as registers must be saved to stack and reloded from there. However, that can happen with any code change we make.
|
Simon Goldschmidt <goldsimon> |
Fri 12 Feb 2010 03:01:02 PM UTC, comment #3: If anyone else can reproduce the single finding that this patch hurt throughput, I'd love to see it verified. I just don't see how it's possible that it could. Code size impact has to be minimal. In fact, the weaker the processor, the better the optimization should be. |
Bill Auerbach <billauerbach> |
Fri 12 Feb 2010 02:53:35 PM UTC, comment #2: Do we get a consensus here? Kieran was against making this an option on lwip-devel. I agree but I'd make this the standard instead of not including it.
|
Simon Goldschmidt <goldsimon> |
Mon 11 Jan 2010 05:30:12 PM UTC, comment #1: I want to add that I did not test this on a big-endian platform. I don't see where in the code it would matter but I thought it was important for me to state this. |
Bill Auerbach <billauerbach> |
Mon 11 Jan 2010 02:59:25 PM UTC, original submission:
This patch inlines the ip header checksum. It's one of several changes I made that had a noticeable speed increase sending packets. Since we already touch each field in the header, adding to a sum on the fly had a minimal code size impact. On my platform, it wasn't much more than the function call it replaced to generate the checksum.
|
Bill Auerbach <billauerbach> |
Depends on the following items: None found
Items that depend on this one: None found
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 |
---|---|---|---|---|---|
2010-04-28 | goldsimon | Status | None | Done | |
Assigned to | None | goldsimon | |||
Open/Closed | Open | Closed | |||
2010-04-21 | goldsimon | Planned Release | None | 1.4.0 | |
2010-03-16 | billauerbach | Attached File | - | Added hdr_chksum_benchmark.c, #19960 | |
2010-01-11 | billauerbach | Attached File | - | Added ipinlinechecksum.diff, #19459 |
Powered by Savane 3.13-02a9.
Corresponding source code
I've finally checked this in. It stays an option only because CHECKSUM_GEN_IP can be turned off and I'll just leave in the old code, doesn't hurt if anyone really wants to save the bytes in code size...