[ovs-dev] [rfc 3/6] ovs-datapath: Remove set_skb_csum_bits()
horms at verge.net.au
Mon Aug 30 07:34:14 UTC 2010
On Thu, Aug 26, 2010 at 07:42:01PM -0700, Jesse Gross wrote:
> On Thu, Aug 26, 2010 at 4:57 AM, Simon Horman <horms at verge.net.au> wrote:
> > On Wed, Aug 25, 2010 at 09:33:05PM -0700, Jesse Gross wrote:
> >> On Wed, Aug 25, 2010 at 12:08 AM, Simon Horman <horms at verge.net.au> wrote:
> >> > set_skb_csum_bits() is empty in the current kernel tree
> >> > so just remove it.
> >> There are actually a lot of checksum related compatibility bits mixed
> >> into the main code. It's definitely the single most invasive
> >> compatibility issue that we have.
> >> The issue is that on older kernels both the modern CHECKSUM_COMPLETE
> >> and CHECKSUM_PARTIAL were defined as CHECKSUM_HW. This normally is OK
> >> because you can tell whether you are on the transmit or receive path
> >> and behave appropriately. However, as a switch we are right in the
> >> middle and can't divine the correct meaning (and the meanings are
> >> essentially opposite). This is made worse by Xen which defines a few
> >> extra bits in the skb to disambiguate these values, which we have to
> >> handle as well. It's a real mess and requires littering things
> >> throughout the code to track and check the actual state. Throw in a
> >> few bugs (like what this function is compensating for) and it gets
> >> even more exciting.
> >> Thankfully the current kernel tree has resolved all of these issues
> >> and we can delete all of this crap when merging upstream. However, we
> >> should probably do it all at the same time. In addition, all of the
> >> other #ifdefs based on the kernel version should be removed at that
> >> time as well. There's also at least one function copied from the
> >> kernel source because it isn't exported, which we will be able to
> >> avoid.
> >> I'm definitely happy to see this type of thing go but I wonder what
> >> the best time is to do that. Doing it now will significantly clean
> >> the code up and will be appreciated by anyone reviewing the code. On
> >> the other hand, since it will be a big patch it will be more work to
> >> integrate changes as we incorporate feedback.
> > Thanks for the explanation.
> > The actual motivation for removing it now is that LINUX_VERSION
> > didn't seem to be defined, and rather than dragging it in, I removed
> > the code... one thing led to another.
> > I do agree that all this stuff should be removed at the same time,
> > so I'll drop this change for now and add any includes needed to make
> > the compile happen.
> Hmm, that's interesting that it isn't defined. I see that version.h
> isn't included, is that the only issue? If you give me a patch (or
> tell me what the problem is) I can push it and we won't need this.
> > However, if we are going for a merge into net/ instead of drivers/staging/,
> > then I suspect we may be asked to remove this code before the merge.
> I'm sure that we'll need to remove it before the merge. However,
> since there will probably be other things that come up we might as
> well delay until that time.
More information about the dev