Re: [PATCH net-next V17 3/3] 802.1AD: Flow handling, actions, vlan parsing and netlink attributes
From: Pravin Shelar <hidden>
Date: 2015-10-20 20:34:46
On Tue, Oct 20, 2015 at 7:26 AM, Thomas F Herbert [off-list ref] wrote:
On 10/19/15 2:28 PM, Pravin Shelar wrote:quoted
On Sat, Oct 17, 2015 at 6:12 PM, Thomas F Herbert [off-list ref] wrote:quoted
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 <redacted> --- 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...quoted
diff --git a/net/openvswitch/flow_netlink.cb/net/openvswitch/flow_netlink.c index c92d6a2..97a6d12 100644--- a/net/openvswitch/flow_netlink.c +++ b/net/openvswitch/flow_netlink.c...quoted
@@ -1320,6 +1443,7 @@ static int __ovs_nla_put_key(const structsw_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 structsw_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. Call to parse_flow_nlattrs in parse_vlan_from_nlattrs() has no effect so it should be removed.
One think I see I should have done is check for eth_type_vlan() on the inner tag as well.quoted
Currently we are reversing netlink encoding while sending back the vlan attributes. cvlan and vlan is independent,Are you talking about a corner case where the incoming packet has an inner vlan but no outer vlN?quoted
nope, this is usual case.
quoted
therefore we need to check swkey->eth.cvlan.tc outside of swkey->eth.vlan.tci block. Also redundant check for swkey->eth.cvlan.tci should be removedI must be missing something because I don't understand this either. First my patch encodes the outer vlan and then sets the encap bit and then encodes the inner vlan. The encoding shows up correctly in user space.quoted
Once ovs_nla_get_match() is fixed, there is no need to change this
function to fix that ordering.
I was talking about following redundant check:
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);quoted
quoted
quoted
quoted
quoted
quoted
quoted
if (!swkey->eth.cvlan.tci) <<<<<<<<<<<<<
goto unencap;
}
quoted
Can you also post changes for userspace vswitchd so that I can try next patch.I posted a patch for user space (V14) to ovs dev on October 2nd. I haven't made any changes since then. http://openvswitch.org/pipermail/dev/2015-October/060874.html
That patch does not apply on OVS master. You could also send me link to ovs userpsace github repo with these patches applied so that I can try kernel datapath changes. _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev