On Fri, Sep 23, 2022 at 1:18 PM Eelco Chaudron <[email protected]> wrote:
> > > On 7 Sep 2022, at 8:54, Ales Musil wrote: > > > When the packet was traveling through patch port boundary > > OvS would check if any of the actions is reversible, > > if not it would clone the packet. However, the check > > was only at the first level of the second bridge. > > That caused some issues when the packet had gone > > through more actions, some of them might have been > > irreversible. > > > > In order to keep the semantics the same we might > > need to run the actions twice in the worst case > > scenario. During the clone there are 4 scenarios > > that can happen. > > > > 1) The action is last one for that flow, > > in that case we just continue without clone. > > > > 2) There is irreversible action in the action > > set (first level). In this case we know that > > there is at leas one irreversible action which > > is enough to do clone. > > > > 3) All actions in first level are reversible, > > we can try to run all actions as if we don't > > need any clone and inspect the ofpbuf at the > > end. In positive case there are no irreversible > > actions so we will just submit the buffer and continue. > > > > 4) This is same as 3) with the difference that > > there is irreversible action in the ofpbuf. > > To keep the semantics we need to re-run the actions > > and treat it as clone. This requires resotration > > of the xlate_ctx. > > > > Add test cases for all irreversible actions > > to see if they are properly cloned. > > > > Signed-off-by: Ales Musil <[email protected]> > > Some more comments on this v5, see inline below. > Thank you for the review. I will address those points in v6. > > > --- > > v4: Rebase on top of current master. > > Address comments from Eelco. > > v5: Make the code more readable and reduce duplication. > > --- > > lib/odp-util.c | 148 +++++++++++++++++++++++++++++ > > lib/odp-util.h | 1 + > > ofproto/ofproto-dpif-trace.c | 2 +- > > ofproto/ofproto-dpif-trace.h | 1 + > > ofproto/ofproto-dpif-xlate.c | 177 +++++++++++++++++++++++++---------- > > tests/ofproto-dpif.at | 105 +++++++++++++++++++++ > > 6 files changed, 386 insertions(+), 48 deletions(-) > > > > diff --git a/lib/odp-util.c b/lib/odp-util.c > > index ba5be4bb3..6526daafb 100644 > > --- a/lib/odp-util.c > > +++ b/lib/odp-util.c > > @@ -8768,3 +8768,151 @@ commit_odp_actions(const struct flow *flow, > struct flow *base, > > > > return slow1 ? slow1 : slow2; > > } > > + > > +static inline bool > > +nlattr_action_is_reversible(const uint16_t type) > > The below function are all named irreversible, so it might confuse people > looking at the code. > Maybe make this function nlattr_action_is_irreversible(), so it's more > consistent. > > > +{ > > + switch ((enum ovs_action_attr) type) { > > + case OVS_ACTION_ATTR_CT: > > + case OVS_ACTION_ATTR_CT_CLEAR: > > + case OVS_ACTION_ATTR_TRUNC: > > + case OVS_ACTION_ATTR_PUSH_ETH: > > + case OVS_ACTION_ATTR_POP_ETH: > > + case OVS_ACTION_ATTR_PUSH_NSH: > > + case OVS_ACTION_ATTR_POP_NSH: > > + case OVS_ACTION_ATTR_METER: > > + case OVS_ACTION_ATTR_TUNNEL_PUSH: > > + case OVS_ACTION_ATTR_TUNNEL_POP: > > + return false; > > + > > + case OVS_ACTION_ATTR_UNSPEC: > > + case OVS_ACTION_ATTR_OUTPUT: > > + case OVS_ACTION_ATTR_USERSPACE: > > + case OVS_ACTION_ATTR_SET: > > + case OVS_ACTION_ATTR_PUSH_VLAN: > > + case OVS_ACTION_ATTR_POP_VLAN: > > + case OVS_ACTION_ATTR_SAMPLE: > > + case OVS_ACTION_ATTR_RECIRC: > > + case OVS_ACTION_ATTR_HASH: > > + case OVS_ACTION_ATTR_SET_MASKED: > > + case OVS_ACTION_ATTR_CLONE: > > + case OVS_ACTION_ATTR_CHECK_PKT_LEN: > > + case OVS_ACTION_ATTR_LB_OUTPUT: > > + case OVS_ACTION_ATTR_ADD_MPLS: > > + case OVS_ACTION_ATTR_PUSH_MPLS: > > + case OVS_ACTION_ATTR_POP_MPLS: > > + case OVS_ACTION_ATTR_DROP: > > + case __OVS_ACTION_ATTR_MAX: > > This is an unknown/unsupported action, I would suggest returning false for > this. > > > + return true; > > + } > > + return false; > > +} > > + > > +static bool > > +odp_cpl_contains_irreversible_actions(const struct nlattr *attr) > > +{ > > + static const struct nl_policy ovs_cpl_policy[] = { > > + [OVS_CHECK_PKT_LEN_ATTR_PKT_LEN] = {.type = NL_A_U16}, > > + [OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_GREATER] = {.type = > NL_A_NESTED}, > > + [OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_LESS_EQUAL] = {.type = > NL_A_NESTED}, > > + }; > > + struct nlattr *a[ARRAY_SIZE(ovs_cpl_policy)]; > > + > > + if (!nl_parse_nested(attr, ovs_cpl_policy, a, ARRAY_SIZE(a))) { > > + return false; > > + } > > + > > + const struct nlattr *greater = > > + a[OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_GREATER]; > > + const struct nlattr *less = > > + a[OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_LESS_EQUAL]; > > + const void *greater_data = nl_attr_get(greater); > > + const void *less_data = nl_attr_get(less); > > + size_t greater_len = nl_attr_get_size(greater); > > + size_t less_len = nl_attr_get_size(less); > > + > > + return odp_contains_irreversible_action(greater_data, greater_len) > || > > + odp_contains_irreversible_action(less_data, less_len); > > +} > > + > > +static bool > > +odp_sample_contains_irreversible_actions(const struct nlattr *attr) > > +{ > > + static const struct nl_policy ovs_sample_policy[] = { > > + [OVS_SAMPLE_ATTR_PROBABILITY] = {.type = NL_A_U32}, > > + [OVS_SAMPLE_ATTR_ACTIONS] = {.type = NL_A_NESTED} > > + }; > > + struct nlattr *a[ARRAY_SIZE(ovs_sample_policy)]; > > + > > + if (!nl_parse_nested(attr, ovs_sample_policy, a, ARRAY_SIZE(a))) { > > + return false; > > + } > > + > > + const struct nlattr *actions = a[OVS_SAMPLE_ATTR_ACTIONS]; > > + const void *actions_data = nl_attr_get(actions); > > + size_t actions_len = nl_attr_get_size(actions); > > + > > + return odp_contains_irreversible_action(actions_data, actions_len); > > +} > > + > > +/* Check if any of the actions in the ofpbuf is irreversible. */ > > +bool > > +odp_contains_irreversible_action(const void *attrs, size_t attrs_len) > > +{ > > + const struct nlattr *attr; > > + int left; > > + > > + NL_ATTR_FOR_EACH (attr, left, attrs, attrs_len) { > > + uint16_t type = attr->nla_type; > > + > > + switch ((enum ovs_action_attr) type) { > > + /* Skip "clone" because it already encapsulates > irreversible * > > No need for the ending “ *” here. > > > + * actions. */ > > + case OVS_ACTION_ATTR_CLONE: > > + continue; > > + /* Check nested actions of "check_packet_len". */ > > + case OVS_ACTION_ATTR_CHECK_PKT_LEN: > > + if (odp_cpl_contains_irreversible_actions(attr)) { > > + return true; > > + } > > + break; > > + /* Check nested actions of "sample". */ > > + case OVS_ACTION_ATTR_SAMPLE: > > + if (odp_sample_contains_irreversible_actions(attr)) { > > + return true; > > + } > > + break; > > + /* Check any other action. */ > > + case OVS_ACTION_ATTR_CT: > > + case OVS_ACTION_ATTR_CT_CLEAR: > > + case OVS_ACTION_ATTR_TRUNC: > > + case OVS_ACTION_ATTR_PUSH_ETH: > > + case OVS_ACTION_ATTR_POP_ETH: > > + case OVS_ACTION_ATTR_PUSH_NSH: > > + case OVS_ACTION_ATTR_POP_NSH: > > + case OVS_ACTION_ATTR_METER: > > + case OVS_ACTION_ATTR_TUNNEL_PUSH: > > + case OVS_ACTION_ATTR_TUNNEL_POP: > > + case OVS_ACTION_ATTR_UNSPEC: > > + case OVS_ACTION_ATTR_OUTPUT: > > + case OVS_ACTION_ATTR_USERSPACE: > > + case OVS_ACTION_ATTR_SET: > > + case OVS_ACTION_ATTR_PUSH_VLAN: > > + case OVS_ACTION_ATTR_POP_VLAN: > > + case OVS_ACTION_ATTR_RECIRC: > > + case OVS_ACTION_ATTR_HASH: > > + case OVS_ACTION_ATTR_SET_MASKED: > > + case OVS_ACTION_ATTR_LB_OUTPUT: > > + case OVS_ACTION_ATTR_ADD_MPLS: > > + case OVS_ACTION_ATTR_PUSH_MPLS: > > + case OVS_ACTION_ATTR_POP_MPLS: > > + case OVS_ACTION_ATTR_DROP: > > + if (!nlattr_action_is_reversible(type)) { > > + return true; > > + } > > + case __OVS_ACTION_ATTR_MAX: > > This is the same as an invalid action, so I think we should return false. > We should probably return false on each unknown option. > How about the following change? > Applied, but I think you have meant to return true, as any unknown action being irreversible by default? > > --- a/lib/odp-util.c > +++ b/lib/odp-util.c > @@ -8875,13 +8875,13 @@ odp_contains_irreversible_action(const void > *attrs, size_t attrs_len) > if (odp_cpl_contains_irreversible_actions(attr)) { > return true; > } > - break; > + continue; > /* Check nested actions of "sample". */ > case OVS_ACTION_ATTR_SAMPLE: > if (odp_sample_contains_irreversible_actions(attr)) { > return true; > } > - break; > + continue; > /* Check any other action. */ > case OVS_ACTION_ATTR_CT: > case OVS_ACTION_ATTR_CT_CLEAR: > @@ -8910,9 +8910,11 @@ odp_contains_irreversible_action(const void *attrs, > size_t attrs_len) > if (!nlattr_action_is_reversible(type)) { > return true; > } > - case __OVS_ACTION_ATTR_MAX: > continue; > + case __OVS_ACTION_ATTR_MAX: > + break; > } > + return false; > } > return false; > } > > > > + continue; > > + } > > + } > > + return false; > > +} > > diff --git a/lib/odp-util.h b/lib/odp-util.h > > index a1d0d0fba..d842974ba 100644 > > --- a/lib/odp-util.h > > +++ b/lib/odp-util.h > > @@ -373,6 +373,7 @@ void odp_put_pop_eth_action(struct ofpbuf > *odp_actions); > > void odp_put_push_eth_action(struct ofpbuf *odp_actions, > > const struct eth_addr *eth_src, > > const struct eth_addr *eth_dst); > > +bool odp_contains_irreversible_action(const void *attrs, size_t > attrs_len); > > > > struct attr_len_tbl { > > int len; > > diff --git a/ofproto/ofproto-dpif-trace.c b/ofproto/ofproto-dpif-trace.c > > index 109940ad2..c5102e3d9 100644 > > --- a/ofproto/ofproto-dpif-trace.c > > +++ b/ofproto/ofproto-dpif-trace.c > > @@ -108,7 +108,7 @@ oftrace_add_recirc_node(struct ovs_list > *recirc_queue, > > return true; > > } > > > > -static void > > +void > > oftrace_recirc_node_destroy(struct oftrace_recirc_node *node) > > { > > if (node) { > > diff --git a/ofproto/ofproto-dpif-trace.h b/ofproto/ofproto-dpif-trace.h > > index 4b04f1756..463ff5b16 100644 > > --- a/ofproto/ofproto-dpif-trace.h > > +++ b/ofproto/ofproto-dpif-trace.h > > @@ -95,5 +95,6 @@ bool oftrace_add_recirc_node(struct ovs_list > *recirc_queue, > > const struct ofpact_nat *, > > const struct dp_packet *, uint32_t > recirc_id, > > const uint16_t zone); > > +void oftrace_recirc_node_destroy(struct oftrace_recirc_node *node); > > > > #endif /* ofproto-dpif-trace.h */ > > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c > > index 0b5080e39..c93a8131f 100644 > > --- a/ofproto/ofproto-dpif-xlate.c > > +++ b/ofproto/ofproto-dpif-xlate.c > > @@ -5748,6 +5748,97 @@ xlate_sample_action(struct xlate_ctx *ctx, > > compose_sample_action(ctx, probability, &cookie, tunnel_out_port, > false); > > } > > > > +/* Struct to store the xlate_ctx that needs to be restored after > > +* clone is finished. */ > > +struct xlate_ctx_clone_state { > > + uint32_t stack_size; > > + uint32_t action_set_size; > > + uint32_t odp_actions_size; > > + > > + struct flow xin_flow; > > + struct flow base_flow; > > + > > + bool was_mpls; > > + bool conntracked; > > + > > + size_t recirc_size; > > + > > + int resubmit; > > + uint32_t sflow_n_outputs; > > +}; > > + > > +static struct xlate_ctx_clone_state > > +xlate_ctx_clone_state_store(struct xlate_ctx *ctx) > > +{ > > + struct xlate_ctx_clone_state state = { > > + .stack_size = ctx->stack.size, > > + .action_set_size = ctx->action_set.size, > > + .odp_actions_size = ctx->odp_actions->size, > > + .xin_flow = ctx->xin->flow, > > + .base_flow = ctx->base_flow, > > + .was_mpls = ctx->was_mpls, > > + .conntracked = ctx->conntracked, > > + .recirc_size = ctx->xin->recirc_queue > > + ? ovs_list_size(ctx->xin->recirc_queue) > > + : 0, > > + .resubmit = ctx->resubmits, > > + .sflow_n_outputs = ctx->sflow_n_outputs, > > + }; > > + return state; > > +} > > + > > +/* Restore pieces that need to be restored always. */ > > +static void > > +xlate_ctx_clone_state_restore_common(struct xlate_ctx *ctx, > > + struct xlate_ctx_clone_state > *old_state) > > +{ > > + ctx->stack.size = old_state->stack_size; > > + ctx->action_set.size = old_state->action_set_size; > > + ctx->xin->flow = old_state->xin_flow; > > +} > > + > > +/* Restore xlate_ctx member to the old_state, with additional steps for > > + * revert. */ > > +static void > > +xlate_ctx_clone_state_restore(struct xlate_ctx *ctx, > > + struct xlate_ctx_clone_state *old_state, > > + bool revert) > > Having two functions for restore is a bit confusing to me. Maybe there is > a nicer way? Without much thought, an additional flag could be added, i.e., > > xlate_ctx_clone_state_restore(struct xlate_ctx *ctx, > struct xlate_ctx_clone_state *old_state, > bool none_clone_restore, bool revert) > > if (!none_clone_restore) { > > /* The clone's conntrack execution should have no effect on the > original > ... > ... > ctx->base_flow = old_state->base_flow; > } > > > +{ > > + xlate_ctx_clone_state_restore_common(ctx, old_state); > > + > > + /* The clone's conntrack execution should have no effect on the > original > > + * packet. */ > > + ctx->conntracked = old_state->conntracked; > > + > > + /* Popping MPLS from the clone should have no effect on the original > > + * packet. */ > > + ctx->was_mpls = old_state->was_mpls; > > + > > + /* Restore the 'base_flow' for the next action. */ > > + ctx->base_flow = old_state->base_flow; > > + > > + if (revert) { > > + ctx->resubmits = old_state->resubmit; > > + ctx->sflow_n_outputs = old_state->sflow_n_outputs; > > + /* Revert the actions that were added by non-clone path. */ > > nit: add cr/lf > > > + ctx->odp_actions->size = old_state->odp_actions_size; > > nit: add cr/lf > > > + /* Clear recirculations that were added by non-clone path so > that > > + * packets are not recirculated twice. */ > > + size_t old_recirc_size = old_state->recirc_size; > > + size_t recirc_size = ctx->xin->recirc_queue > > + ? ovs_list_size(ctx->xin->recirc_queue) > > + : 0; > > + > > + for (; old_recirc_size < recirc_size; old_recirc_size++) { > > + struct oftrace_recirc_node *node = > > + CONTAINER_OF(ovs_list_pop_back(ctx->xin->recirc_queue), > > + struct oftrace_recirc_node, node); > > + > > + oftrace_recirc_node_destroy(node); > > + } > > + } > > +} > > + > > /* Determine if an datapath action translated from the openflow action > > * can be reversed by another datapath action. > > * > > @@ -5829,37 +5920,18 @@ reversible_actions(const struct ofpact *ofpacts, > size_t ofpacts_len) > > } > > > > static void > > -clone_xlate_actions(const struct ofpact *actions, size_t actions_len, > > - struct xlate_ctx *ctx, bool is_last_action, > > - bool group_bucket_action OVS_UNUSED) > > +do_clone_xlate_actions(const struct ofpact *actions, size_t actions_len, > > + struct xlate_ctx *ctx) > > { > > - struct ofpbuf old_stack = ctx->stack; > > - union mf_subvalue new_stack[1024 / sizeof(union mf_subvalue)]; > > For the stack and action_set you no longer reserve a clean array. > I think this was needed to stop it from leaking, see commit “b827b231”. > The commit states that it is a design decision, I don't mind having it as before. It just seemed to me that this won't harm, also before that commit it was not "clearing" the stack or action_set at all, which I do by moving the size back. This avoids taking up stack space. > > > - ofpbuf_use_stub(&ctx->stack, new_stack, sizeof new_stack); > > - ofpbuf_put(&ctx->stack, old_stack.data, old_stack.size); > > - > > - struct ofpbuf old_action_set = ctx->action_set; > > - uint64_t actset_stub[1024 / 8]; > > - ofpbuf_use_stub(&ctx->action_set, actset_stub, sizeof actset_stub); > > - ofpbuf_put(&ctx->action_set, old_action_set.data, > old_action_set.size); > > - > > size_t offset, ac_offset; > > - struct flow old_flow = ctx->xin->flow; > > - > > - if (reversible_actions(actions, actions_len) || is_last_action) { > > - old_flow = ctx->xin->flow; > > - do_xlate_actions(actions, actions_len, ctx, is_last_action, > false); > > - xlate_ctx_process_freezing(ctx); > > - goto xlate_done; > > - } > > + struct xlate_ctx_clone_state old_state = > xlate_ctx_clone_state_store(ctx); > > > > /* Commit datapath actions before emitting the clone action to > > * avoid emitting those actions twice. Once inside > > * the clone, another time for the action after clone. */ > > xlate_commit_actions(ctx); > > - struct flow old_base = ctx->base_flow; > > - bool old_was_mpls = ctx->was_mpls; > > - bool old_conntracked = ctx->conntracked; > > + /* The base needs to be stored after xlate_commit_actions. */ > > + old_state.base_flow = ctx->base_flow; > > > > /* The actions are not reversible, a datapath clone action is > > * required to encode the translation. Select the clone action > > @@ -5870,10 +5942,7 @@ clone_xlate_actions(const struct ofpact *actions, > size_t actions_len, > > do_xlate_actions(actions, actions_len, ctx, true, false); > > xlate_ctx_process_freezing(ctx); > > nl_msg_end_non_empty_nested(ctx->odp_actions, offset); > > - goto dp_clone_done; > > - } > > - > > - if (ctx->xbridge->support.sample_nesting > 3) { > > + } else if (ctx->xbridge->support.sample_nesting > 3) { > > /* Use sample action as datapath clone. */ > > offset = nl_msg_start_nested(ctx->odp_actions, > OVS_ACTION_ATTR_SAMPLE); > > ac_offset = nl_msg_start_nested(ctx->odp_actions, > > @@ -5887,31 +5956,45 @@ clone_xlate_actions(const struct ofpact > *actions, size_t actions_len, > > UINT32_MAX); /* 100% probability. */ > > nl_msg_end_nested(ctx->odp_actions, offset); > > } > > - goto dp_clone_done; > > + } else { > > + /* Datapath does not support clone, skip xlate 'oc' and > > + * report an error */ > > + xlate_report_error(ctx, "Failed to compose clone action"); > > } > > + xlate_ctx_clone_state_restore(ctx, &old_state, false); > > +} > > > > - /* Datapath does not support clone, skip xlate 'oc' and > > - * report an error */ > > - xlate_report_error(ctx, "Failed to compose clone action"); > > +static void > > +clone_xlate_actions(const struct ofpact *actions, size_t actions_len, > > + struct xlate_ctx *ctx, bool is_last_action, > > + bool group_bucket_action OVS_UNUSED) > > +{ > > + /* We have to clone if any of the actions is irreversible. */ > > + if (!is_last_action && !reversible_actions(actions, actions_len)) { > > + do_clone_xlate_actions(actions, actions_len, ctx); > > + return; > > + } > > > > -dp_clone_done: > > - /* The clone's conntrack execution should have no effect on the > original > > - * packet. */ > > - ctx->conntracked = old_conntracked; > > + /* Let's run the actions as if they are reversible, if we find out > that > > + * there was any irreversible action we can restore the xlate_ctx > and run > > + * the do_clone instead. If the action is last we don't have to > check and > > + * can just proceed with the actions. */ > > Any stats to suggest that this is the best approach? I’m wondering if this > is the common case, or if the real clone is more common. If it's the > latter, we might want to swap the order. > > Maybe you can do some tests with OVN and add two coverage counters, one > for total invocation of this function and one for real clones, and see the > results while running through a real-user OVN use case? > I will try to come up with some OVN tests that might give a better perspective which is more common. For start I'll run the counters over our tests in OVN. > > > + struct xlate_ctx_clone_state old_state = > xlate_ctx_clone_state_store(ctx); > > > > - /* Popping MPLS from the clone should have no effect on the original > > - * packet. */ > > - ctx->was_mpls = old_was_mpls; > > + do_xlate_actions(actions, actions_len, ctx, is_last_action, false); > > > > - /* Restore the 'base_flow' for the next action. */ > > - ctx->base_flow = old_base; > > + void *added_actions = > > + ofpbuf_at_assert(ctx->odp_actions, old_state.odp_actions_size, > 0); > > + uint32_t added_size = ctx->odp_actions->size - > old_state.odp_actions_size; > > > > -xlate_done: > > - ofpbuf_uninit(&ctx->action_set); > > - ctx->action_set = old_action_set; > > - ofpbuf_uninit(&ctx->stack); > > - ctx->stack = old_stack; > > - ctx->xin->flow = old_flow; > > + if (is_last_action > > + || !odp_contains_irreversible_action(added_actions, > added_size)) { > > + xlate_ctx_process_freezing(ctx); > > + xlate_ctx_clone_state_restore_common(ctx, &old_state); > > + } else { > > + xlate_ctx_clone_state_restore(ctx, &old_state, true); > > + do_clone_xlate_actions(actions, actions_len, ctx); > > + } > > } > > > > static void > > diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at > > index 2e91ae1a1..6719479e5 100644 > > --- a/tests/ofproto-dpif.at > > +++ b/tests/ofproto-dpif.at > > @@ -11817,3 +11817,108 @@ AT_CHECK([test 1 = `ovs-ofctl parse-pcap > p2-tx.pcap | wc -l`]) > > > > OVS_VSWITCHD_STOP > > AT_CLEANUP > > + > > +AT_SETUP([ofproto-dpif - nested patch ports clone]) > > + > > +OVS_VSWITCHD_START( > > + [add-port br0 br0-p0 -- set Interface br0-p0 type=dummy > ofport_request=1 -- \ > > + add-port br0 br0-p1 -- set Interface br0-p1 type=patch > options:peer=br1-p0 -- \ > > + add-br br1 -- \ > > + set bridge br1 other-config:hwaddr=aa:66:aa:66:00:01 -- \ > > + set bridge br1 datapath-type=dummy other-config:datapath-id=1111 > fail-mode=secure -- \ > > + add-port br1 br1-p0 -- set Interface br1-p0 type=patch > options:peer=br0-p1 -- \ > > + add-port br1 br1-p1 -- set Interface br1-p1 type=patch > options:peer=br2-p0 -- \ > > + add-port br1 br1-p2 -- set Interface br1-p2 type=dummy > ofport_request=2 --\ > > + add-br br2 -- \ > > + set bridge br2 other-config:hwaddr=aa:66:aa:66:00:02 -- \ > > + set bridge br2 datapath-type=dummy other-config:datapath-id=2222 > fail-mode=secure -- \ > > + add-port br2 br2-p0 -- set Interface br2-p0 type=patch > options:peer=br1-p1 -- \ > > + add-port br2 br2-p1 -- set Interface br2-p1 type=dummy > ofport_request=3]) > > + > > +AT_CHECK([ovs-ofctl -O OpenFlow13 add-meter br1 'meter=1 pktps stats > bands=type=drop rate=2']) > > +AT_CHECK([ovs-ofctl -O OpenFlow13 add-meter br2 'meter=1 pktps stats > bands=type=drop rate=2']) > > +AT_CHECK([ovs-ofctl del-flows br0]) > > +AT_CHECK([ovs-ofctl -O OpenFlow13 add-flow br0 > in_port=local,ip,actions=br0-p1,br0-p0]) > > +AT_CHECK([ovs-ofctl -O OpenFlow13 add-flow br1 > in_port=br1-p0,ip,actions=meter:1,br1-p1,br1-p2]) > > +AT_CHECK([ovs-ofctl -O OpenFlow13 add-flow br2 > in_port=br2-p0,ip,actions=meter:1,br2-p1]) > > + > > +AT_CHECK([ovs-appctl ofproto/trace br0 "in_port(local),ip" | grep > "Datapath actions:"], [0], [dnl > > +Datapath actions: clone(meter(0),clone(meter(1),102),101),100 > > +]) > > + > > +OVS_VSWITCHD_STOP > > +AT_CLEANUP > > + > > +AT_SETUP([ofproto-dpif - patch ports - ct (clone)]) > > + > > +OVS_VSWITCHD_START( > > + [add-port br0 p0 -- set Interface p0 type=dummy ofport_request=1 -- \ > > + add-port br0 p1 -- set Interface p1 type=patch options:peer=p2 > ofport_request=2 -- \ > > + add-br br1 -- \ > > + set bridge br1 other-config:hwaddr=aa:66:aa:66:00:00 -- \ > > + set bridge br1 datapath-type=dummy other-config:datapath-id=1234 > fail-mode=secure -- \ > > + add-port br1 p2 -- set Interface p2 type=patch options:peer=p1 -- \ > > + add-port br1 p3 -- set Interface p3 type=dummy ofport_request=3]) > > + > > +AT_CHECK([ovs-ofctl del-flows br0]) > > +AT_CHECK([ovs-ofctl -O OpenFlow13 add-flow br0 > in_port=local,ip,actions=p1,p0]) > > +AT_CHECK([ovs-ofctl -O OpenFlow13 add-flow br1 > "in_port=1,ip,actions=resubmit(,3)"]) > > +AT_CHECK([ovs-ofctl -O OpenFlow13 add-flow br1 > "table=3,in_port=1,ip,ct_state=-trk,actions=ct(table=0)"]) > > +AT_CHECK([ovs-ofctl -O OpenFlow13 add-flow br1 > "table=3,in_port=1,ip,ct_state=+trk,actions=p3"]) > > + > > +AT_CHECK([ovs-appctl ofproto/trace br0 "in_port(local),ip" | \ > > + sed 's/recirc(0x[[0-9]]*)/recirc(X)/' | grep "Datapath > actions:"], [0], [dnl > > There is a strip_recirc() macro which you could use. > > > +Datapath actions: clone(ct,recirc(X)),1 > > +Datapath actions: 3 > > +]) > > + > > +OVS_VSWITCHD_STOP > > +AT_CLEANUP > > + > > + > > +AT_SETUP([ofproto-dpif - patch ports - trunc (clone)]) > > + > > +OVS_VSWITCHD_START( > > + [add-port br0 p0 -- set Interface p0 type=dummy ofport_request=1 -- \ > > + add-port br0 p1 -- set Interface p1 type=patch options:peer=p2 > ofport_request=2 -- \ > > + add-br br1 -- \ > > + set bridge br1 other-config:hwaddr=aa:66:aa:66:00:00 -- \ > > + set bridge br1 datapath-type=dummy other-config:datapath-id=1234 > fail-mode=secure -- \ > > + add-port br1 p2 -- set Interface p2 type=patch options:peer=p1 -- \ > > + add-port br1 p3 -- set Interface p3 type=dummy ofport_request=3]) > > + > > +AT_CHECK([ovs-ofctl del-flows br0]) > > +AT_CHECK([ovs-ofctl -O OpenFlow13 add-flow br0 > in_port=local,ip,actions=p1,p0]) > > +AT_CHECK([ovs-ofctl -O OpenFlow13 add-flow br1 > "in_port=1,ip,actions=resubmit(,3)"]) > > +AT_CHECK([ovs-ofctl -O OpenFlow13 add-flow br1 > "table=3,in_port=1,ip,actions=output(port=p3, max_len=20)"]) > > + > > +AT_CHECK([ovs-appctl ofproto/trace br0 "in_port(local),ip" | grep > "Datapath actions:"], [0], [dnl > > +Datapath actions: clone(trunc(20),3),1 > > +]) > > + > > +OVS_VSWITCHD_STOP > > +AT_CLEANUP > > + > > +AT_SETUP([ofproto-dpif - patch ports - encap (clone)]) > > + > > +OVS_VSWITCHD_START( > > + [add-port br0 p0 -- set Interface p0 type=dummy ofport_request=1 -- \ > > + add-port br0 p1 -- set Interface p1 type=patch options:peer=p2 > ofport_request=2 -- \ > > + add-br br1 -- \ > > + set bridge br1 other-config:hwaddr=aa:66:aa:66:00:00 -- \ > > + set bridge br1 datapath-type=dummy other-config:datapath-id=1234 > fail-mode=secure -- \ > > + add-port br1 p2 -- set Interface p2 type=patch options:peer=p1 -- \ > > + add-port br1 p3 -- set Interface p3 type=dummy ofport_request=3]) > > + > > +AT_CHECK([ovs-ofctl del-flows br0]) > > +AT_CHECK([ovs-ofctl -O OpenFlow13 add-flow br0 > "in_port=local,ip,actions=encap(nsh()),encap(ethernet),p1,p0"]) > > +AT_CHECK([ovs-ofctl -O OpenFlow13 add-flow br1 > "in_port=1,dl_type=0x894f,actions=resubmit(,3)"]) > > +AT_CHECK([ovs-ofctl -O OpenFlow13 add-flow br1 > "in_port=1,dl_type=0x894f,actions=decap(),decap(),p3"]) > > + > > +AT_CHECK([ovs-appctl ofproto/trace br0 "in_port(local),ip" | \ > > + sed 's/recirc(0x[[0-9]]*)/recirc(X)/' | grep "Datapath > actions:"], [0], [dnl > > There is a strip_recirc() macro which you could use. > > > +Datapath actions: > push_nsh(flags=0,ttl=63,mdtype=1,np=3,spi=0x0,si=255,c1=0x0,c2=0x0,c3=0x0,c4=0x0),push_eth(src=00:00:00:00:00:00,dst=00:00:00:00:00:00),clone(pop_eth,pop_nsh(),recirc(X)),1 > > +]) > > + > > +OVS_VSWITCHD_STOP > > +AT_CLEANUP > > The code has two exception cases for check packet length, and sample, > maybe we should include tests for these also so the code is covered. > > > -- > > 2.37.2 > > > > _______________________________________________ > > dev mailing list > > [email protected] > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > Thanks, Ales -- Ales Musil Senior Software Engineer - OVN Core Red Hat EMEA <https://www.redhat.com> [email protected] IM: amusil <https://red.ht/sig> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
