| Message ID | 20170119005304.GA39232@cran64.bj.intel.com |
|---|---|
| State | Not Applicable |
| Headers | show |
On Wed, Jan 18, 2017 at 4:53 PM, Yang, Yi <yi.y.yang@intel.com> wrote: > On Wed, Jan 18, 2017 at 01:29:14PM -0800, Joe Stringer wrote: >> On 18 January 2017 at 11:54, Eric Garver <e@erig.me> wrote: >> > On Tue, Jan 17, 2017 at 12:37:19AM +0000, Yang, Yi Y wrote: >> >> What userspace do "802.1ad patches" depend on? Per Pravin's statement, we just backport 802.1ad patches to ovs, then the below patch can be applied to ovs. >> > >> > Userspace does not yet have support for 802.1ad. I'm still working on >> > it. You can check the list archives for a recent RFC version. >> > >> > I don't know if it's acceptable to backport the datapath (kernel module) >> > support before the userspace support is accepted. If not, you'll have to >> > wait on the userspace. >> > Perhaps Pravin can answer. >> >> IMO the general method of: >> 1) Add support upstream >> 2) Add userspace support >> 3) Add backport >> >> ...works nicely because we get feedback for all interested parties for >> the APIs in (1), (2) can add tests and be easily tested against a >> version that works (upstream kernel) and a version that doesn't >> (version in tree) to ensure both cases are handled in a reasonable >> way, then (3) allows people on older kernels to gain access to the >> newer features. >> >> That said, if other people are blocking on (3) then I think that piece >> should be expedited. Let's say (2) and (3) were swapped, it just means >> we need to be a bit more careful when reviewing/testing to check that >> the newer userspace still handles old kernels (that lack support) >> fine. >> >> The nice thing about getting the backport earlier is, the closer to >> upstream we are, the sooner we may find issues that affect the latest >> code. > > If so, I think the below patch is the best solution to current > situation, it will be automatically overwritten once userspace part and > 802.1ad backport are ready to merge. > It is much easier to backport and review patches when it is done in same sequence as applied on upstream kernel. It is not just about this patch. by changing backport order the implementation and review of both L3 patch series and 802.1AD patch series becomes harder than necessary. > So I propose we can merge this patch serial first, then merge Eric > Garver's rtnetlink patch serial. By then, ovs can completely support L3 > tunnel ports in both compat mode and kernel datapath mode, it will also > work well on Linux 4.7 and later versions. > In general out of tree kernel module does not deviate much from in-tree ovs module. So there is no need for following patch and we should backport 802.1AD datapath patches first even before the userspace support. > From 127b1c7af10220dacc5b0c6a7c8b4e19d9d4ecac Mon Sep 17 00:00:00 2001 > From: Yi Yang <yi.y.yang@intel.com> > Date: Wed, 28 Dec 2016 10:27:11 +0800 > Subject: [PATCH v2 14/17] datapath: Fix skb->protocol for vlan frames > > Do not always set skb->protocol to be the ethertype of the L3 header. > For a packet with non-accelerated VLAN tags skb->protocol needs to be the ethertype of the outermost non-accelerated VLAN ethertype. > > Any VLAN offloading is undone on the OVS netlink interface, and any VLAN tags added by userspace are non-accelerated, as are double tagged VLAN packets. > > Fixes: 018c1dda5f ("openvswitch: 802.1AD Flow handling, actions, vlan parsing, netlink attributes") > Fixes: 5108bbaddc ("openvswitch: add processing of L3 packets") > Signed-off-by: Jarno Rajahalme <jarno@ovn.org> > Signed-off-by: Yi Yang <yi.y.yang@intel.com> > --- > datapath/datapath.c | 1 - > datapath/flow.c | 42 +++++++++++++++++------------------------- > 2 files changed, 17 insertions(+), 26 deletions(-) > > diff --git a/datapath/datapath.c b/datapath/datapath.c > index 1deb62d..dd19a0e 100644 > --- a/datapath/datapath.c > +++ b/datapath/datapath.c > @@ -617,7 +617,6 @@ static int ovs_packet_cmd_execute(struct sk_buff *skb, struct genl_info *info) > rcu_assign_pointer(flow->sf_acts, acts); > packet->priority = flow->key.phy.priority; > packet->mark = flow->key.phy.skb_mark; > - packet->protocol = flow->key.eth.type; > > rcu_read_lock(); > dp = get_dp_rcu(net, ovs_header->dp_ifindex); > diff --git a/datapath/flow.c b/datapath/flow.c > index 240bd00..cf35272 100644 > --- a/datapath/flow.c > +++ b/datapath/flow.c > @@ -334,6 +334,7 @@ static int parse_vlan(struct sk_buff *skb, struct sw_flow_key *key) > > qp = (struct qtag_prefix *) skb->data; > key->eth.tci = qp->tci | htons(VLAN_TAG_PRESENT); > + key->eth.type = qp->eth_type; > __skb_pull(skb, sizeof(struct qtag_prefix)); > > return 0; > @@ -493,6 +494,7 @@ static int key_extract(struct sk_buff *skb, struct sw_flow_key *key) > return -EINVAL; > > skb_reset_network_header(skb); > + key->eth.type = skb->protocol; > } else { > eth = eth_hdr(skb); > ether_addr_copy(key->eth.src, eth->h_source); > @@ -504,11 +506,14 @@ static int key_extract(struct sk_buff *skb, struct sw_flow_key *key) > */ > > key->eth.tci = 0; > - if (skb_vlan_tag_present(skb)) > + if (skb_vlan_tag_present(skb)) { > key->eth.tci = htons(skb->vlan_tci); > + key->eth.type = skb->vlan_proto; > + } > else if (eth->h_proto == htons(ETH_P_8021Q)) > if (unlikely(parse_vlan(skb, key))) > return -ENOMEM; > + skb->protocol = key->eth.type; > > key->eth.type = parse_ethertype(skb); > if (unlikely(key->eth.type == htons(0))) > @@ -519,7 +524,8 @@ static int key_extract(struct sk_buff *skb, struct sw_flow_key *key) > } > > skb_reset_mac_len(skb); > - key->eth.type = skb->protocol; > + if (!eth_type_is_vlan(skb->protocol)) > + skb->protocol = key->eth.type; > > /* Network layer. */ > if (key->eth.type == htons(ETH_P_IP)) { > @@ -786,29 +792,15 @@ int ovs_flow_key_extract_userspace(struct net *net, const struct nlattr *attr, > if (err) > return err; > > - if (ovs_key_mac_proto(key) == MAC_PROTO_NONE) { > - /* key_extract assumes that skb->protocol is set-up for > - * layer 3 packets which is the case for other callers, > - * in particular packets recieved from the network stack. > - * Here the correct value can be set from the metadata > - * extracted above. > - */ > - skb->protocol = key->eth.type; > - } else { > - struct ethhdr *eth; > - > - skb_reset_mac_header(skb); > - eth = eth_hdr(skb); > - > - /* Normally, setting the skb 'protocol' field would be > - * handled by a call to eth_type_trans(), but it assumes > - * there's a sending device, which we may not have. > - */ > - if (eth_proto_is_802_3(eth->h_proto)) > - skb->protocol = eth->h_proto; > - else > - skb->protocol = htons(ETH_P_802_2); > - } > + /* key_extract assumes that skb->protocol is set-up for > + * layer 3 packets which is the case for other callers, > + * in particular packets recieved from the network stack. > + * Here the correct value can be set from the metadata > + * extracted above. For layer 2 packets we initialize > + * skb->protocol to zero and set it in key_extract() while > + * parsing the L2 headers. > + */ > + skb->protocol = key->eth.type; > > return key_extract(skb, key); > } > -- > 2.1.0 >
On 19 January 2017 at 12:17, Pravin Shelar <pshelar@ovn.org> wrote: > On Wed, Jan 18, 2017 at 4:53 PM, Yang, Yi <yi.y.yang@intel.com> wrote: >> On Wed, Jan 18, 2017 at 01:29:14PM -0800, Joe Stringer wrote: >>> On 18 January 2017 at 11:54, Eric Garver <e@erig.me> wrote: >>> > On Tue, Jan 17, 2017 at 12:37:19AM +0000, Yang, Yi Y wrote: >>> >> What userspace do "802.1ad patches" depend on? Per Pravin's statement, we just backport 802.1ad patches to ovs, then the below patch can be applied to ovs. >>> > >>> > Userspace does not yet have support for 802.1ad. I'm still working on >>> > it. You can check the list archives for a recent RFC version. >>> > >>> > I don't know if it's acceptable to backport the datapath (kernel module) >>> > support before the userspace support is accepted. If not, you'll have to >>> > wait on the userspace. >>> > Perhaps Pravin can answer. >>> >>> IMO the general method of: >>> 1) Add support upstream >>> 2) Add userspace support >>> 3) Add backport >>> >>> ...works nicely because we get feedback for all interested parties for >>> the APIs in (1), (2) can add tests and be easily tested against a >>> version that works (upstream kernel) and a version that doesn't >>> (version in tree) to ensure both cases are handled in a reasonable >>> way, then (3) allows people on older kernels to gain access to the >>> newer features. >>> >>> That said, if other people are blocking on (3) then I think that piece >>> should be expedited. Let's say (2) and (3) were swapped, it just means >>> we need to be a bit more careful when reviewing/testing to check that >>> the newer userspace still handles old kernels (that lack support) >>> fine. >>> >>> The nice thing about getting the backport earlier is, the closer to >>> upstream we are, the sooner we may find issues that affect the latest >>> code. >> >> If so, I think the below patch is the best solution to current >> situation, it will be automatically overwritten once userspace part and >> 802.1ad backport are ready to merge. >> > It is much easier to backport and review patches when it is done in > same sequence as applied on upstream kernel. It is not just about this > patch. by changing backport order the implementation and review of > both L3 patch series and 802.1AD patch series becomes harder than > necessary. Sorry, yes, my earlier explanation was regarding a particular feature and ordering of userspace implementation vs. backport patches. With regards to the overall kernel backport, I agree the upstream patches should be backported in sequence according to upstream. Then we know we haven't missed anything. Also, dependencies from one patch to the next are less likely to cause trouble while rebasing, so it makes the backporter's life easier.
On Thu, Jan 19, 2017 at 03:12:20PM -0800, Joe Stringer wrote: > On 19 January 2017 at 12:17, Pravin Shelar <pshelar@ovn.org> wrote: > > On Wed, Jan 18, 2017 at 4:53 PM, Yang, Yi <yi.y.yang@intel.com> wrote: > >> On Wed, Jan 18, 2017 at 01:29:14PM -0800, Joe Stringer wrote: > >>> On 18 January 2017 at 11:54, Eric Garver <e@erig.me> wrote: > >>> > On Tue, Jan 17, 2017 at 12:37:19AM +0000, Yang, Yi Y wrote: > >>> >> What userspace do "802.1ad patches" depend on? Per Pravin's statement, we just backport 802.1ad patches to ovs, then the below patch can be applied to ovs. > >>> > > >>> > Userspace does not yet have support for 802.1ad. I'm still working on > >>> > it. You can check the list archives for a recent RFC version. > >>> > > >>> > I don't know if it's acceptable to backport the datapath (kernel module) > >>> > support before the userspace support is accepted. If not, you'll have to > >>> > wait on the userspace. > >>> > Perhaps Pravin can answer. > >>> > >>> IMO the general method of: > >>> 1) Add support upstream > >>> 2) Add userspace support > >>> 3) Add backport > >>> > >>> ...works nicely because we get feedback for all interested parties for > >>> the APIs in (1), (2) can add tests and be easily tested against a > >>> version that works (upstream kernel) and a version that doesn't > >>> (version in tree) to ensure both cases are handled in a reasonable > >>> way, then (3) allows people on older kernels to gain access to the > >>> newer features. > >>> > >>> That said, if other people are blocking on (3) then I think that piece > >>> should be expedited. Let's say (2) and (3) were swapped, it just means > >>> we need to be a bit more careful when reviewing/testing to check that > >>> the newer userspace still handles old kernels (that lack support) > >>> fine. > >>> > >>> The nice thing about getting the backport earlier is, the closer to > >>> upstream we are, the sooner we may find issues that affect the latest > >>> code. > >> > >> If so, I think the below patch is the best solution to current > >> situation, it will be automatically overwritten once userspace part and > >> 802.1ad backport are ready to merge. > >> > > It is much easier to backport and review patches when it is done in > > same sequence as applied on upstream kernel. It is not just about this > > patch. by changing backport order the implementation and review of > > both L3 patch series and 802.1AD patch series becomes harder than > > necessary. > > Sorry, yes, my earlier explanation was regarding a particular feature > and ordering of userspace implementation vs. backport patches. > > With regards to the overall kernel backport, I agree the upstream > patches should be backported in sequence according to upstream. Then > we know we haven't missed anything. Also, dependencies from one patch > to the next are less likely to cause trouble while rebasing, so it > makes the backporter's life easier. Sounds good to me. Yi is welcome to try the 802.1ad datapath backport. Otherwise I'll get to when I can. Thanks everyone.
Thank you all, I'll try to backport 802.1ad patches. -----Original Message----- From: ovs-dev-bounces@openvswitch.org [mailto:ovs-dev-bounces@openvswitch.org] On Behalf Of Eric Garver Sent: Friday, January 20, 2017 7:53 AM To: Joe Stringer <joe@ovn.org> Cc: ovs dev <dev@openvswitch.org>; Jan Scheurich <jan.scheurich@web.de> Subject: Re: [ovs-dev] [PATCH v2 14/17] datapath: Fix skb->protocol for vlan frames On Thu, Jan 19, 2017 at 03:12:20PM -0800, Joe Stringer wrote: > On 19 January 2017 at 12:17, Pravin Shelar <pshelar@ovn.org> wrote: > > On Wed, Jan 18, 2017 at 4:53 PM, Yang, Yi <yi.y.yang@intel.com> wrote: > >> On Wed, Jan 18, 2017 at 01:29:14PM -0800, Joe Stringer wrote: > >>> On 18 January 2017 at 11:54, Eric Garver <e@erig.me> wrote: > >>> > On Tue, Jan 17, 2017 at 12:37:19AM +0000, Yang, Yi Y wrote: > >>> >> What userspace do "802.1ad patches" depend on? Per Pravin's statement, we just backport 802.1ad patches to ovs, then the below patch can be applied to ovs. > >>> > > >>> > Userspace does not yet have support for 802.1ad. I'm still > >>> > working on it. You can check the list archives for a recent RFC version. > >>> > > >>> > I don't know if it's acceptable to backport the datapath (kernel > >>> > module) support before the userspace support is accepted. If > >>> > not, you'll have to wait on the userspace. > >>> > Perhaps Pravin can answer. > >>> > >>> IMO the general method of: > >>> 1) Add support upstream > >>> 2) Add userspace support > >>> 3) Add backport > >>> > >>> ...works nicely because we get feedback for all interested parties > >>> for the APIs in (1), (2) can add tests and be easily tested > >>> against a version that works (upstream kernel) and a version that > >>> doesn't (version in tree) to ensure both cases are handled in a > >>> reasonable way, then (3) allows people on older kernels to gain > >>> access to the newer features. > >>> > >>> That said, if other people are blocking on (3) then I think that > >>> piece should be expedited. Let's say (2) and (3) were swapped, it > >>> just means we need to be a bit more careful when reviewing/testing > >>> to check that the newer userspace still handles old kernels (that > >>> lack support) fine. > >>> > >>> The nice thing about getting the backport earlier is, the closer > >>> to upstream we are, the sooner we may find issues that affect the > >>> latest code. > >> > >> If so, I think the below patch is the best solution to current > >> situation, it will be automatically overwritten once userspace part > >> and 802.1ad backport are ready to merge. > >> > > It is much easier to backport and review patches when it is done in > > same sequence as applied on upstream kernel. It is not just about > > this patch. by changing backport order the implementation and review > > of both L3 patch series and 802.1AD patch series becomes harder than > > necessary. > > Sorry, yes, my earlier explanation was regarding a particular feature > and ordering of userspace implementation vs. backport patches. > > With regards to the overall kernel backport, I agree the upstream > patches should be backported in sequence according to upstream. Then > we know we haven't missed anything. Also, dependencies from one patch > to the next are less likely to cause trouble while rebasing, so it > makes the backporter's life easier. Sounds good to me. Yi is welcome to try the 802.1ad datapath backport. Otherwise I'll get to when I can. Thanks everyone.
diff --git a/datapath/datapath.c b/datapath/datapath.c index 1deb62d..dd19a0e 100644 --- a/datapath/datapath.c +++ b/datapath/datapath.c @@ -617,7 +617,6 @@ static int ovs_packet_cmd_execute(struct sk_buff *skb, struct genl_info *info) rcu_assign_pointer(flow->sf_acts, acts); packet->priority = flow->key.phy.priority; packet->mark = flow->key.phy.skb_mark; - packet->protocol = flow->key.eth.type; rcu_read_lock(); dp = get_dp_rcu(net, ovs_header->dp_ifindex); diff --git a/datapath/flow.c b/datapath/flow.c index 240bd00..cf35272 100644 --- a/datapath/flow.c +++ b/datapath/flow.c @@ -334,6 +334,7 @@ static int parse_vlan(struct sk_buff *skb, struct sw_flow_key *key) qp = (struct qtag_prefix *) skb->data; key->eth.tci = qp->tci | htons(VLAN_TAG_PRESENT); + key->eth.type = qp->eth_type; __skb_pull(skb, sizeof(struct qtag_prefix)); return 0; @@ -493,6 +494,7 @@ static int key_extract(struct sk_buff *skb, struct sw_flow_key *key) return -EINVAL; skb_reset_network_header(skb); + key->eth.type = skb->protocol; } else { eth = eth_hdr(skb); ether_addr_copy(key->eth.src, eth->h_source); @@ -504,11 +506,14 @@ static int key_extract(struct sk_buff *skb, struct sw_flow_key *key) */ key->eth.tci = 0; - if (skb_vlan_tag_present(skb)) + if (skb_vlan_tag_present(skb)) { key->eth.tci = htons(skb->vlan_tci); + key->eth.type = skb->vlan_proto; + } else if (eth->h_proto == htons(ETH_P_8021Q)) if (unlikely(parse_vlan(skb, key))) return -ENOMEM; + skb->protocol = key->eth.type; key->eth.type = parse_ethertype(skb); if (unlikely(key->eth.type == htons(0))) @@ -519,7 +524,8 @@ static int key_extract(struct sk_buff *skb, struct sw_flow_key *key) } skb_reset_mac_len(skb); - key->eth.type = skb->protocol; + if (!eth_type_is_vlan(skb->protocol)) + skb->protocol = key->eth.type; /* Network layer. */ if (key->eth.type == htons(ETH_P_IP)) { @@ -786,29 +792,15 @@ int ovs_flow_key_extract_userspace(struct net *net, const struct nlattr *attr, if (err) return err; - if (ovs_key_mac_proto(key) == MAC_PROTO_NONE) { - /* key_extract assumes that skb->protocol is set-up for - * layer 3 packets which is the case for other callers, - * in particular packets recieved from the network stack. - * Here the correct value can be set from the metadata - * extracted above. - */ - skb->protocol = key->eth.type; - } else { - struct ethhdr *eth; - - skb_reset_mac_header(skb); - eth = eth_hdr(skb); - - /* Normally, setting the skb 'protocol' field would be - * handled by a call to eth_type_trans(), but it assumes - * there's a sending device, which we may not have. - */ - if (eth_proto_is_802_3(eth->h_proto)) - skb->protocol = eth->h_proto; - else - skb->protocol = htons(ETH_P_802_2); - } + /* key_extract assumes that skb->protocol is set-up for + * layer 3 packets which is the case for other callers, + * in particular packets recieved from the network stack. + * Here the correct value can be set from the metadata + * extracted above. For layer 2 packets we initialize + * skb->protocol to zero and set it in key_extract() while + * parsing the L2 headers. + */ + skb->protocol = key->eth.type; return key_extract(skb, key); }