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

Jesse Gross jesse at nicira.com
Sat Aug 2 01:11:56 UTC 2014


On Fri, Aug 1, 2014 at 3:19 PM, Lori Jakab <lojakab at cisco.com> wrote:
> 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?

I'm not excited about this. Thomas Morin was already talking about
using this code to do MPLS over GRE, which would need exactly the
semantics of popping an Ethernet header. I would like to find a way to
do this now, otherwise it is likely that we will have to solve the
same problem with another new action very soon.

>>>>> +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.

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.

>>>>> 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?

Sure.

>>>>> @@ -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?

Yes.

>> 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.

In that case, I think we need to always unwildcard 'noeth' rather than
making it conditional on whether you have a match on the MAC
addresses.

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

There isn't currently any support on matching on multiple headers but
it seems like a natural possibility if we introduce the capability to
push/pop Ethernet headers. Once those are there, it seems like you
could do it with 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.

Is there any situation where it could be used (what meaning would it
have)? Normally we reject things that can never make sense.



More information about the dev mailing list