From: Thomas F Herbert <thomasfherbert@gmail•com>
To: Pravin Shelar <pshelar@nicira•com>,
Thomas F Herbert <therbert@redhat•com>
Cc: netdev <netdev@vger•kernel.org>,
"dev@openvswitch•org" <dev@openvswitch•org>
Subject: Re: [PATCH net-next V17 3/3] 802.1AD: Flow handling, actions, vlan parsing and netlink attributes
Date: Fri, 23 Oct 2015 15:42:46 -0400 [thread overview]
Message-ID: <562A8DB6.8000006@gmail.com> (raw)
In-Reply-To: <CALnjE+puu=KCFPjYXQv=HyNpmVuisKCTNFxXz9eN_=VU9O7JeA@mail.gmail.com>
On 10/23/15 3:26 PM, Pravin Shelar wrote:
> On Wed, Oct 21, 2015 at 7:39 AM, Thomas F Herbert <therbert@redhat•com> wrote:
>>
>> On 10/20/15 4:34 PM, Pravin Shelar wrote:
>>> On Tue, Oct 20, 2015 at 7:26 AM, Thomas F Herbert
>>> <thomasfherbert@gmail•com> wrote:
>>>> On 10/19/15 2:28 PM, Pravin Shelar wrote:
>>>>> On Sat, Oct 17, 2015 at 6:12 PM, Thomas F Herbert
>>>>> <thomasfherbert@gmail•com> wrote:
>>>>>> Add support for 802.1ad including the ability to push and pop double
>>>>>> tagged vlans. Add support for 802.1ad to netlink parsing and flow
>>>>>> conversion. Uses double nested encap attributes to represent double
>>>>>> tagged vlan. Inner TPID encoded along with ctci in nested attributes.
>>>>>>
>>>>>> Signed-off-by: Thomas F Herbert<thomasfherbert@gmail•com>
>>>>>> ---
>>>>>> net/openvswitch/actions.c | 6 +-
>>>>>> net/openvswitch/flow.c | 76 +++++++++++++-----
>>>>>> net/openvswitch/flow.h | 8 +-
>>>>>> net/openvswitch/flow_netlink.c | 172
>>>>>> +++++++++++++++++++++++++++++++++++++----
>>>>>> net/openvswitch/vport-netdev.c | 4 +-
>>>>>> 5 files changed, 227 insertions(+), 39 deletions(-)
>>>>>>
>>>>>> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
>>>>>> index 315f533..09cc1c9 100644
>>>>> ...
>>>>>
>>>>>> diff --git a/net/openvswitch/flow_netlink.c
>>>>>> b/net/openvswitch/flow_netlink.c
>>>>>> index c92d6a2..97a6d12 100644
>>>>>> --- a/net/openvswitch/flow_netlink.c
>>>>>> +++ b/net/openvswitch/flow_netlink.c
>>>>> ...
>>>>>
>>>>>> @@ -1320,6 +1443,7 @@ static int __ovs_nla_put_key(const struct
>>>>>> sw_flow_key *swkey,
>>>>>> {
>>>>>> struct ovs_key_ethernet *eth_key;
>>>>>> struct nlattr *nla, *encap;
>>>>>> + struct nlattr *in_encap = NULL;
>>>>>>
>>>>>> if (nla_put_u32(skb, OVS_KEY_ATTR_RECIRC_ID,
>>>>>> output->recirc_id))
>>>>>> goto nla_put_failure;
>>>>>> @@ -1368,17 +1492,29 @@ static int __ovs_nla_put_key(const struct
>>>>>> sw_flow_key *swkey,
>>>>>> ether_addr_copy(eth_key->eth_src, output->eth.src);
>>>>>> ether_addr_copy(eth_key->eth_dst, output->eth.dst);
>>>>>>
>>>>>> - if (swkey->eth.tci || swkey->eth.type == htons(ETH_P_8021Q)) {
>>>>>> - __be16 eth_type;
>>>>>> - eth_type = !is_mask ? htons(ETH_P_8021Q) :
>>>>>> htons(0xffff);
>>>>>> - if (nla_put_be16(skb, OVS_KEY_ATTR_ETHERTYPE, eth_type)
>>>>>> ||
>>>>>> - nla_put_be16(skb, OVS_KEY_ATTR_VLAN,
>>>>>> output->eth.tci))
>>>>>> + if (swkey->eth.vlan.tci || eth_type_vlan(swkey->eth.type)) {
>>>>>> + if (nla_put_be16(skb, OVS_KEY_ATTR_ETHERTYPE,
>>>>>> + output->eth.vlan.tpid) ||
>>>>>> + nla_put_be16(skb, OVS_KEY_ATTR_VLAN,
>>>>>> output->eth.vlan.tci))
>>>>>> goto nla_put_failure;
>>>>>> encap = nla_nest_start(skb, OVS_KEY_ATTR_ENCAP);
>>>>>> - if (!swkey->eth.tci)
>>>>>> + if (!swkey->eth.vlan.tci)
>>>>>> goto unencap;
>>>>>> - } else
>>>>>> + if (swkey->eth.cvlan.tci) {
>>>>>> + /* Customer tci is nested but uses same key
>>>>>> attribute.
>>>>>> + */
>>>>>> + if (nla_put_be16(skb, OVS_KEY_ATTR_ETHERTYPE,
>>>>>> + output->eth.cvlan.tpid) ||
>>>>>> + nla_put_be16(skb, OVS_KEY_ATTR_VLAN,
>>>>>> + output->eth.cvlan.tci))
>>>>>> + goto nla_put_failure;
>>>>>> + in_encap = nla_nest_start(skb,
>>>>>> OVS_KEY_ATTR_ENCAP);
>>>>>> + if (!swkey->eth.cvlan.tci)
>>>>>> + goto unencap;
>>>>>> + }
>>>>>> + } else {
>>>>>> encap = NULL;
>>>>>> + }
>>>>> After the vlan parsing changes, we need to encode cvlan in outer
>>>>> netlink attribute and then encode regular vlan.
>>>> I don't understand this. cvlan is inner vlan, why would it be encoded in
>>>> outer vlan without the 2nd layer of encapsulation?
>>>>
>>> Lets start with double tagged packet.
>>> packet: eth, vlan 10, inner vlan 20, ip.
>>> flow key would be:
>>> flow_key: vlan = 10, cvlan = 20
>>> That would get serialize over netlink like:
>>>
>>> eth_type(0x8100),vlan(vid=10,pcp=0),encap(eth_type(0x8100),vlan(vid=20,pcp=0),
>>> encap(eth_type(0x0800),ipv4(frag=no))...
>>> So far looks good.
>>>
>>> Now userspace sends back same key and installs flow in kernel
>>> datapath. But ovs_nla_get_match() parses netink key and sets vlan in
>>> reverse order. After parsing netlink in ovs_nla_get_match() vlans
>>> flow-key would look like:
>>> flow_key: vlan = 20, cvlan = 10. This is not what we started with.
>>>
>>> Now I think rather than fixing __ovs_nla_put_key (), we need to fix
>>> ovs_nla_get_match() to keep vlan order.
>>> I also noticed that eth.vlan.tpid is never initialized from netlink
>>> attributes.
>> Yes, you are right. The code in master never encoded the tpid in the key and
>> I didn't add it when I modified the swkey for the addition of the vlan
>> struct.
>>
>> Yes, you are right the code is wrong. The intention was to leave the old
>> code which left the outer tci in the key for later processing. In the code
>> in master, after checking proper vlan the function ovs_key_from_nlattrs()
>> encoded the tci.
>>
>> Now I see that I must refactor the code in ovs_nla_get_match() and
>> parse_vlan_from_nlattrs to explicitly encode the outer vlan including tpid
>> before the inner vlan.
>>> Call to parse_flow_nlattrs in parse_vlan_from_nlattrs() has no effect
>>> so it should be removed
>> parse_flow_nlattrs() is called for each level of encapsulation. It sets
>> v_attrs with the keys found in the flow key. This is to insure that I am
>> looking only at the keys for the current level of encapsulation. Then it is
>> called a final time to parse out the l3 attributes for encoding.
> ovs_nla_get_match() calls parse_flow_nlattrs() and then again in
> parse_vlan_from_nlattrs(), second parse_flow_nlattr() can potentially
> overwrite vlan attributes. Therefore it should called after the outer
> vlan attributes are read.
Thanks! Yes, I agree. I am fixing this with the refactoring I am doing now.
prev parent reply other threads:[~2015-10-23 19:42 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-10-18 1:12 [PATCH net-next V17 0/3] openvswitch: Add support for 802.1ad Thomas F Herbert
2015-10-18 1:12 ` [PATCH net-next V17 1/3] openvswitch: 802.1ad uapi changes Thomas F Herbert
[not found] ` <1445130748-27671-2-git-send-email-thomasfherbert-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2015-10-19 18:19 ` Pravin Shelar
[not found] ` <1445130748-27671-1-git-send-email-thomasfherbert-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2015-10-18 1:12 ` [PATCH net-next V17 2/3] Check for vlan ethernet types for 8021.q or 802.1ad Thomas F Herbert
[not found] ` <1445130748-27671-3-git-send-email-thomasfherbert-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2015-10-19 18:20 ` Pravin Shelar
2015-10-18 1:12 ` [PATCH net-next V17 3/3] 802.1AD: Flow handling, actions, vlan parsing and netlink attributes Thomas F Herbert
2015-10-19 18:28 ` Pravin Shelar
2015-10-20 14:26 ` Thomas F Herbert
[not found] ` <56264F12.1050004-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2015-10-20 20:34 ` Pravin Shelar
[not found] ` <CALnjE+rjNNwvuRRa2hktwHukTvnTx=AC4PegiAvwOT9UGtZKFw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2015-10-21 14:39 ` Thomas F Herbert
[not found] ` <5627A3A9.5010708-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
2015-10-23 19:26 ` Pravin Shelar
2015-10-23 19:42 ` Thomas F Herbert [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=562A8DB6.8000006@gmail.com \
--to=thomasfherbert@gmail$(echo .)com \
--cc=dev@openvswitch$(echo .)org \
--cc=netdev@vger$(echo .)kernel.org \
--cc=pshelar@nicira$(echo .)com \
--cc=therbert@redhat$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox