[ovs-dev] [PATCH 14/14] datapath: Consolidate checksum compatibility code.
Ben Pfaff
blp at nicira.com
Fri Dec 3 23:15:17 UTC 2010
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".
> 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.
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.
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.
Acked-by: Ben Pfaff <blp at nicira.com>
More information about the dev
mailing list