On Thu, Feb 28, 2013 at 12:57 AM, Simon Horman <ho...@verge.net.au> wrote:
> On Wed, Feb 27, 2013 at 07:39:02AM -0800, Jesse Gross wrote:
>> On Tue, Feb 26, 2013 at 11:10 PM, Simon Horman <ho...@verge.net.au> wrote:
>> > On Tue, Feb 26, 2013 at 02:49:47PM -0800, Jesse Gross wrote:
>> >> > diff --git a/datapath/flow.c b/datapath/flow.c
>> >> > index fad9e19..27e1920 100644
>> >> > --- a/datapath/flow.c
>> >> > +++ b/datapath/flow.c
>> >> > @@ -728,6 +728,17 @@ int ovs_flow_extract(struct sk_buff *skb, u16
>> >> > in_port, struct sw_flow_key *key,
>> >> > memcpy(key->ipv4.arp.tha, arp->ar_tha,
>> >> > ETH_ALEN);
>> >> > key_len = SW_FLOW_KEY_OFFSET(ipv4.arp);
>> >> > }
>> >> > + } else if (eth_p_mpls(key->eth.type)) {
>> >> > + error = check_header(skb, MPLS_HLEN);
>> >> > + if (unlikely(error))
>> >> > + goto out;
>> >> > +
>> >> > + key_len = SW_FLOW_KEY_OFFSET(mpls.top_lse);
>> >> > + memcpy(&key->mpls.top_lse, skb_network_header(skb),
>> >> > MPLS_HLEN);
>> >> > +
>> >> > + /* Update network header */
>> >> > + skb_set_network_header(skb, skb_network_header(skb) -
>> >> > + skb->data + MPLS_HLEN);
>> >>
>> >> Is it possible to actually use this network header pointer?
>> >
>> > It is possible to use it in ovs_flow_extract_l3_onwards() in conjunction
>> > with the inner/outer flow changes I have in other patches (and you have
>> > asked me to drop).
>> >
>> > It does seem to me that it may be incorrect if there is more than one MPLS
>> > LSE present in the frame.
>> >
>> > But I'm not sure if either of those answers relate to the point you are
>> > making. Are there use-cases which concern you?
>>
>> I was just unsure of when it would be useful. It seems like if we're
>> not going to try to act beyond the MPLS labels that we can parse then
>> it makes the most sense to just leave the network layer unset (maybe
>> we can use it to point to the start of the MPLS stack instead of
>> needing a special l2 length then?).
>
> I think that removing the call to skb_set_network_header() you have
> isolated above is harmless enough as I don't think it is used because
> if frame is MPLS then no l3+ processing is done.
>
> I looked at making use of the network header to mark the top of
> the MPLS stack and not adding the special l2 length, however, I believe
> it breaks down in the situation I will now describe.
>
> Suppose an IP frame has two actions applied, dec_ttl and push_mpls.
> This is valid because an IP match may be made and ovs-vswitchd
> can install set(ip) and push_mpls actions in the datapath.
>
> Now, for some reason that I must confess that I don't fully understand
> at this moment the actions are installed as push_mpls and then set(ip).
>
> I believe that for the scheme you suggest above to work push_mpls (and
> pop_mpls) need to update the network_header such that it always points to
> the to of the MPLS stack. This is to allow multiple mpls actions to behave
> correctly, e.g. mpls_push then mpls_set.
>
> The problem being that set(ip) uses the network_header and if it has been
> set to the top of the MPLS stack by push_mpls then the set(ip) action will
> corrupt the packet.
I see. The reason why the actions are output like this is because
userspace simply accumulates changes to the flow until they can
possibly have an effect (when a packet is output) and then applies the
minimal set of actions to transform the packet from its current state
into the desired state. Since the changes up to this point have all
been independent of each other, ordering doesn't matter and userspace
simply creates the actions in a fixed (but arbitrary) order.
I think it would be reasonable and fairly easy to enforce the
constraint that userspace must emit actions in the correct order (IP
then MPLS). After all, if a packet arrived as MPLS then we wouldn't
allow a set IP action so it doesn't really make sense to support push
MPLS and then set IP since it is effectively the same thing.
Does that avoid the problem? The more that I think about it, the
nicer it seems to use the network header pointer here.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
