[ovs-dev] MPLS and VLAN QinQ patch

Tue Jun 26 17:02:22 PDT 2012

Patch sent last night had missed some fixes in the kernel + whitespace
errors. Anyways attaching latest patch.

On Mon, Jun 25, 2012 at 5:34 PM, ravi kerur <rkerur at gmail.com> wrote:
> Attached latest patch which mimics upstream kernel offload in ovs for
> both mpls and qinq.
>
> Upstream linux patch has changes for mpls and qinq and includes e1000e
> driver changes as well. They can be divided into different sub-patches
> before sending upstream but can be reviewed as a whole.
>
> On Mon, Jun 25, 2012 at 3:54 PM, Jesse Gross <jesse at nicira.com> wrote:
>> On Thu, Jun 21, 2012 at 11:08 AM, ravi kerur <rkerur at gmail.com> wrote:
>>> On Tue, Jun 19, 2012 at 11:55 AM, Jesse Gross <jesse at nicira.com> wrote:
>>>> On Thu, Jun 14, 2012 at 5:29 PM, ravi kerur <rkerur at gmail.com> wrote:
>>>>> On Thu, Jun 14, 2012 at 8:22 AM, Jesse Gross <jesse at nicira.com> wrote:
>>>>>> On Jun 14, 2012, at 10:13 PM, ravi kerur <rkerur at gmail.com> wrote:
>>>>>>
>>>>>>> On Thu, Jun 14, 2012 at 3:58 AM, Jesse Gross <jesse at nicira.com> wrote:
>>>>>>>> On Thu, Jun 14, 2012 at 1:24 PM, ravi kerur <rkerur at gmail.com> wrote:
>>>>>>>>> On Wed, Jun 13, 2012 at 7:58 PM, Jesse Gross <jesse at nicira.com> wrote:
>>>>>>>>>> On Thu, Jun 14, 2012 at 4:51 AM, ravi kerur <rkerur at gmail.com> wrote:
>>>>>>>>>>> There are additional things that needs to be addressed as well
>>>>>>>>>>>
>>>>>>>>>>> 1. offload code review, it's currently generic enough and getting near
>>>>>>>>>>> line rate performance numbers.
>>>>>>>>>>
>>>>>>>>>> You can't just take what is done for vlans and copy that.  There is
>>>>>>>>>> far too much code that you're adding to OVS.  Did you read what I
>>>>>>>>>> wrote earlier about where to start?:
>>>>>>>>>
>>>>>>>>> <rk> How is adding far too much code to ovs related to offload? It is
>>>>>>>>> handled similar to what vlan is doing for older kernel + in addition
>>>>>>>>> it takes care of handling copy + restore skb->protocol since
>>>>>>>>> skb_gso_segment relies on it and handle cases for non-gso packets to
>>>>>>>>> calculate checksum. I don't understand your comments, have you looked
>>>>>>>>> at latest patch?
>>>>>>>>
>>>>>>>> The vlan code that's there is backporting and emulation for various
>>>>>>>> quirks of vlans on different kernels.  Most of these don't apply to
>>>>>>>> MPLS because no version of Linux supports MPLS.  You can't start from
>>>>>>>> the backported version though, you need to begin with the correct way
>>>>>>>> to do things assuming that you have freedom to modify the upstream
>>>>>>>> kernel because OVS is upstream now and that's the future.  Once you
>>>>>>>> have things working there, you can backport to older versions but if
>>>>>>>> you do it in the opposite order you just end up with a mess.  Once
>>>>>>>> again, did you read what I wrote below?  I know that your code isn't
>>>>>>>> correct just by looking at the diff stat because you didn't modify the
>>>>>>>> file that I told you is the place to start.
>>>>>>>
>>>>>>> <rk> comments below
>>>>>>>>
>>>>>>>>>> Generally speaking the emulation code is handled by skb_gso_segment()
>>>>>>>>>> in dev.c in the kernel code outside of OVS.  This should mostly work
>>>>>>>>>> except that it needs to be able to detect that MPLS requires
>>>>>>>>>> emulation.  This will be the easiest part to get working and is the
>>>>>>>>>> best place to start.  However, in order for this code to work on any
>>>>>>>>>> kernel before your changes get integrated (i.e. Linux 3.6 at the
>>>>>>>>>> earliest) you'll have to emulate it in OVS as well, like we do for
>>>>>>>>>> vlans in vport-netdev.c.
>>>>>>>
>>>>>>> <rk> From the above paragraph, I deciphered it as emulate in ovs since
>>>>>>> it needs to work with any kernel version(vport-netdev.c) and then in
>>>>>>> dev.c and that's what I have done. Modified vport-netdev.c to support
>>>>>>> mpls and qinq and for any kernel version. However, it looks like you
>>>>>>> wanted me to work on dev.c first and later emulate in ovs via
>>>>>>> vport-netdev.c?
>>>>>>
>>>>>> Yes.
>>>>>
>>>>> <rk> ok thanks, will work on it. Just thinking about this, why do you
>>>>> want to modify upstream code dev.c first and not ovs? Atleast it makes
>>>>> sense to work on ovs first since it covers older version + current
>>>>> version and via macro LINUX_VERSION_CODE can be controlled on which
>>>>> version it's enabled and later handle upstream code(so long as it's
>>>>> made sure code gets in upstream on targeted version). Moreover,
>>>>> changes in dev.c and vport-netdev.c are completely different and
>>>>> cannot be treated as a backport, instead they should be treated as two
>>>>> separate changes.
>>>>
>>>> You must treat it as a backport.  If you look at it as a completely
>>>> separate path then you'll almost certainly end up with a different
>>>> model for how to do things and it will make it more complicated than
>>>> necessary because other parts of the code will need to support both.
>>>> Furthermore, the upstream version is going to be simpler so that's
>>>> where you want to be choosing the right model.
>>>>
>>>> Remember, we have to keep both versions in sync over time and the more
>>>> deviation there is the harder this gets.
>>>
>>> <rk> I am not sure I following your argument (first paragraph), but I
>>> definitely agree with keeping both ovs emulation and upstream in sync.
>>> Let me ask couple of questions to clarify things.
>>>
>>> Please note code path is the same until netdev_send is called where
>>> decision is made to use ovs emulation or kernel based on kernel
>>> version.
>>>
>>> ovs emulation supporting mpls and qinq
>>>
>>> 1. first clear hardware netif_* flags
>>> 2. get gso segments which is a list
>>> 3. skb_gso_segment relies on l3/l4 protocol information, save and restore
>>> 4. iterate through the loop of segments and transmit each one of them
>>>
>>> since this is a proven method I have stuck with this in ovs emulation.
>>> Are you suggesting to use different logic here, if yes, I don't
>>> understand why but would like to know what the argument is before
>>> making any changes?
>>
>> I want it to be based off the upstream version.  Since you haven't
>> done that yet, it's clearly not based on that.  That's really all
>> there is to it.
>>
>>> upstream kernel
>>> 1. ovs clears hardware netif_* flags before calling dev_queue_xmit
>>
>> OVS should not clear any flags.  It should initialize them as
>> appropriate for the protocol type.
>>
>>> 2. in skb_gso_segment (upstream kernel code), handle mpls or qinq
>>> eth_types such that correct l3 information is extracted
>>> 3.  change drivers to handle mpls or qinq eth_types to handle checksum offloads.
>>
>> Updating drivers is not necessary at this time.
>>
>>> As I see that changes are different for both(agree upstream kernel
>>> code changes are simpler), I was merely suggesting not to treat them
>>> as backport and code review can happen in parallel.
>>
>> I'm not going to review this until the upstream version exists because
>> I'm going to review the OVS version by comparing it to upstream.  This
>> is how every other feature has been added to OVS, which now supports
>> over 20 different kernel versions.  I don't see any reason why this is
>> different.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: linux_mpls_qinq_changes
Type: application/octet-stream
Size: 4817 bytes
Desc: not available
URL: <http://openvswitch.org/pipermail/dev/attachments/20120626/cb3e6b24/attachment.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-OF-1.1-and-1.3-MPLS-changes.patch
Type: text/x-patch
Size: 172596 bytes
Desc: not available
URL: <http://openvswitch.org/pipermail/dev/attachments/20120626/cb3e6b24/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-OF-1.1-VLAN-QinQ-changes.patch
Type: text/x-patch
Size: 137364 bytes
Desc: not available
URL: <http://openvswitch.org/pipermail/dev/attachments/20120626/cb3e6b24/attachment-0001.bin>