| Message ID | cover.1588869591.git.martin.varghese@nokia.com |
|---|---|
| Headers | show |
| Series | Encap & Decap actions for MPLS packet type | expand |
Hi Martin, I planned to do a full review of this patchset, which I usually start with testing. However, during my testing, I found that this patch is not backward compatible. If I try to run this on a kernel without the kmod change, the existing push_mpls action fails. On top of this, I think that if the kernel does not support the OVS_ACTION_ATTR_ADD_MPLS action, it should fallback using the SLOW_ACTION, so the packet gets processed in userspace. In addition, I got a compiler warning on the following: > @@ -180,6 +222,11 @@ parse_ed_prop_type(uint16_t prop_class, > } else { > return false; > } > + case OFPPPC_MPLS: > + if (!strcmp(str, "ether_type")) { > + *type = OFPPPT_PROP_MPLS_ETHERTYPE; > + return true; > + } Either do a "return false" or an explicit fall trough to stop the compiler from complaining. > default: > return false; > } So please fix the backward/forward compatibility stuff in the next version, and I'll do the full review then as I suspect there will be a bunch of related changes. Also, make sure the patches apply and compile individually. See the comment from Ilya earlier. Cheers, Eelco On 8 May 2020, at 6:30, Martin Varghese wrote: > From: Martin Varghese <martin.varghese@nokia.com> > > The existing PUSH MPLS & POP MPLS actions inserts & removes MPLS > header > between ethernet header and the IP header. Though this behaviour is > fine > for L3 VPN where an IP packet is encapsulated inside a MPLS tunnel, it > does not suffice the L2 VPN requirements. In L2 VPN the ethernet > packets > must be encapsulated inside MPLS tunnel > > In this change the encap & decap actions are extended to support MPLS > packet type. The encap & decap adds and removes MPLS header at the > start > of packet as depicted below. > > Encapsulation: > > Actions - encap(mpls(ether_type=0x8847)),encap(ethernet) > > Incoming packet -> | ETH | IP | Payload | > > 1 Actions - encap(mpls(ether_type=0x8847)) [Datapath action - > ADD_MPLS:0x8847] > > Outgoing packet -> | MPLS | ETH | Payload| > > 2 Actions - encap(ethernet) [ Datapath action - push_eth ] > > Outgoing packet -> | ETH | MPLS | ETH | Payload| > > Decapsulation: > > Incoming packet -> | ETH | MPLS | ETH | IP | Payload | > > Actions - decap(),decap(packet_type(ns=0,type=0) > > 1 Actions - decap() [Datapath action - pop_eth) > > Outgoing packet -> | MPLS | ETH | IP | Payload| > > 2 Actions - decap(packet_type(ns=0,type=0) [Datapath action - > POP_MPLS:0x6558] > > Outgoing packet -> | ETH | IP | Payload > > Martin Varghese (2): > Datapath: New MPLS actions for layer 2 tunnelling > Encap & Decap actions for MPLS packet type > > NEWS | 1 + > datapath/actions.c | 42 ++++++++--- > datapath/flow_netlink.c | 33 +++++++++ > datapath/linux/compat/include/linux/openvswitch.h | 35 ++++++++- > include/openvswitch/ofp-ed-props.h | 18 +++++ > lib/dpif-netdev.c | 1 + > lib/dpif.c | 1 + > lib/odp-execute.c | 12 +++ > lib/odp-util.c | 38 ++++++++-- > lib/ofp-actions.c | 5 ++ > lib/ofp-ed-props.c | 89 > +++++++++++++++++++++++ > lib/ovs-actions.xml | 32 ++++++-- > lib/packets.c | 36 +++++++++ > lib/packets.h | 2 + > ofproto/ofproto-dpif-ipfix.c | 1 + > ofproto/ofproto-dpif-sflow.c | 1 + > ofproto/ofproto-dpif-xlate.c | 54 ++++++++++++++ > tests/system-traffic.at | 34 +++++++++ > 18 files changed, 410 insertions(+), 25 deletions(-) > > -- > 1.8.3.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Hi Eelco, Thanks for taking time to review the patch. I had noted the comments from Ilya, but could not finish doing the next version because of other commitments. I hope I will be able to send out the next version soon. Regards, Martin
Hi Eelco, As you suggested in a different email I have tried to use the new ADD_MPLS datapath action for the "push_mpls" userspace action. But these tests fail. 487: mpls-xlate.at:3 MPLS xlate action 6230 488: mpls-xlate.at:147 MPLS xlate action - patch-port 6231 489: mpls-xlate.at:191 MPLS xlate action - group bucket 6232 1143: ofproto-dpif.at:7118 ofproto-dpif - sFlow packet sampling - MPLS 6233 1165: ofproto-dpif.at:8204 ofproto-dpif - MPLS actions that result in a userspace action 6234 1166: ofproto-dpif.at:8240 ofproto-dpif - MPLS actions that result in a drop 6235 1177: ofproto-dpif.at:8534 ofproto-dpif megaflow - mpls I understand these tests fail as they don't see "PUSH_MPLS" datapath action in datapath flows. Is there any framework to selectively use ADD_MPLS or PUSH_MPLS in test scripts based on what is supported in datapath ? Or, Should we continue to use PUSH_MPLS datapath action for userspace push_mpls action ? Regards, Martin
On 11 Mar 2021, at 5:57, Varghese, Martin (Nokia - IN/Bangalore) wrote: > Hi Eelco, > > As you suggested in a different email I have tried to use the new > ADD_MPLS datapath action for the "push_mpls" userspace action. > But these tests fail. > 487: mpls-xlate.at:3 MPLS xlate action > 6230 488: mpls-xlate.at:147 MPLS xlate action - patch-port > 6231 489: mpls-xlate.at:191 MPLS xlate action - group bucket > 6232 1143: ofproto-dpif.at:7118 ofproto-dpif - sFlow packet sampling - > MPLS > 6233 1165: ofproto-dpif.at:8204 ofproto-dpif - MPLS actions that > result in a userspace action > 6234 1166: ofproto-dpif.at:8240 ofproto-dpif - MPLS actions that > result in a drop > 6235 1177: ofproto-dpif.at:8534 ofproto-dpif megaflow - mpls > > I understand these tests fail as they don't see "PUSH_MPLS" datapath > action in datapath flows. > > Is there any framework to selectively use ADD_MPLS or PUSH_MPLS in > test scripts based on what is supported in datapath ? With AT_SKIP_IF you can only skip the entire test, not do a conditional somewhere in the test. So you could either write two test cases and execute them based on a specific condition, or you could pass the result through sed and replace the new keyword with the old one (if the parameters apply the same). > Or, Should we continue to use PUSH_MPLS datapath action for userspace > push_mpls action ? That I leave up to you, as long as it’s consistent in all cases.
From: Martin Varghese <martin.varghese@nokia.com> The existing PUSH MPLS & POP MPLS actions inserts & removes MPLS header between ethernet header and the IP header. Though this behaviour is fine for L3 VPN where an IP packet is encapsulated inside a MPLS tunnel, it does not suffice the L2 VPN requirements. In L2 VPN the ethernet packets must be encapsulated inside MPLS tunnel In this change the encap & decap actions are extended to support MPLS packet type. The encap & decap adds and removes MPLS header at the start of packet as depicted below. Encapsulation: Actions - encap(mpls(ether_type=0x8847)),encap(ethernet) Incoming packet -> | ETH | IP | Payload | 1 Actions - encap(mpls(ether_type=0x8847)) [Datapath action - ADD_MPLS:0x8847] Outgoing packet -> | MPLS | ETH | Payload| 2 Actions - encap(ethernet) [ Datapath action - push_eth ] Outgoing packet -> | ETH | MPLS | ETH | Payload| Decapsulation: Incoming packet -> | ETH | MPLS | ETH | IP | Payload | Actions - decap(),decap(packet_type(ns=0,type=0) 1 Actions - decap() [Datapath action - pop_eth) Outgoing packet -> | MPLS | ETH | IP | Payload| 2 Actions - decap(packet_type(ns=0,type=0) [Datapath action - POP_MPLS:0x6558] Outgoing packet -> | ETH | IP | Payload Martin Varghese (2): Datapath: New MPLS actions for layer 2 tunnelling Encap & Decap actions for MPLS packet type NEWS | 1 + datapath/actions.c | 42 ++++++++--- datapath/flow_netlink.c | 33 +++++++++ datapath/linux/compat/include/linux/openvswitch.h | 35 ++++++++- include/openvswitch/ofp-ed-props.h | 18 +++++ lib/dpif-netdev.c | 1 + lib/dpif.c | 1 + lib/odp-execute.c | 12 +++ lib/odp-util.c | 38 ++++++++-- lib/ofp-actions.c | 5 ++ lib/ofp-ed-props.c | 89 +++++++++++++++++++++++ lib/ovs-actions.xml | 32 ++++++-- lib/packets.c | 36 +++++++++ lib/packets.h | 2 + ofproto/ofproto-dpif-ipfix.c | 1 + ofproto/ofproto-dpif-sflow.c | 1 + ofproto/ofproto-dpif-xlate.c | 54 ++++++++++++++ tests/system-traffic.at | 34 +++++++++ 18 files changed, 410 insertions(+), 25 deletions(-)