patchlwIP - A Lightweight TCP/IP stack - Patches: patch #6253, Added csum to struct pbuf.

 
 

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

patch #6253: Added csum to struct pbuf.

Submitter:  Andrey Volkov <avolkov>
Submitted:  Wed 31 Oct 2007 05:41:09 PM UTC
   
 
Category:  None Priority:  5 - Normal
Status:  Wont Do Privacy:  Public
Assigned to:  goldsimon Open/Closed:  Closed
Planned Release:  None

Jump to the original submission

Fri 26 Mar 2010 04:20:05 PM UTC, comment #17: 

Won't do (see task #6849): it doesn't work with pbuf_header and we have another way to do it for TX.

Simon Goldschmidt <goldsimon>
Group administrator
Thu 29 Oct 2009 09:48:23 PM UTC, comment #16: 

I know this is still on 'In Progress', but I haven't had time for this since nearly two years now. However, I'm still planning for this to get in to 1.4.0!

The problem with including it in the pbuf struct was that we wanted to try to keep memory footprint low: When including the checksum in the pbuf, you'd need
- 2 bytes for checksum
- maybe 2 or more bytes for checksum coverage
per pbuf. Especially in low-memory-environments where you prefer chains of small pbufs, you would waste memory (because you don't keep the checksum once per packet but once per pbuf).

Therefore, for TX, we wanted to include the checksum somewhere else, not in the pbuf struct but once per packet. This might of course differ between UDP/RAW (where one call results in one packet) and TCP (where multiple calls can be coalesced into one packet).

RX is different of course: I'd imagine to throw away packets with invalid checksum before even passing them to the netif->input function, whereas your solution might involve saving the checksum and simply comparing calculated- and header-checksum in the correct protocol-layer (if I remember correctly)?

Simon Goldschmidt <goldsimon>
Group administrator
Thu 29 Oct 2009 08:37:30 PM UTC, comment #15: 

Do you want to expand this and include it in 1.4.0?  I've implemented hardware checksum offloading for both UDP and TCP.  The changes were minimal for my method of having the checksum at the time I wanted to send the data (tcp_enqueue and udp_sendto).

It's relevant to this patch because I needed a csum (checksum) in the pbuf struct for what I did.

Bill Auerbach <billauerbach>
Thu 28 Aug 2008 10:52:38 AM UTC, comment #14: 


> Since, when my app. generate udp payload, it do it directly to
> preallocated buffer, and do it in asm routine, i.e. payload csum
> calculation is part of payload data generation process.


The standard lwIP procedure for this kind of MAC would be to check the 2 checksums for correctness in your driver (i.e. before passing them to the lwIP input functions) and to discard packets with invalid checksums right away. You can then turn off checksum checking on receive for protocols you know (this would be tcp, udp and icmp presumably). While this involves a little bit of 'hacked' code (processing IP and transaport layer in MAC driver), it is more performant and also what is done in more advanced MACs (discarding packets with wrong checksums).

As to sending packets: From what you say, I think saving the checksum per packet (not per pbuf) would still be OK: You would include the transport checksum (data only, not header) when filling the pbuf, this transport checksum could be recalculated when the transport header is added; the IP checksum can't be calculated while copying as there is no copying involved when filling the header (i.e. IP checksum will be generated as it is now). For eth header, there is no checksum!

Simon Goldschmidt <goldsimon>
Group administrator
Wed 27 Aug 2008 12:26:19 PM UTC, comment #13: 

Hi Simon

> Why do you "calculate csum for it before I call udp_send (and do
> it fast in asm)" instead of providing this fast asm routing as
> LWIP_CHKSUM macro?


Since, when my app. generate udp payload, it do it directly to preallocated buffer, and do it in asm routine, i.e. payload csum calculation is part of payload data generation process.

> Anyway, from your description, it seems to me as one checksum
> per packet could also be sufficient...


