[ovs-dev] [rfc 3/6] ovs-datapath: Remove set_skb_csum_bits()

Jesse Gross jesse at nicira.com
Fri Aug 27 02:42:01 UTC 2010


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 mailing list