patchlwIP - 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: 

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...

Simon Goldschmidt <goldsimon>
Group administrator
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.

I agree about this going in and not with an option.  It may be slightly larger in code size but there's no way it could be slower.

Bill Auerbach <billauerbach>
Fri 26 Mar 2010 04:34:34 PM UTC, comment #14: 

I measured this on my PC:
#loops: 50000000, optimized: 3027, unoptimized: 4944

However, the problem of this measurement is of course the fact that in reality, the effect could be different as data might then not be in the caches. I still think this patch should go in (as default, without an option).

Simon Goldschmidt <goldsimon>
Group administrator
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.

It duplicates the pertinent parts of IP header checksumming for inlining and not inlining.  In the not inlining case, it uses the current lwipopts.h settings for checksumming.  Because code and data will be cached on systems that can cache, this shows just the difference of executed code (processing cycles saved).

a) On a 100MhZ NIOS II with the lwIP standard checksum (algorithm 1) it's 2.22 times faster.  (1200mS and 538mS for 500,000 loops).  This is little endian and alignment matters.

b) On a 533MhZ PowerPC with the lwIP standard checksum (algorithm 1) it's 3.63 times faster.  (1265mS and 348mS for 5,000,000 loops).  This is big endian and alignment matters.

c) On a 533MhZ PowerPC with a highly optimized assembly checksum routine it's 2.26 times faster (788mS and 348mS for 5,000,000 loops).


(file #19960)

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>
Group administrator
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.

I have 2 other platforms I can test besides the NIOS-II.

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.

I should mention that a 5% decrease can easily come from the measurement utility - especially if the platform used for measurement has been a PC running under windows...

Simon Goldschmidt <goldsimon>
Group administrator
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: 


> 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.


I can't help much there, as my results will be the same as Bill's since we both use the NIOS-II...

Simon Goldschmidt <goldsimon>
Group administrator
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>
Group Member
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.

You could also extrapolate that if the processor can't store this checksum in a register, it can't store the loop count in inet_chksum either and so that routine is going to be very slow as well.

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.

Also, I do think that the majority of architectures profits from this patch!

Simon Goldschmidt <goldsimon>
Group administrator
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.

Most processors I know have enough registers to keep the checksum in a register, which means there is a significant performance gain.

Simon Goldschmidt <goldsimon>
Group administrator
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.

#define INLINE_IP_CHECKSUM 0 must be added to your opts.h file and the override added to the lwip_opts.h file.

Bill Auerbach <billauerbach>

 

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

Attached Files
file #19459:  ipinlinechecksum.diff added by billauerbach (2KiB - text/x-patch)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by kieranm (Posted a comment)
  • -email is unavailable- added by goldsimon (Posted a comment)
  • -email is unavailable- added by billauerbach (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
    2010-04-28 goldsimon StatusNone Done
        Assigned toNone goldsimon
        Open/ClosedOpen Closed
    2010-04-21 goldsimon Planned ReleaseNone 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

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code