Only and only if packet == pbuf. But in my case, for any outgoing udp packet, I need 2 pbufs in chain - first for udp/ip/eth headers (and lwip will calculate csum for it in udp_sendto_if), second - for payload (with precalculated csum).

Also, for each incoming ethernet packet, AD BF537 MAC calculate 2 csums - for ip header and for ip payload. So, in this case, lwip need NOT to recalculate checksums for incoming ip packets, if driver store they somewhere (in pbufs).

--
Andrey

P.S. Oops, I'm sorry, that don't tell it before, but I use only low level api, so udp_send called directly from my application.

Andrey Volkov <avolkov>
Tue 26 Aug 2008 08:29:41 PM UTC, comment #12: 

Andrey, I know this comment is a little late, I'm only starting to work on this again now:

Why do you "calculate csum for it before I call udp_send (and do it fast in asm)" instead of providing this fast asm routing as LWIP_CHKSUM macro?

Anyway, from your description, it seems to me as one checksum per packet could also be sufficient...

Simon Goldschmidt <goldsimon>
Group administrator
Thu 01 Nov 2007 03:28:53 PM UTC, comment #11: 

Simon Goldschmidt wrote:

> Follow-up Comment #6, patch #6253 (project lwip):
>
> Andrey Volkov wrote:
>> Yes, but I introduce this field mainly for next purposes:
>>  1) speed up reassembly of incoming ip packets. In reassembly case,
>>     ip payload csum of each fragment should be stored somewhere.
>>     (IMHO, pbuf is very appropriate for it).
>
> That might be solved best by a change of the IP implementation by not
> checking reassembled packets again (we check a checksum we generate ourselves,
> anyway...)
>
>>  2) Hardware usually could calculate ip payload/headers checksum,
>>     but not tcp/udp pseudo checksum, hence ip csum should be stored.
>>     Also, I dislike to mix different levels of stack in one piece
>>     of code. I.e. if I've ip checksum in eth driver, I could undef
>>     CHECKSUM_CHECK_IP, but not CHECKSUM_CHECK_TCP/CHECKSUM_CHECK_UDP,
>>     since I don't know it in driver level.
>
> I don't think I really understand you here. Could you maybe describe your
> changes to inet_chksum_pseudo_partial()? What I understand from reading the
> code, you use p->csum for the last pbuf in a linked list only, so effectively
> per packet, not per pbuf! (But then again, if your pbufs are big enough to
> hold a full packet - no pbuf chains - then this is the same for you.)

Yes, it is misunderstandings from your side ;).

Well I'll try to describe how I currently use lwip:

1) BF537 have chained DMA (i.e. could send/receive N chained pbufs)
   and could calculate ip payload/headers csum only for a single
   incoming eth packet.
2) In my app. I need to transfer large udp packets (i.e. they fragmented
   almost always).
3) When I fill payload for outgoing udp d-gramms, I calculate csum for
   it before I call udp_send (and do it fast in asm).
   So only what lwip is need to do, is calculate csum for udp/ip
   headers. I.e. on input of inet_chksum_pseudo in udp_send
   I've chain minimum from 2 pbufs:
   udp header (without valid csum) and ref to my large udp payload
  (with valid csum).
4) When stack receive fragmented udp, ip csum, calculated by hwd.,
   presented not in every ip fragment (mainly pbuf's csum must be
   recalculated if pbuf_realloc in ip_input really truncate pbuf).
   So, when inet_chksum_pseudo called from udp_input for reassembled
   udp, it have again pbuf's chain with partially calculated csum as
   input arg :(.

In both cases I need csum field in every pbuf, and I'm ready to
sacrifice memory space for any other pbuf usage.

Now about inet_chksum_pseudo: it check flag in every pbuf of chain,
and if it set then use value of csum field, if not then calculate it.
If, in simplest case, chain contain only one pbuf with valid csum, then
inet_chksum_pseudo should do nothing. Same for inet_chksum_pseudo_partial (checksum_len, in general case, could be greater then size of one pbuf).

>
>> And,
>>  3) In my hwd. driver, I don't 'memcpy' incoming packets, DMA do it
>>     directly to pbuf.
>
> Of course, that's because task #6849 wants to speed up systems that don't
> have HW-checksum engines (by only loading the data once into the processor by
> combining memcpy with checksum generation).



