[ovs-dev] MPLS and VLAN QinQ patch

[ravi kerur]

Thanks Pravin for the review comments. Inline <rk>

On Tue, Jun 12, 2012 at 5:31 PM, Pravin Shelar <pshelar at nicira.com> wrote:
> Hi Ravi,
>
> I see following issues with datapath mpls support:
>
> Actions:
> I do not see skb->csum updated when mpls action is performed on packet.

<rk> csum calculation is usually done in the kernel stack and I leave
like that. Moreover, there could be multiple mpls actions for a single
header and hence I thought it would not be efficient to do it
everytime in ovs but rather rely on kernel during transmit. Please let
me know otherwise.

Note:(I have verified via tcpdump that packets are transmitted with
correct checksum)
>
> Push / Set_lse action:
> You can compute mpls_lse value flow setup time in userspace. That will
> save time in every packet switching plus it simplifies code. we can
> easily do it for single label case.

<rk> I had run into issues when I was handling modification in
userspace and just update in kernel, will look into it.
>
> TTL actions:
> As Jesse has mentioned before there could be one Set action which
> should be able to handle three different ttl related actions added.
> e.g. OVS_ACTION_ATTR_COPY_TTL_IN can generate set-ipv4 action from
> userspace. OVS_ACTION_ATTR_COPY_TTL_OUT and
> OVS_ACTION_ATTR_DEC_MPLS_TTL can be set mpls action. This works for
> single label case. But ttl_in and ttl_out also needs mpls inner packet
> parsing in flow_extract to get exact flow in kernel datapath.
>

<rk> I had disagreed with Jesse for this approach and unfortunately
our discussion didn't continue since he was busy. My understanding is
that you use set actions when you are modifying multiple fields in a
single header and it makes perfect sense to me. In case of mpls we are
dealing with 2 headers ip and mpls

for e.g.

copy_ttl_in could be mpls-outer-header to mpls-inner-header (in case
of stacked labels) and mpls-header to ip for single label.

copy_ttl_out could be mpls-inner-header to mpls-outer-header(stacked
labels) and ip to mpls-header for signle label

dec_mpls_ttl, single header operation.

hence I think it should be left as is. Let me know if I have
misunderstood set operations.

I am not sure whether we should do take inner mpls header parsing
approach in kernel because it deviates from the spec. The only
deviation so far in the code v/s spec is handling of ttl and tos bits
for inner-most mpls header and it is done that way because of
practical deployed use-case.

Thanks,
Ravi

> Thanks,
> Pravin.
>
> On Tue, May 22, 2012 at 10:36 AM,  <Ravi.Kerur at telekom.com> wrote:
>> Hi,
>>
>>
>>
>> Attached MPLS and VLAN QinQ patch after rebasing to following commit
>>
>>
>>
>> commit 046f1f89e6d7716581de207dd0c54421926bc25b
>> Author: Ethan Jackson <ethan at nicira.com>
>> Date:   Mon May 21 13:20:18 2012 -0700
>>
>>
>>
>> Patch(s) have undergone additional integration testing and incorporates
>> initial code review comments from Ben.
>>
>>
>>
>> Thanks,
>>
>> Ravi
>>
>>
>> _______________________________________________
>> dev mailing list
>> dev at openvswitch.org
>> http://openvswitch.org/mailman/listinfo/dev
>>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev