Hi Jesse,
Sorry for the long delay in replying to the thread, was busy with
OpenDaylight. See inline below:
On 9/12/14 12:26 AM, Jesse Gross wrote:
On Mon, Sep 8, 2014 at 5:43 AM, Lori Jakab <loja...@cisco.com> wrote:
On 8/25/14 3:33 AM, Jesse Gross wrote:
On Thu, Aug 21, 2014 at 12:24 PM, Lori Jakab <loja...@cisco.com> wrote:
On 8/6/14 4:37 AM, Jesse Gross wrote:
Besides the fact that it would be nice to unify things, I'm not sure
that it is actually correct to pull off VLAN, MPLS, etc. tags that we
don't understand. Presumably, these are being used to create some kind
of isolation and dropping all the tags that we don't understand would
just blindly merge things together. Also, from an implementation
perspective, we don't really parse beyond what we understand (this is
not quite true for MPLS but it is for VLANs and any unknown protocols)
so we wouldn't necessarily actually get to an L3 header. On the other
hand, if we only deal with tags that we do understand then can't we
have more atomic instructions that pull them off one-by-one?
Atomic instructions make a lot of sense, and we definitely should support
them. However, if a packet is sent to an L3 port, it should not have any
Ethernet, VLAN or MPLS header. Note that for now these actions are not
user
visible, and cannot be added with OpenFlow, they are added automatically
by
the user space code, when a packet is sent from an L2->L3 port or L3->L2
port.
I agreed with Ben that this behavior will be changed in the future, based
on
discussions in the ONF's EXT-112 proposal, where the consensus was to
have
explicit push_eth and pop_eth OpenFlow actions. However, the current
behavior of the LISP code is to add/remove L2 headers automatically, so
we
want to keep that in the first phase, until I implement OpenFlow support.
Once automatic header popping is not used, we can remove the
"extract_l3_packet()" action.
From the implementation perspective, this action would rely on the
skb->network_header being set correctly.
I'm not sure that this really addresses the concerns that I have from
both policy and implementation perspectives.
From the policy perspective: Will a Cisco implementation of LISP
accept a packet on an unknown VLAN, throw away the tag when it does L2
processing, and then continue on to do L3 processing and LISP? Somehow
I doubt it.
From an implementation perspective: There is no mechanism to rely on
skb->network_header being set in the general case. It will only be set
in situations where OVS can parse the packet. OVS doesn't know about
QinQ for example so you the action that you have defined won't
actually give you an L3 packet.
Finally, from a Linux perspective, removing kernel interfaces isn't
something that is OK as a matter of policy.
The fact that the OVS kernel module doesn't know how to inherently
find the L3 is not as bad as it seems. Conveniently, it means that we
can pop off exactly the same headers as we know how to parse so there
is no loss of functionality to doing this individually. As bonus,
since userspace needs to generate these actions, the policy can be
enforced there as well.
All compelling arguments, so I agree, let's not add an "extract_l3_packet()"
action and have pop_eth() only remove an Ethernet header. VLAN tags, if
any, should be removed by a previous pop_vlan() action before calling
pop_eth(). Does this implementation look correct to you?
--- a/datapath/actions.c
+++ b/datapath/actions.c
@@ -421,9 +421,9 @@ static int set_eth_addr(struct sk_buff *skb,
static int pop_eth(struct sk_buff *skb)
{
- skb_pull_rcsum(skb, skb_network_offset(skb));
+ skb_pull_rcsum(skb, ETH_HLEN);
skb_reset_mac_header(skb);
- vlan_set_tci(skb, 0);
+ skb->mac_len -= ETH_HLEN;
OVS_CB(skb)->is_layer3 = true;
--- a/datapath/flow_netlink.c
+++ b/datapath/flow_netlink.c
@@ -1831,6 +1831,8 @@ static int __ovs_nla_copy_actions(const struct nlattr
*attr,
case OVS_ACTION_ATTR_POP_ETH:
if (noeth)
return -EINVAL;
+ if (vlan_tci != htons(0))
+ return -EINVAL;
I wonder if this check is necessary/correct? It seems like there could
be other equivalent L2 tags that we don't know about, like QinQ.
Yes, but those tags should be removed before a pop_eth() action, not as
the part of the pop_eth() action. At least that was my understanding to
the above discussion. So I added that conditional to "enforce" that we
have a "clean" Ethernet header.
I think is_layer3 in the control block should not be used on output anymore,
since we cannot guarantee it is correct. So another change I suggest is
removing the check for a packet being layer 2 or layer 3 in the various
${vport}-send() functions, and keep its use limited to receiving.
That generally makes sense to me. For things like LISP would you then
check to make sure that this is an IPv4/v6 packet before
encapsulation?
Well, the lisp_send() function in vport-lisp.c only receives the vport
and sk_buff as parameters, and OVS_CB doesn't have the flow key
anymore. A patch I proposed initially just removed the the 'flow'
member in ad50cb605b24495956b6a7664d379abd3912ed50, but I see the
'pkt_key' member was removed later by Pravin:
commit e74d48171e590b50cdcb2798a3e7c5c32313887b
Author: Pravin B Shelar <pshe...@nicira.com>
Date: Wed Sep 17 18:58:44 2014 -0700
datapath: Remove pkt_key from OVS_CB.
OVS keeps pointer to packet key in skb->cb, but the packet key is
store on stack. This could make code bit tricky. So it is better to
get rid of the pointer.
Signed-off-by: Pravin B Shelar <pshe...@nicira.com>
Acked-by: Andy Zhou <az...@nicira.com>
So I don't have the necessary information there to do the check.
If so, could we apply the same mechanism on the receive
side to completely avoid the need for the is_layer3 flag (since I
think it would then only be read in a single place)?
I need the is_layer3 flag to be able to parse the packet correctly in
key_extract() in flow.c. Not sure if there is another way to do it,
especially without a flow key in SKB_CB.
--- a/datapath/actions.c
+++ b/datapath/actions.c
@@ -258,6 +258,7 @@ static void push_eth(struct sk_buff *skb, const
struct
ovs_action_push_eth *ethh
skb_push(skb, ETH_HLEN);
skb_reset_mac_header(skb);
skb_reset_mac_len(skb);
+ vlan_set_tci(skb, 0);
I don't think it's right to throw away the vlan tag. My assumption
is
that it should be placed into the packet before we push on the
Ethernet header (this actually should never happen since we disallow
pushing multiple Ethernet headers but it still seems like the right
thing to do).
That's not what I would expect from a simple push_eth() action. If
someone
wanted to push a VLAN tag, that should be an explicit action. So I
would
just throw away whatever there is in skb->vlan_tci.
The contents of skb->vlan_tci were already pushed explicitly (look at
the implementation of push_vlan()). From a user's perspective, there
is no difference - it is just an internal optimization to pull the tag
out of the packet, so I can't see any justification for throwing away
that tag.
OK. I will remove that line.
But just to be clear, I think we still need to explicitly push the
vlan tag on the packet. Otherwise, it will migrate from the inner
Ethernet header to the outer.
Still not sure what I need to do here. If skb->vlan_tci was set by a
push_vlan action, then the vlan tag should already have been pushed on
the
packet, right?
Yes, the user has requested that a vlan tag be applied. skb->vlan_tci
is a form of offloading that should be transparent and will no longer
work in this case, so it needs to be de-offloaded. Please take a look
at the OVS vlan operations and other vlan code in the kernel if this
isn't clear.
I wasn't aware of how hardware offloading of VLAN tags worked. How about
the following incremental?
That looks basically right to me although since the code is very
similar to the start of push_vlan(), I would probably pull it out as a
separate function.
Done, I pulled out the common code as a separate function.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