Andrey Volkov <avolkov>
Thu 01 Nov 2007 02:38:54 PM UTC, comment #10: 

Oh, I hate it when it turns out we've been fervently agreeing all along :-).

> I don't think I understand. What do you mean with 'this'? DMA
> alignment? I wouldn't add fields to struct pbuf until we have
> code that takes care for DMA alignment or else my code doesn't
> work any more, either!


Yes, I meant alignment for DMA/cache/whatever. My changes to the core code were modest. The syntax of enforcing alignment is non-portable (unless you start to let things get wasteful), so I thought the best thing would be to allow the driver or port to override how the pbuf pools are declared. I then also added a hook that could be called on freeing.

But the patch is not against CVS, so I'd need to port it to the current code base, after updating my own port to CVS.

Jonathan Larmour <jifl>
Group Member
Thu 01 Nov 2007 02:27:31 PM UTC, comment #9: 


> Yes, I know that it's been put off till after 1.3. That doesn't stop it being relevant.


No, of course it's not!

> To be clear, I already have working and deployed code for this. This is not vapourware.


I don't think I understand. What do you mean with 'this'? DMA alignment? I wouldn't add fields to struct pbuf until we have code that takes care for DMA alignment or else my code doesn't work any more, either!

> ... so you are now wasting 28 bytes per pbuf.


Of course, that's wasted memory. I'll take a look at the extra space for checksum at the beginning (or end?) of a pbuf for checksum-on-copy, that sounds quite nice to me!

Anyway, both checksum-on-copy AND the change this patch suggests should be held back until after 1.3.0 release!

Simon Goldschmidt <goldsimon>
Group administrator
Thu 01 Nov 2007 02:07:44 PM UTC, comment #8: 


>> usually there are alignment constraints, particularly if
>> caching is also involved, in which case you have to keep
>> packet data and other data (including struct pbuf itself)
>> in different cache lines
>
> That's true of course, but it's not covered today!


Yes, I know that it's been put off till after 1.3. That doesn't stop it being relevant.

> We would need a second memory alignment setting for DMA data.
> Then, when adding fields to the struct pbuf, the struct would
> automatically be aligned for DMA needs.


To be clear, I already have working and deployed code for this. This is not vapourware.

But if you increase a struct pbuf from 32 to 36 bytes, then those systems with DMA and caches will have to align to the cache line, which is often 32 bytes, so you are now wasting 28 bytes per pbuf. Other systems place other constraints on DMA alignment, even when cache line alignment is irrelevant, for example on Coldfire.

Anyway, this problem is avoidable.


Jonathan Larmour <jifl>
Group Member
Thu 01 Nov 2007 01:54:53 PM UTC, comment #7: 

[ Moving mail from lwip-devel to the patch, to keep discussion in one place ]
Andrey Volkov wrote:

> Jonathan Larmour wrote:
>> struct pbuf is very good right now for being exactly 32 bytes,
>> which is very amenable to keeping pbuf buffer pools aligned on
>> hardware that supports DMA - usually there are alignment
>> constraints, particularly if caching is also involved, in
>> which case you have to keep packet data and other data
>> including struct pbuf itself) in different cache lines.
> But I don't change size of pbuf everywhere, they changed only
> if someone defined CHECKSUM_HARDWARE_SUPPORT (and
> LWIP_PBUF_CB_SIZE, which i plan to introduce in next patch).


Indeed I'm aware - that's why I was pointing out the issues even when it's an option.
 

