| Message ID | AM2PR07MB1042E004F156EE3C9ADA4D168AD30@AM2PR07MB1042.eurprd07.prod.outlook.com |
|---|---|
| State | Changes Requested |
| Headers | show |
On Fri, Jun 30, 2017 at 03:29:32PM +0000, Zoltán Balogh wrote: > From: Jan Scheurich <jan.scheurich@ericsson.com> > > This commit implements a skeleton for the translation of generic encap > and decap actions in ofproto-dpif and adds support to encap and decap an > Ethernet header. > > In general translation of encap commits pending actions and then rewrites > struct flow in accordance with the new packet type and header. In the > case of encap(ethernet) it suffices to change the packet type from > (1, Ethertype) to (0,0) and set the dl_type accordingly. A new > pending_encap flag in xlate ctx is set to mark that an corresponding > datapath encap action must be triggered at the next commit. In the > case of encap(ethernet) ofproto generetas a push_eth action. > > The general case for translation of decap() is to emit a datapath action > to decap the current outermost header and then recirculate the packet > to reparse the inner headers. In the special case of an Ethernet packet, > decap() just changes the packet type from (0,0) to (1, dl_type) without > a need to recirculate. The emission of the pop_eth action for the > datapath is postponed to the next commit. > > Hence encap(ethernet) and decap() on an Ethernet packet are OF octions > that only incur a cost in the dataplane when a modifed packet is > actually committed, e.g. because it is sent out. They can freely be > used for normalizing the packet type in the OF pipeline without > degrading performance. > > Signed-off-by: Jan Scheurich <jan.scheurich@ericsson.com> > Signed-off-by: Yi Yang <yi.y.yang@intel.com> Thanks for posting this. It looks like a lot of the purpose of this commit is to fill in the blanks with the first commit in the series, fixing some of the issues and implementing bits that were missing. I think that it would be easier to review if the two commits were squashed together. I am not sure that I understand the way of doing deferred encap actions. It looks like, for example, one can chain a number of encap actions, but that the result is just encapsulating once. Am I reading that right? Thanks, Ben.
Ben, encap action is just adding an empty header, set_field action will set the fields in the encapped header, so encap and set actions can be combined if we defer encap action, this is an optimization. -----Original Message----- From: Ben Pfaff [mailto:blp@ovn.org] Sent: Wednesday, July 12, 2017 8:28 AM To: Zoltán Balogh <zoltan.balogh@ericsson.com> Cc: 'dev@openvswitch.org' <dev@openvswitch.org>; Jan Scheurich <jan.scheurich@ericsson.com>; Georg Schmuecking <Georg.Schmuecking@ericsson.com>; Yang, Yi Y <yi.y.yang@intel.com>; Jiri Benc (jbenc@redhat.com) <jbenc@redhat.com> Subject: Re: [PATCH 2/4] Translation of generic encap and decap actions On Fri, Jun 30, 2017 at 03:29:32PM +0000, Zoltán Balogh wrote: > From: Jan Scheurich <jan.scheurich@ericsson.com> > > This commit implements a skeleton for the translation of generic encap > and decap actions in ofproto-dpif and adds support to encap and decap > an Ethernet header. > > In general translation of encap commits pending actions and then > rewrites struct flow in accordance with the new packet type and > header. In the case of encap(ethernet) it suffices to change the > packet type from (1, Ethertype) to (0,0) and set the dl_type > accordingly. A new pending_encap flag in xlate ctx is set to mark that > an corresponding datapath encap action must be triggered at the next > commit. In the case of encap(ethernet) ofproto generetas a push_eth action. > > The general case for translation of decap() is to emit a datapath > action to decap the current outermost header and then recirculate the > packet to reparse the inner headers. In the special case of an > Ethernet packet, > decap() just changes the packet type from (0,0) to (1, dl_type) > without a need to recirculate. The emission of the pop_eth action for > the datapath is postponed to the next commit. > > Hence encap(ethernet) and decap() on an Ethernet packet are OF octions > that only incur a cost in the dataplane when a modifed packet is > actually committed, e.g. because it is sent out. They can freely be > used for normalizing the packet type in the OF pipeline without > degrading performance. > > Signed-off-by: Jan Scheurich <jan.scheurich@ericsson.com> > Signed-off-by: Yi Yang <yi.y.yang@intel.com> Thanks for posting this. It looks like a lot of the purpose of this commit is to fill in the blanks with the first commit in the series, fixing some of the issues and implementing bits that were missing. I think that it would be easier to review if the two commits were squashed together. I am not sure that I understand the way of doing deferred encap actions. It looks like, for example, one can chain a number of encap actions, but that the result is just encapsulating once. Am I reading that right? Thanks, Ben.
OK. Are you willing to squash the first two patches for the next version? It looks to me like they form a single logical change. On Wed, Jul 12, 2017 at 12:41:10AM +0000, Yang, Yi Y wrote: > Ben, encap action is just adding an empty header, set_field action will set the fields in the encapped header, so encap and set actions can be combined if we defer encap action, this is an optimization. > > -----Original Message----- > From: Ben Pfaff [mailto:blp@ovn.org] > Sent: Wednesday, July 12, 2017 8:28 AM > To: Zoltán Balogh <zoltan.balogh@ericsson.com> > Cc: 'dev@openvswitch.org' <dev@openvswitch.org>; Jan Scheurich <jan.scheurich@ericsson.com>; Georg Schmuecking <Georg.Schmuecking@ericsson.com>; Yang, Yi Y <yi.y.yang@intel.com>; Jiri Benc (jbenc@redhat.com) <jbenc@redhat.com> > Subject: Re: [PATCH 2/4] Translation of generic encap and decap actions > > On Fri, Jun 30, 2017 at 03:29:32PM +0000, Zoltán Balogh wrote: > > From: Jan Scheurich <jan.scheurich@ericsson.com> > > > > This commit implements a skeleton for the translation of generic encap > > and decap actions in ofproto-dpif and adds support to encap and decap > > an Ethernet header. > > > > In general translation of encap commits pending actions and then > > rewrites struct flow in accordance with the new packet type and > > header. In the case of encap(ethernet) it suffices to change the > > packet type from (1, Ethertype) to (0,0) and set the dl_type > > accordingly. A new pending_encap flag in xlate ctx is set to mark that > > an corresponding datapath encap action must be triggered at the next > > commit. In the case of encap(ethernet) ofproto generetas a push_eth action. > > > > The general case for translation of decap() is to emit a datapath > > action to decap the current outermost header and then recirculate the > > packet to reparse the inner headers. In the special case of an > > Ethernet packet, > > decap() just changes the packet type from (0,0) to (1, dl_type) > > without a need to recirculate. The emission of the pop_eth action for > > the datapath is postponed to the next commit. > > > > Hence encap(ethernet) and decap() on an Ethernet packet are OF octions > > that only incur a cost in the dataplane when a modifed packet is > > actually committed, e.g. because it is sent out. They can freely be > > used for normalizing the packet type in the OF pipeline without > > degrading performance. > > > > Signed-off-by: Jan Scheurich <jan.scheurich@ericsson.com> > > Signed-off-by: Yi Yang <yi.y.yang@intel.com> > > Thanks for posting this. > > It looks like a lot of the purpose of this commit is to fill in the blanks with the first commit in the series, fixing some of the issues and implementing bits that were missing. I think that it would be easier to review if the two commits were squashed together. > > I am not sure that I understand the way of doing deferred encap actions. It looks like, for example, one can chain a number of encap actions, but that the result is just encapsulating once. Am I reading that right? > > Thanks, > > Ben.
Zoltan will do that. -----Original Message----- From: Ben Pfaff [mailto:blp@ovn.org] Sent: Wednesday, July 12, 2017 11:59 AM To: Yang, Yi Y <yi.y.yang@intel.com> Cc: Zoltán Balogh <zoltan.balogh@ericsson.com>; 'dev@openvswitch.org' <dev@openvswitch.org>; Jan Scheurich <jan.scheurich@ericsson.com>; Georg Schmuecking <Georg.Schmuecking@ericsson.com>; Jiri Benc (jbenc@redhat.com) <jbenc@redhat.com> Subject: Re: [PATCH 2/4] Translation of generic encap and decap actions OK. Are you willing to squash the first two patches for the next version? It looks to me like they form a single logical change. On Wed, Jul 12, 2017 at 12:41:10AM +0000, Yang, Yi Y wrote: > Ben, encap action is just adding an empty header, set_field action will set the fields in the encapped header, so encap and set actions can be combined if we defer encap action, this is an optimization. > > -----Original Message----- > From: Ben Pfaff [mailto:blp@ovn.org] > Sent: Wednesday, July 12, 2017 8:28 AM > To: Zoltán Balogh <zoltan.balogh@ericsson.com> > Cc: 'dev@openvswitch.org' <dev@openvswitch.org>; Jan Scheurich > <jan.scheurich@ericsson.com>; Georg Schmuecking > <Georg.Schmuecking@ericsson.com>; Yang, Yi Y <yi.y.yang@intel.com>; > Jiri Benc (jbenc@redhat.com) <jbenc@redhat.com> > Subject: Re: [PATCH 2/4] Translation of generic encap and decap > actions > > On Fri, Jun 30, 2017 at 03:29:32PM +0000, Zoltán Balogh wrote: > > From: Jan Scheurich <jan.scheurich@ericsson.com> > > > > This commit implements a skeleton for the translation of generic > > encap and decap actions in ofproto-dpif and adds support to encap > > and decap an Ethernet header. > > > > In general translation of encap commits pending actions and then > > rewrites struct flow in accordance with the new packet type and > > header. In the case of encap(ethernet) it suffices to change the > > packet type from (1, Ethertype) to (0,0) and set the dl_type > > accordingly. A new pending_encap flag in xlate ctx is set to mark > > that an corresponding datapath encap action must be triggered at the > > next commit. In the case of encap(ethernet) ofproto generetas a push_eth action. > > > > The general case for translation of decap() is to emit a datapath > > action to decap the current outermost header and then recirculate > > the packet to reparse the inner headers. In the special case of an > > Ethernet packet, > > decap() just changes the packet type from (0,0) to (1, dl_type) > > without a need to recirculate. The emission of the pop_eth action > > for the datapath is postponed to the next commit. > > > > Hence encap(ethernet) and decap() on an Ethernet packet are OF > > octions that only incur a cost in the dataplane when a modifed > > packet is actually committed, e.g. because it is sent out. They can > > freely be used for normalizing the packet type in the OF pipeline > > without degrading performance. > > > > Signed-off-by: Jan Scheurich <jan.scheurich@ericsson.com> > > Signed-off-by: Yi Yang <yi.y.yang@intel.com> > > Thanks for posting this. > > It looks like a lot of the purpose of this commit is to fill in the blanks with the first commit in the series, fixing some of the issues and implementing bits that were missing. I think that it would be easier to review if the two commits were squashed together. > > I am not sure that I understand the way of doing deferred encap actions. It looks like, for example, one can chain a number of encap actions, but that the result is just encapsulating once. Am I reading that right? > > Thanks, > > Ben.
diff --git a/lib/odp-util.c b/lib/odp-util.c index f4c0b66..d9fee4f 100644 --- a/lib/odp-util.c +++ b/lib/odp-util.c @@ -5862,13 +5862,17 @@ put_ethernet_key(const struct ovs_key_ethernet *eth, struct flow *flow) } static void -commit_set_ether_addr_action(const struct flow *flow, struct flow *base_flow, - struct ofpbuf *odp_actions, - struct flow_wildcards *wc, - bool use_masked) +commit_set_ether_action(const struct flow *flow, struct flow *base_flow, + struct ofpbuf *odp_actions, + struct flow_wildcards *wc, + bool use_masked) { struct ovs_key_ethernet key, base, mask; + if (flow->packet_type != htonl(PT_ETH)) { + return; + } + get_ethernet_key(flow, &key); get_ethernet_key(base_flow, &base); get_ethernet_key(&wc->masks, &mask); @@ -5881,29 +5885,6 @@ commit_set_ether_addr_action(const struct flow *flow, struct flow *base_flow, } static void -commit_ether_action(const struct flow *flow, struct flow *base_flow, - struct ofpbuf *odp_actions, struct flow_wildcards *wc, - bool use_masked) -{ - if (flow->packet_type == htonl(PT_ETH)) { - if (base_flow->packet_type != htonl(PT_ETH)) { - odp_put_push_eth_action(odp_actions, &flow->dl_src, &flow->dl_dst); - base_flow->packet_type = flow->packet_type; - base_flow->dl_src = flow->dl_src; - base_flow->dl_dst = flow->dl_dst; - } else { - commit_set_ether_addr_action(flow, base_flow, odp_actions, wc, - use_masked); - } - } else { - if (base_flow->packet_type == htonl(PT_ETH)) { - odp_put_pop_eth_action(odp_actions); - base_flow->packet_type = flow->packet_type; - } - } -} - -static void commit_vlan_action(const struct flow* flow, struct flow *base, struct ofpbuf *odp_actions, struct flow_wildcards *wc) { @@ -6340,6 +6321,50 @@ commit_set_pkt_mark_action(const struct flow *flow, struct flow *base_flow, } } +static void +commit_packet_type_change(const struct flow *flow, + struct flow *base_flow, + struct ofpbuf *odp_actions, + struct flow_wildcards *wc, + bool pending_encap) +{ + if (flow->packet_type == base_flow->packet_type) { + return; + } + + if (pending_encap) { + switch (ntohl(flow->packet_type)) { + case PT_ETH: { + /* push_eth */ + odp_put_push_eth_action(odp_actions, &flow->dl_src, + &flow->dl_dst); + base_flow->packet_type = flow->packet_type; + base_flow->dl_src = flow->dl_src; + base_flow->dl_dst = flow->dl_dst; + break; + } + default: + /* Only the above protocols are supported for encap. The check + * is done at action decoding. */ + OVS_NOT_REACHED(); + } + } else { + if (pt_ns(flow->packet_type) == OFPHTN_ETHERTYPE && + base_flow->packet_type == htonl(PT_ETH)) { + /* pop_eth */ + odp_put_pop_eth_action(odp_actions); + base_flow->packet_type = flow->packet_type; + base_flow->dl_src = eth_addr_zero; + base_flow->dl_dst = eth_addr_zero; + } else { + /* All other cases are handled through recirculation. */ + OVS_NOT_REACHED(); + } + } + + wc->masks.packet_type = OVS_BE32_MAX; +} + /* If any of the flow key data that ODP actions can modify are different in * 'base' and 'flow', appends ODP actions to 'odp_actions' that change the flow * key from 'base' into 'flow', and then changes 'base' the same way. Does not @@ -6352,12 +6377,13 @@ commit_set_pkt_mark_action(const struct flow *flow, struct flow *base_flow, enum slow_path_reason commit_odp_actions(const struct flow *flow, struct flow *base, struct ofpbuf *odp_actions, struct flow_wildcards *wc, - bool use_masked) + bool use_masked, bool pending_encap) { enum slow_path_reason slow1, slow2; bool mpls_done = false; - commit_ether_action(flow, base, odp_actions, wc, use_masked); + commit_packet_type_change(flow, base, odp_actions, wc, pending_encap); + commit_set_ether_action(flow, base, odp_actions, wc, use_masked); /* Make packet a non-MPLS packet before committing L3/4 actions, * which would otherwise do nothing. */ if (eth_type_mpls(base->dl_type) && !eth_type_mpls(flow->dl_type)) { diff --git a/lib/odp-util.h b/lib/odp-util.h index fe183a9..f01c069 100644 --- a/lib/odp-util.h +++ b/lib/odp-util.h @@ -274,7 +274,8 @@ enum slow_path_reason commit_odp_actions(const struct flow *, struct flow *base, struct ofpbuf *odp_actions, struct flow_wildcards *wc, - bool use_masked); + bool use_masked, + bool pending_encap); /* ofproto-dpif interface. * diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c index e2247c8..575c59e 100644 --- a/ofproto/ofproto-dpif-xlate.c +++ b/ofproto/ofproto-dpif-xlate.c @@ -234,6 +234,8 @@ struct xlate_ctx { bool in_action_set; /* Currently translating action_set, if true. */ bool in_packet_out; /* Currently translating a packet_out msg, if * true. */ + bool pending_encap; /* Waiting to commit a pending encap + * action, if true. */ uint8_t table_id; /* OpenFlow table ID where flow was found. */ ovs_be64 rule_cookie; /* Cookie of the rule being translated. */ @@ -3233,7 +3235,8 @@ xlate_commit_actions(struct xlate_ctx *ctx) ctx->xout->slow |= commit_odp_actions(&ctx->xin->flow, &ctx->base_flow, ctx->odp_actions, ctx->wc, - use_masked); + use_masked, ctx->pending_encap); + ctx->pending_encap = false; } static void @@ -5431,6 +5434,85 @@ compose_conntrack_action(struct xlate_ctx *ctx, struct ofpact_conntrack *ofc) } static void +rewrite_flow_encap_ethernet(struct xlate_ctx *ctx, + struct flow *flow, + struct flow_wildcards *wc) +{ + wc->masks.packet_type = OVS_BE32_MAX; + if (pt_ns(flow->packet_type) == OFPHTN_ETHERTYPE) { + /* Only adjust the packet_type and zero the dummy Ethernet addresses. */ + ovs_be16 ethertype = pt_ns_type_be(flow->packet_type); + flow->packet_type = htonl(PT_ETH); + flow->dl_src = eth_addr_zero; + flow->dl_dst = eth_addr_zero; + flow->dl_type = ethertype; + } else { + xlate_report_debug(ctx, OFT_ACTION, + "encap(ethernet) unsupported for packet type " + "ethernet"); + /* TODO: Error handling: drop packet. */ + ctx->error = 1; + } +} + +static void +xlate_generic_encap_action(struct xlate_ctx *ctx, + const struct ofpact_encap *encap) +{ + struct flow *flow = &ctx->xin->flow; + struct flow_wildcards *wc = ctx->wc; + + /* Ensure that any pending actions on the inner packet are applied before + * rewriting the flow */ + xlate_commit_actions(ctx); + + /* Rewrite the flow to reflect the effect of pushing the new encap header. */ + switch (ntohl(encap->new_pkt_type)) { + case PT_ETH: + rewrite_flow_encap_ethernet(ctx, flow, wc); + break; + default: + /* TODO: Error handling: Should not happen if the PT is checked + * at decoding */ + break; + } + + if (!ctx->error) { + /* The actual encap datapath action will be generated at next commit. */ + ctx->pending_encap = true; + } +} + +/* Returns true if packet must be recirculated after decapsulation. */ +static bool +xlate_generic_decap_action(struct xlate_ctx *ctx, + const struct ofpact_decap *decap OVS_UNUSED) +{ + struct flow *flow = &ctx->xin->flow; + + /* Ensure that any pending actions on the current packet are applied + * before generating the decap action. */ + xlate_commit_actions(ctx); + + /* We assume for now that the new_pkt_type is PT_USE_NEXT_PROTO. */ + switch (ntohl(flow->packet_type)) { + case PT_ETH: + /* Just change the packet_type. + * Delay generating pop_eth to the next commit. */ + flow->packet_type = htonl(PACKET_TYPE(OFPHTN_ETHERTYPE, + ntohs(flow->dl_type))); + return false; + default: + xlate_report_debug(ctx, OFT_ACTION, + "decap() for unsupported packet type %x", + ntohl(flow->packet_type)); + /* TODO: Error handling: drop packet. */ + ctx->error = 1; + return false; + } +} + +static void recirc_for_mpls(const struct ofpact *a, struct xlate_ctx *ctx) { /* No need to recirculate if already exiting. */ @@ -5922,7 +6004,18 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len, break; case OFPACT_ENCAP: + xlate_generic_encap_action(ctx, ofpact_get_ENCAP(a)); + break; + case OFPACT_DECAP: { + bool recirc_needed = + xlate_generic_decap_action(ctx, ofpact_get_DECAP(a)); + if (!ctx->error && recirc_needed) { + /* Recirculate for parsing of inner packet. */ + ctx_trigger_freeze(ctx); + /* Then continue with next action. */ + a = ofpact_next(a); + } break; } @@ -6255,6 +6348,7 @@ xlate_actions(struct xlate_in *xin, struct xlate_out *xout) .in_group = false, .in_action_set = false, .in_packet_out = xin->in_packet_out, + .pending_encap = false, .table_id = 0, .rule_cookie = OVS_BE64_MAX, @@ -6413,6 +6507,7 @@ xlate_actions(struct xlate_in *xin, struct xlate_out *xout) flow->packet_type = htonl(PT_ETH); flow->dl_src = eth_addr_zero; flow->dl_dst = eth_addr_zero; + ctx.pending_encap = true; } if (!xin->ofpacts && !ctx.rule) {