[ovs-dev] [PATCH 14/14] datapath: Consolidate checksum compatibility code.

Jesse Gross jesse at nicira.com
Sat Dec 4 01:45:58 UTC 2010


On Fri, Dec 3, 2010 at 3:15 PM, Ben Pfaff <blp at nicira.com> wrote:
> On Thu, Dec 02, 2010 at 12:37:03PM -0800, Jesse Gross wrote:
>> Checksum offloading has changed quite a bit across different kernel
>> and Xen versions.  Since it is part of the skb data structure it is
>> unfortunately difficult to separate out into compatibility code.
>> This consolidates all of the checksum code in one, place which makes
>
> I would move the comma after the word "place".

Fixed.

>
>> it easier read and remove as we prepare for upstreaming.  On newer
>> kernels it also puts everything in inline functions, eliminating the
>> need to run through the compat code or make extra function calls.
>>
>> Signed-off-by: Jesse Gross <jesse at nicira.com>
>
> The code looks equivalent to me, with the caveat that I didn't verify
> that Linux implements the same logic in the !NEED_CSUM_NORMALIZE case.
> Most of it is just moving code around, as you say.  I like the idea of
> putting all the ugliness in one place.
>
> I would be tempted to also move the definition of NEED_CSUM_NORMALIZE
> and enum csum_type to checksum.h, then make datapath.h #include
> "checksum.h".  But I'll understand if you don't want that additional
> dependency.

I was originally planning on doing that but checksum.h includes
datapath.h  It's only used by a function that is needed for old
kernels, so I just moved that to checksum.c and pulled the
declarations over.

>
> As part of moving compute_ip_summed() between files you deleted the
> default "can't happen" case.  I guess you decided that it wasn't worth
> worrying about it.

Yeah, I don't think any new forms of checksum offloading a going to
appear soon in Linux and if they did it probably wouldn't just work,
so I removed that.

>
> Is code generation OK for get_skb_csum_pointers()?  David Miller has
> commented once or twice that returning values through reference
> arguments like that generates bad code.  If so, it could be rewritten as
> a pair of functions, each of which returns one of the desired values.
> But it is a very simple function and inline, and GCC might be good
> enough to see through that.
>

GCC seems to be smart enough to handle it - I looked at the
disassembly from both ways and they were identical, so I left it
alone.

> Acked-by: Ben Pfaff <blp at nicira.com>

Thanks, I pushed this out.




More information about the dev mailing list