| Message ID | 1595497526-16084-1-git-send-email-rudrasurya.r@altencalsoftlabs.com |
|---|---|
| State | Rejected |
| Headers | show |
| Series | [ovs-dev] ofproto/trace: Support for DP-HASH recirculation | expand |
Hi Team, Please consider this as gentle reminder. Could you please review below changes. Thanks & Regards, Surya. -----Original Message----- From: Surya Rudra <rudrasurya.r@altencalsoftlabs.com> Sent: 23 July 2020 03:15 PM To: ovs-dev@openvswitch.org Cc: Surya Rudra <rudrasurya.r@altencalsoftlabs.com> Subject: [PATCH] ofproto/trace: Support for DP-HASH recirculation Issue: Current OVS implementation, recirculation information is not displayed in ofproto trace command if it happens for dp-hash calculation. Fix: Updated ofproto/trace implementation to support dp-hash recirculation. Test: Added unit testcase to test the changes. Signed-off-by: Surya Rudra <rudrasurya.r@altencalsoftlabs.com> --- ofproto/ofproto-dpif-trace.c | 19 ++++++++++++++++++- ofproto/ofproto-dpif-trace.h | 5 ++++- ofproto/ofproto-dpif-xlate.c | 16 +++++++++++++++- tests/ofproto-dpif.at | 21 +++++++++++++++++++++ 4 files changed, 58 insertions(+), 3 deletions(-) diff --git a/ofproto/ofproto-dpif-trace.c b/ofproto/ofproto-dpif-trace.c index 78a54c7..d558c4b 100644 --- a/ofproto/ofproto-dpif-trace.c +++ b/ofproto/ofproto-dpif-trace.c @@ -88,7 +88,7 @@ oftrace_add_recirc_node(struct ovs_list *recirc_queue, enum oftrace_recirc_type type, const struct flow *flow, const struct ofpact_nat *ofn, const struct dp_packet *packet, uint32_t recirc_id, - const uint16_t zone) + const uint16_t zone, struct ovs_action_hash + *hash_act) { if (!recirc_id_node_find_and_ref(recirc_id)) { return false; @@ -99,6 +99,7 @@ oftrace_add_recirc_node(struct ovs_list *recirc_queue, node->type = type; node->recirc_id = recirc_id; + node->act_hash = hash_act; node->flow = *flow; node->flow.recirc_id = recirc_id; node->flow.ct_zone = zone; @@ -682,6 +683,22 @@ ofproto_trace_recirc_node(struct oftrace_recirc_node *node, ds_destroy(&s); } node->flow.ct_state = ct_state; + } else if (node->type == OFT_RECIRC_DP_HASH) { + const struct ovs_action_hash *hash_act = node->act_hash; + uint32_t hash = 0; + switch (hash_act->hash_alg) { + case OVS_HASH_ALG_SYM_L4: + hash = flow_hash_symmetric_l3l4(&node->flow, + hash_act->hash_basis, + false); + break; + case OVS_HASH_ALG_L4: + default: + hash = flow_hash_5tuple(&node->flow, + hash_act->hash_basis); + break; + } + node->flow.dp_hash = hash; } ds_put_char(output, '\n'); diff --git a/ofproto/ofproto-dpif-trace.h b/ofproto/ofproto-dpif-trace.h index 4b04f17..e50c415 100644 --- a/ofproto/ofproto-dpif-trace.h +++ b/ofproto/ofproto-dpif-trace.h @@ -54,6 +54,7 @@ enum oftrace_recirc_type { OFT_RECIRC_CONNTRACK, OFT_RECIRC_MPLS, OFT_RECIRC_BOND, + OFT_RECIRC_DP_HASH, }; /* A node within a trace. */ @@ -74,6 +75,7 @@ struct oftrace_recirc_node { struct flow flow; struct dp_packet *packet; const struct ofpact_nat *nat_act; + struct ovs_action_hash *act_hash; }; /* A node within a next_ct_states list. */ @@ -94,6 +96,7 @@ bool oftrace_add_recirc_node(struct ovs_list *recirc_queue, enum oftrace_recirc_type, const struct flow *, const struct ofpact_nat *, const struct dp_packet *, uint32_t recirc_id, - const uint16_t zone); + const uint16_t zone, + struct ovs_action_hash *hash_act); #endif /* ofproto-dpif-trace.h */ diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c index e0ede2c..08caacb 100644 --- a/ofproto/ofproto-dpif-xlate.c +++ b/ofproto/ofproto-dpif-xlate.c @@ -4977,6 +4977,20 @@ finish_freezing__(struct xlate_ctx *ctx, uint8_t table) sizeof *act_hash); act_hash->hash_alg = ctx->dp_hash_alg; act_hash->hash_basis = ctx->dp_hash_basis; + + if (OVS_UNLIKELY(ctx->xin->trace) && recirc_id) { + if (oftrace_add_recirc_node(ctx->xin->recirc_queue, + OFT_RECIRC_DP_HASH, &ctx->xin->flow, NULL, + ctx->xin->packet, recirc_id, 0, act_hash)) { + xlate_report(ctx, OFT_DETAIL, "A clone of the packet is " + "forked to recirculate. The forked pipeline will " + "be resumed at table %u.", table); + } else { + xlate_report(ctx, OFT_DETAIL, "Failed to trace the " + "DP hash forked pipeline with recirc_id = %d.", + recirc_id); + } + } } nl_msg_put_u32(ctx->odp_actions, OVS_ACTION_ATTR_RECIRC, recirc_id); } @@ -5010,7 +5024,7 @@ compose_recirculate_and_fork(struct xlate_ctx *ctx, uint8_t table, if (oftrace_add_recirc_node(ctx->xin->recirc_queue, OFT_RECIRC_CONNTRACK, &ctx->xin->flow, ctx->ct_nat_action, ctx->xin->packet, - recirc_id, zone)) { + recirc_id, zone, NULL)) { xlate_report(ctx, OFT_DETAIL, "A clone of the packet is forked to " "recirculate. The forked pipeline will be resumed at " "table %u.", table); diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at index feabb73..6adb087 100644 --- a/tests/ofproto-dpif.at +++ b/tests/ofproto-dpif.at @@ -627,6 +627,27 @@ recirc_id(0x1),dp_hash(0xXXXX/0xf),in_port(1),packet_type(ns=0,id=0),eth_ty pe(0x OVS_VSWITCHD_STOP AT_CLEANUP +AT_SETUP([ofproto-dpif - select group in action list]) +OVS_VSWITCHD_START add_of_ports br0 1 10 AT_CHECK([ovs-ofctl -O +OpenFlow12 add-group br0 +'group_id=1234,type=select,bucket=set_field:192.168.3.90->ip_src,output +:10']) AT_CHECK([ovs-ofctl -O OpenFlow12 add-flow br0 'ip +actions=group:1234,output:10']) AT_CHECK([ovs-appctl ofproto/trace br0 +'in_port=1,dl_src=50:54:00:00:00:05,dl_dst=50:54:00:00:00:07,dl_type=0x +0800,nw_src=192.168.0.1,nw_dst=192.168.0.2,nw_proto=1,nw_tos=0,nw_ttl=128, icmp_type=8,icmp_code=0'], [0], [stdout]) AT_CHECK([tail -10 stdout], [0], + [ group:1234 + -> using bucket 0 + bucket 0 + set_field:192.168.3.90->ip_src + output:10 + output:10 + +Final flow: unchanged +Megaflow: +recirc_id=0x1,dp_hash=0xa/0xf,eth,ip,in_port=1,nw_src=192.168.0.1,nw_fr +ag=no Datapath actions: +set(ipv4(src=192.168.3.90)),10,set(ipv4(src=192.168.0.1)),10 +]) +OVS_VSWITCHD_STOP +AT_CLEANUP + AT_SETUP([ofproto-dpif - all group in action set]) OVS_VSWITCHD_START add_of_ports br0 1 10 11 -- 2.7.4
On Thu, Jul 23, 2020 at 03:15:26PM +0530, Surya Rudra via dev wrote: > Issue: > Current OVS implementation, recirculation information > is not displayed in ofproto trace command if it happens > for dp-hash calculation. > > Fix: > Updated ofproto/trace implementation to support dp-hash > recirculation. > > Test: > Added unit testcase to test the changes. > > Signed-off-by: Surya Rudra <rudrasurya.r@altencalsoftlabs.com> This is going to be deceptive in the common case where the kernel datapath is used, since the kernel uses a different hash function from OVS userspace.
diff --git a/ofproto/ofproto-dpif-trace.c b/ofproto/ofproto-dpif-trace.c index 78a54c7..d558c4b 100644 --- a/ofproto/ofproto-dpif-trace.c +++ b/ofproto/ofproto-dpif-trace.c @@ -88,7 +88,7 @@ oftrace_add_recirc_node(struct ovs_list *recirc_queue, enum oftrace_recirc_type type, const struct flow *flow, const struct ofpact_nat *ofn, const struct dp_packet *packet, uint32_t recirc_id, - const uint16_t zone) + const uint16_t zone, struct ovs_action_hash *hash_act) { if (!recirc_id_node_find_and_ref(recirc_id)) { return false; @@ -99,6 +99,7 @@ oftrace_add_recirc_node(struct ovs_list *recirc_queue, node->type = type; node->recirc_id = recirc_id; + node->act_hash = hash_act; node->flow = *flow; node->flow.recirc_id = recirc_id; node->flow.ct_zone = zone; @@ -682,6 +683,22 @@ ofproto_trace_recirc_node(struct oftrace_recirc_node *node, ds_destroy(&s); } node->flow.ct_state = ct_state; + } else if (node->type == OFT_RECIRC_DP_HASH) { + const struct ovs_action_hash *hash_act = node->act_hash; + uint32_t hash = 0; + switch (hash_act->hash_alg) { + case OVS_HASH_ALG_SYM_L4: + hash = flow_hash_symmetric_l3l4(&node->flow, + hash_act->hash_basis, + false); + break; + case OVS_HASH_ALG_L4: + default: + hash = flow_hash_5tuple(&node->flow, + hash_act->hash_basis); + break; + } + node->flow.dp_hash = hash; } ds_put_char(output, '\n'); diff --git a/ofproto/ofproto-dpif-trace.h b/ofproto/ofproto-dpif-trace.h index 4b04f17..e50c415 100644 --- a/ofproto/ofproto-dpif-trace.h +++ b/ofproto/ofproto-dpif-trace.h @@ -54,6 +54,7 @@ enum oftrace_recirc_type { OFT_RECIRC_CONNTRACK, OFT_RECIRC_MPLS, OFT_RECIRC_BOND, + OFT_RECIRC_DP_HASH, }; /* A node within a trace. */ @@ -74,6 +75,7 @@ struct oftrace_recirc_node { struct flow flow; struct dp_packet *packet; const struct ofpact_nat *nat_act; + struct ovs_action_hash *act_hash; }; /* A node within a next_ct_states list. */ @@ -94,6 +96,7 @@ bool oftrace_add_recirc_node(struct ovs_list *recirc_queue, enum oftrace_recirc_type, const struct flow *, const struct ofpact_nat *, const struct dp_packet *, uint32_t recirc_id, - const uint16_t zone); + const uint16_t zone, + struct ovs_action_hash *hash_act); #endif /* ofproto-dpif-trace.h */ diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c index e0ede2c..08caacb 100644 --- a/ofproto/ofproto-dpif-xlate.c +++ b/ofproto/ofproto-dpif-xlate.c @@ -4977,6 +4977,20 @@ finish_freezing__(struct xlate_ctx *ctx, uint8_t table) sizeof *act_hash); act_hash->hash_alg = ctx->dp_hash_alg; act_hash->hash_basis = ctx->dp_hash_basis; + + if (OVS_UNLIKELY(ctx->xin->trace) && recirc_id) { + if (oftrace_add_recirc_node(ctx->xin->recirc_queue, + OFT_RECIRC_DP_HASH, &ctx->xin->flow, NULL, + ctx->xin->packet, recirc_id, 0, act_hash)) { + xlate_report(ctx, OFT_DETAIL, "A clone of the packet is " + "forked to recirculate. The forked pipeline will " + "be resumed at table %u.", table); + } else { + xlate_report(ctx, OFT_DETAIL, "Failed to trace the " + "DP hash forked pipeline with recirc_id = %d.", + recirc_id); + } + } } nl_msg_put_u32(ctx->odp_actions, OVS_ACTION_ATTR_RECIRC, recirc_id); } @@ -5010,7 +5024,7 @@ compose_recirculate_and_fork(struct xlate_ctx *ctx, uint8_t table, if (oftrace_add_recirc_node(ctx->xin->recirc_queue, OFT_RECIRC_CONNTRACK, &ctx->xin->flow, ctx->ct_nat_action, ctx->xin->packet, - recirc_id, zone)) { + recirc_id, zone, NULL)) { xlate_report(ctx, OFT_DETAIL, "A clone of the packet is forked to " "recirculate. The forked pipeline will be resumed at " "table %u.", table); diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at index feabb73..6adb087 100644 --- a/tests/ofproto-dpif.at +++ b/tests/ofproto-dpif.at @@ -627,6 +627,27 @@ recirc_id(0x1),dp_hash(0xXXXX/0xf),in_port(1),packet_type(ns=0,id=0),eth_type(0x OVS_VSWITCHD_STOP AT_CLEANUP +AT_SETUP([ofproto-dpif - select group in action list]) +OVS_VSWITCHD_START +add_of_ports br0 1 10 +AT_CHECK([ovs-ofctl -O OpenFlow12 add-group br0 'group_id=1234,type=select,bucket=set_field:192.168.3.90->ip_src,output:10']) +AT_CHECK([ovs-ofctl -O OpenFlow12 add-flow br0 'ip actions=group:1234,output:10']) +AT_CHECK([ovs-appctl ofproto/trace br0 'in_port=1,dl_src=50:54:00:00:00:05,dl_dst=50:54:00:00:00:07,dl_type=0x0800,nw_src=192.168.0.1,nw_dst=192.168.0.2,nw_proto=1,nw_tos=0,nw_ttl=128,icmp_type=8,icmp_code=0'], [0], [stdout]) +AT_CHECK([tail -10 stdout], [0], + [ group:1234 + -> using bucket 0 + bucket 0 + set_field:192.168.3.90->ip_src + output:10 + output:10 + +Final flow: unchanged +Megaflow: recirc_id=0x1,dp_hash=0xa/0xf,eth,ip,in_port=1,nw_src=192.168.0.1,nw_frag=no +Datapath actions: set(ipv4(src=192.168.3.90)),10,set(ipv4(src=192.168.0.1)),10 +]) +OVS_VSWITCHD_STOP +AT_CLEANUP + AT_SETUP([ofproto-dpif - all group in action set]) OVS_VSWITCHD_START add_of_ports br0 1 10 11
Issue: Current OVS implementation, recirculation information is not displayed in ofproto trace command if it happens for dp-hash calculation. Fix: Updated ofproto/trace implementation to support dp-hash recirculation. Test: Added unit testcase to test the changes. Signed-off-by: Surya Rudra <rudrasurya.r@altencalsoftlabs.com> --- ofproto/ofproto-dpif-trace.c | 19 ++++++++++++++++++- ofproto/ofproto-dpif-trace.h | 5 ++++- ofproto/ofproto-dpif-xlate.c | 16 +++++++++++++++- tests/ofproto-dpif.at | 21 +++++++++++++++++++++ 4 files changed, 58 insertions(+), 3 deletions(-)