[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