On Wed, Jun 13, 2012 at 5:55 AM, Jesse Gross <je...@nicira.com> wrote:
> On Jun 13, 2012, at 12:12 PM, ravi kerur <rke...@gmail.com> wrote:
>
>> Thanks Pravin for the review comments. Inline <rk>
>>
>> On Tue, Jun 12, 2012 at 5:31 PM, Pravin Shelar <pshe...@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)
>
> skb->csum is used on the receive side by NICs that just generically compute a
> checksum over the entire packet rather than giving a yes/no on the validity.
> You must update it as you change the packet.
>
>
>>>
>>> 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
>
> It does save some time if you modify multiple header fields but that's not
> the main goal. The primary objective is to push as much complexity as
> possible to userspace and flow setup time. The kernel should just expose the
> simplest building blocks possible.
>
>> 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 spent a fair amount of time thinking about this today and concluded that
> neither option is very attractive as is.
>
> If we go with what you have now, I'm fairly confident that we will regret it
> in the future. The kernel code used to more directly implement various
> features and prior to upstreaming we broke many of them down. I'm happy with
> the results of that but this time we won't have the benefit of revising
> things later. This is particularly bad because it deviates from our usual
> model of userspace controlling everything and here userspace won't even know
> what the flow looks like. The effects of this tend to metastasize because
> when userspace doesn't know what the packet looks like it can't implement
> things that it might overwise be able to do and more and more ends up in the
> kernel. The other thing, which is specific to MPLS, is that there is no
> inherent way to know the type of the payload. Userspace is vastly more likely
> to have this information in the event that we want to do something with the
> inner packet. In your patch the kernel is basically assuming that the type
> is IP (OpenFlow doesn't give us any additional information but it doesn't
> seem like a good idea in general).
>
For now we can implement ttl_in and ttl_out only in userspace by not
installing flow if these are not very common actions used.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