>> Now this isn't something which
>> is very easy in lwIP at the moment, but only because it's
>> been deliberately postponed until after 1.3. I think there's
>> a task for it somewhere. But I have made my own modifications
>> in my source base, and when the time comes after 1.3, I was
>> going to raise the issue.
>>
>> So increasing its size should be avoided if at all possible.
>> Even an option isn't so good - some hardware has
>> hardware-assisted checksum computation, and devices like that
>> would probably support DMA too.
>> But I think there are further issues that make such a change
>> difficult - this causes every pbuf that is to be sent to be
>> increased too. And every pbuf in a pbuf chain, when there only
>> need be one per whole packet.
>>
> No, each packet should contain this field, since, if yore hwd.
> have support of csum/DMA, then your PBUF_POOL_BUFSIZE will be
> equal to eth frame size, isn't it?


No it isn't. Some MACs I have worked have either hard fixed buffer sizes of low granularity (e.g. 128 bytes), some have soft configurable fixed sizes (so you can make them chains of whatever size you like, and the hardware will only consume as many as it needs), and a few have been fixed at the full ethernet frame size. The last is of course very wasteful of memory.

If alignment is an issue, then that would be a time to append the checksum to the end of the packet. Then alignment is unaffected.
 

>> There are other possibilities, but the one that occurs to me
>> most is to include the checksum in the pbuf contents
>> somewhere, either the start or end. This fact could be
>> indicated by a pbuf flag (or two, if you want to allow choice
>> of start or end).
>>
> If you add 'abstract' control block, you are lost alignment in
> worst case too. So I don't see difference from my suggestion.


pbufs already have flags. My proposal does not increase the struct pbuf size.
 
Jifl

Jonathan Larmour <jifl>
Group Member
Thu 01 Nov 2007 01:11:44 PM UTC, comment #6: 

Andrey Volkov wrote:

> Yes, but I introduce this field mainly for next purposes:
>  1) speed up reassembly of incoming ip packets. In reassembly case,
>     ip payload csum of each fragment should be stored somewhere.
>     (IMHO, pbuf is very appropriate for it).


That might be solved best by a change of the IP implementation by not checking reassembled packets again (we check a checksum we generate ourselves, anyway...)

>  2) Hardware usually could calculate ip payload/headers checksum,
>     but not tcp/udp pseudo checksum, hence ip csum should be stored.
>     Also, I dislike to mix different levels of stack in one piece
>     of code. I.e. if I've ip checksum in eth driver, I could undef
>     CHECKSUM_CHECK_IP, but not CHECKSUM_CHECK_TCP/CHECKSUM_CHECK_UDP,
>     since I don't know it in driver level.


I don't think I really understand you here. Could you maybe describe your changes to inet_chksum_pseudo_partial()? What I understand from reading the code, you use p->csum for the last pbuf in a linked list only, so effectively per packet, not per pbuf! (But then again, if your pbufs are big enough to hold a full packet - no pbuf chains - then this is the same for you.)

> And,
>  3) In my hwd. driver, I don't 'memcpy' incoming packets, DMA do it
>     directly to pbuf.


Of course, that's because task #6849 wants to speed up systems that don't have HW-checksum engines (by only loading the data once into the processor by combining memcpy with checksum generation).

Simon Goldschmidt <goldsimon>
Group administrator
Thu 01 Nov 2007 01:01:02 PM UTC, comment #5: 


> include the checksum in the pbuf contents somewhere, either
> the start or end.


Good idea.

Kieran Mansley <kieranm>
Group Member
Thu 01 Nov 2007 12:47:48 PM UTC, comment #4: 


> I'm pretty sure that some developers will ask you to enable
> that with an option


Of course!

> Do you have read "task #6849 : Test how checksum on copy could
> be integrated into the stack"


In fact I did some work on that task already and it also included adding flags to the struct pbuf. Only in my case, this was needed because I was adding one checksum per pbuf (when copying into the pbuf for sending). Of course this must be an option and you trade speed for size!

Andrey's suggested patch is different, of course, since it would turn off checksum generation for the TX side because those are automatically generated by the HW, I suppose (my checksum-checking-HW discarded packets with wrong checksums immediately, by the way, which seems to be even faster than Andrey's patch).


> usually there are alignment constraints, particularly if
> caching is also involved, in which case you have to keep packet
> data and other data (including struct pbuf itself) in different
> cache lines


That's true of course, but it's not covered today! We would need a second memory alignment setting for DMA data. Then, when adding fields to the struct pbuf, the struct would automatically be aligned for DMA needs.


To sum it up, for this patch, a 'checksum-per-packet'-field would be better (especially when looking at the modification of inet_chksum_pseudo_partial(), which I can't really read without comments. BTW: I wonder if the patch works at all since this function is used for UDPLite only), while one flag per pbuf would merge quite nice with my work on task #6849.

Simon Goldschmidt <goldsimon>
Group administrator
Wed 31 Oct 2007 07:56:20 PM UTC, comment #3: 

struct pbuf is very good right now for being exactly 32 bytes, which is very amenable to keeping pbuf buffer pools aligned on hardware that supports DMA - usually there are alignment constraints, particularly if caching is also involved, in which case you have to keep packet data and other data (including struct pbuf itself) in different cache lines. Now this isn't something which is very easy in lwIP at the moment, but only because it's been deliberately postponed until after 1.3. I think there's a task for it somewhere. But I have made my own modifications in my source base, and when the time comes after 1.3, I was going to raise the issue.

So increasing its size should be avoided if at all possible. Even an option isn't so good - some hardware has hardware-assisted checksum computation, and devices like that would probably support DMA too.

But I think there are further issues that make such a change difficult - this causes every pbuf that is to be sent to be increased too. And every pbuf in a pbuf chain, when there only need be one per whole packet.

There are other possibilities, but the one that occurs to me most is to include the checksum in the pbuf contents somewhere, either the start or end. This fact could be indicated by a pbuf flag (or two, if you want to allow choice of start or end).

There may well be other solutions, particularly if you consider the possibility of reducing u16_t ref to a u8_t.

Jonathan Larmour <jifl>
Group Member
Wed 31 Oct 2007 07:14:39 PM UTC, comment #2: 


>Is HWD_SUPPORT_CSUM will be appropriate?
>
>Andrey


CHECKSUM_HARDWARE_SUPPORT (to be in the same spirit than other CHECKSUM_xxx) ?

Do you have read "task #6849 : Test how checksum on copy could be integrated into the stack" (https://savannah.nongnu.org/task/?6849) ?


Frédéric Bernon <fbernon>
Group Member
Wed 31 Oct 2007 05:52:16 PM UTC, comment #1: 

I'm pretty sure that some developers will ask you to enable that with an option in opt.h/lwipopts.h (if they hardware doesn't support it, it's not necessary to add this code and to increase pbuf struct).


Frédéric Bernon <fbernon>
Group Member
Wed 31 Oct 2007 05:41:09 PM UTC, original submission:  

Hello!

This patch add csum field in struct pbuf. This field mainly will be speed up inet csum calculation in in case when hardware support it (like Analog Devices BF537).

--
Regards
Andrey Volkov

Andrey Volkov <avolkov>

 

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

Attached Files
file #14264:  pbuf_csum.diff added by avolkov (3KiB - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by billauerbach (Posted a comment)
  • -email is unavailable- added by kieranm (Posted a comment)
  • -email is unavailable- added by goldsimon (Posted a comment)
  • -email is unavailable- added by jifl (Posted a comment)
  • -email is unavailable- added by fbernon (Posted a comment)
  • -email is unavailable- added by avolkov (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 5 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2010-03-26 goldsimon Open/ClosedOpen Closed
    2010-03-26 goldsimon StatusIn Progress Wont Do
    2008-08-28 goldsimon StatusNone In Progress
        Assigned toNone goldsimon
    2007-10-31 avolkov Attached File- Added pbuf_csum.diff, #14264

    Back to the top

    Powered by Savane 3.13-758e.
    Corresponding source code