[ovs-dev] [PATCH v4 3/3] datapath: add layer 3 flow/port support

Lori Jakab lojakab at cisco.com
Fri Aug 1 22:19:23 UTC 2014


On 8/1/14, 3:25 AM, Jesse Gross wrote:
> On Thu, Jul 31, 2014 at 5:45 AM, Lori Jakab <lojakab at cisco.com> wrote:
>> On 7/31/14, 10:09 AM, Jesse Gross wrote:
>>> On Fri, Jun 27, 2014 at 6:21 AM, Lorand Jakab <lojakab at cisco.com> wrote:
>>>> diff --git a/datapath/actions.c b/datapath/actions.c
>>>> index cb26ad5..20c66f5 100644
>>>> --- a/datapath/actions.c
>>>> +++ b/datapath/actions.c
>>>> +static int pop_eth(struct sk_buff *skb)
>>>> +{
>>>> +       skb_pull_rcsum(skb, skb_network_offset(skb));
>>>> +       skb_reset_mac_header(skb);
>>>> +       vlan_set_tci(skb, 0);
>>>> +
>>>> +       OVS_CB(skb)->is_layer3 = true;
>>> I think we should probably also set skb->mac_len to 0 here (and on
>>> push as well).
>>
>> OK.  I made the following changes:
>>
>> --- a/datapath/actions.c
>> +++ b/datapath/actions.c
>> @@ -245,6 +245,7 @@ static int pop_eth(struct sk_buff *skb)
>>   {
>>          skb_pull_rcsum(skb, skb_network_offset(skb));
>>          skb_reset_mac_header(skb);
>> +       skb->mac_len = 0;
>>          vlan_set_tci(skb, 0);
> I realized that setting the mac_len to zero isn't correct in the
> presence of MPLS. I think we can just use skb_reset_mac_len() here as
> well.
>
> This also means that we shouldn't pull up to the network offset since
> this will also pull off any MPLS labels, which I don't think that we
> want.
>
> Going further, what happens if packet comes in with multiple Ethernet
> headers on it (such as PBB)? This will pull off all of the headers but
> that doesn't seem right. If we were to only pull off one, what are the
> semantics with respect to VLANs?

I have a feeling we may need to rename this action, since the my 
intention actually was to remove everything up to the layer 3 header.  
That includes MPLS header(s) and any number of Ethernet headers.  The 
reason is, I need an action that guarantees that the resulting packet is 
layer 3, and can be sent through a layer 3 port.  I think you're right 
that the expectation from a pop_eth() action would be what you describe 
above: if more then one Ethernet header is present, remove the outer 
header only, keep MPLS headers.

How about renaming this action to extract_l3_packet() or similar?
>
>>>> +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);
>>>> +
>>>> +       ether_addr_copy(eth_hdr(skb)->h_source, ethh->addresses.eth_src);
>>>> +       ether_addr_copy(eth_hdr(skb)->h_dest, ethh->addresses.eth_dst);
>>>> +
>>>> +       eth_hdr(skb)->h_proto = ethh->eth_type;
>>>> +       skb->protocol = ethh->eth_type;
>>>> +
>>>> +       ovs_skb_postpush_rcsum(skb, skb->data, ETH_HLEN);
>>>> +
>>>> +       OVS_CB(skb)->is_layer3 = false;
>>> What happens if there is something in skb->vlan_tci? I think the
>>> effect will be that it still on the outer Ethernet header, which
>>> doesn't seem correct.
>>
>> I missed that.  Addressed with:
>>
>> --- 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.

>
>>>> diff --git a/datapath/datapath.h b/datapath/datapath.h
>>>> index fcd8e86..07ae0c8 100644
>>>> --- a/datapath/datapath.h
>>>> +++ b/datapath/datapath.h
>>>> @@ -107,6 +107,7 @@ struct ovs_skb_cb {
>>>>           struct sw_flow_key      *pkt_key;
>>>>           struct ovs_tunnel_info  *tun_info;
>>>>           struct vport    *input_vport;
>>>> +       bool is_layer3;
>>> We have run into a problem with ovs_skb_cb: it is currently full on
>>> kernels < 3.11 due to compatibility code.
>>
>> What can we do about it?
> It may actually be possible to remove 'flow' from ovs_skb_cb. It is
> really only used in ovs_execute_actions() and it should be easy enough
> to pass in the actions as an argument there.

Should I propose a separate patch to do this?

>
>>>> @@ -600,8 +601,10 @@ static int ovs_key_from_nlattrs(struct sw_flow_match
>>>> *match, u64 attrs,
>>>>                                   eth_key->eth_src, ETH_ALEN, is_mask);
>>>>                   SW_FLOW_KEY_MEMCPY(match, eth.dst,
>>>>                                   eth_key->eth_dst, ETH_ALEN, is_mask);
>>>> +               SW_FLOW_KEY_PUT(match, phy.noeth, false, is_mask);
>>>>                   attrs &= ~(1ULL << OVS_KEY_ATTR_ETHERNET);
>>>> -       }
>>>> +       } else if (!is_mask)
>>>> +               SW_FLOW_KEY_PUT(match, phy.noeth, true, is_mask);
>>> This will never set a mask for flows without an Ethernet header, which
>>> means that an IP flow will match things both with and without a
>>> header. Is that the intention? (It seems to me that it would be better
>>> to force a match on whatever form we are using - this could result in
>>> additional flow setups for mixed traffic but it seems unlikely that
>>> this wouldn't be forced already by something else, like the port
>>> number.)
>>
>> I removed the "if".
> I still have a question to some degree. If I have a packet that came
> in on a physical Ethernet interface and I wildcard the addresses,

You mean the src/dst MAC addresses?

> should the resulting flow match a packet that came in on a pure L3
> port? It conceivable could but are there any issues that might come
> up? (in particular, are there any issues with multiple Ethernet
> headers, etc.?)

I think if the flow key has wildcarded Ethernet addresses, the flow 
should match only on L2 packets.

BTW, for a packet with multiple Ethernet headers, is there already 
support for matching on anything after the outer header?  Does that need 
recirculation?

>
> Somewhat related, what happens if someone specifies just an EtherType
> but no Ethernet addresses? Maybe we should just reject this?

In a flow key?  I don't have a strong opinion on this, but I think I 
wouldn't reject it.

>
> The other changes look OK to me.



More information about the dev mailing list