| Message ID | 1503701479-43894-3-git-send-email-yihung.wei@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
On 08/25/2017 03:51 PM, Yi-Hung Wei wrote: > This patch propagates ct_zone when ofproto/trace automatically runs > through the recirculation process. > > Fixes: e6bc8e749381 ("ofproto/trace: Add support for tracing conntrack recirculation") > Signed-off-by: Yi-Hung Wei <yihung.wei@gmail.com> > --- > ofproto/ofproto-dpif-trace.c | 4 +++- > ofproto/ofproto-dpif-trace.h | 3 ++- > ofproto/ofproto-dpif-xlate.c | 7 ++++--- > tests/ofproto-dpif.at | 14 +++++++------- > 4 files changed, 16 insertions(+), 12 deletions(-) > > diff --git a/ofproto/ofproto-dpif-trace.c b/ofproto/ofproto-dpif-trace.c > index a45c9cfd9619..c3c929520a2d 100644 > --- a/ofproto/ofproto-dpif-trace.c > +++ b/ofproto/ofproto-dpif-trace.c > @@ -91,7 +91,8 @@ oftrace_node_destroy(struct oftrace_node *node) > bool > oftrace_add_recirc_node(struct ovs_list *recirc_queue, > enum oftrace_recirc_type type, const struct flow *flow, > - const struct dp_packet *packet, uint32_t recirc_id) > + const struct dp_packet *packet, uint32_t recirc_id, > + const uint16_t zone) This function is beginning to get a lot of parameters. As a suggestion perhaps you might create a helper struct to contain all the parameters and just pass a pointer. Personally I start looking for ways to cut down on parameter passing when a function gets to 4 or more parameters. Again - just a personal predilection. Otherwise the patch LGTM. Reviewed-by: Greg Rose <gvrose8192@gmail.com> > { > if (!recirc_id_node_find_and_ref(recirc_id)) { > return false; > @@ -104,6 +105,7 @@ oftrace_add_recirc_node(struct ovs_list *recirc_queue, > node->recirc_id = recirc_id; > node->flow = *flow; > node->flow.recirc_id = recirc_id; > + node->flow.ct_zone = zone; > node->packet = packet ? dp_packet_clone(packet) : NULL; > > return true; > diff --git a/ofproto/ofproto-dpif-trace.h b/ofproto/ofproto-dpif-trace.h > index 5178d227ed07..5e51771b19b0 100644 > --- a/ofproto/ofproto-dpif-trace.h > +++ b/ofproto/ofproto-dpif-trace.h > @@ -84,6 +84,7 @@ struct oftrace_node *oftrace_report(struct ovs_list *, enum oftrace_node_type, > const char *text); > bool oftrace_add_recirc_node(struct ovs_list *recirc_queue, > enum oftrace_recirc_type, const struct flow *, > - const struct dp_packet *, uint32_t recirc_id); > + const struct dp_packet *, uint32_t recirc_id, > + const uint16_t zone); > > #endif /* ofproto-dpif-trace.h */ > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c > index 9e1f837cb23e..95c4fef0d9b0 100644 > --- a/ofproto/ofproto-dpif-xlate.c > +++ b/ofproto/ofproto-dpif-xlate.c > @@ -4685,7 +4685,8 @@ finish_freezing(struct xlate_ctx *ctx) > * the remainder of the current action list and asynchronously resume pipeline > * processing in 'table' with the current metadata and action set. */ > static void > -compose_recirculate_and_fork(struct xlate_ctx *ctx, uint8_t table) > +compose_recirculate_and_fork(struct xlate_ctx *ctx, uint8_t table, > + const uint16_t zone) > { > uint32_t recirc_id; > ctx->freezing = true; > @@ -4694,7 +4695,7 @@ compose_recirculate_and_fork(struct xlate_ctx *ctx, uint8_t table) > if (OVS_UNLIKELY(ctx->xin->trace) && recirc_id) { > if (oftrace_add_recirc_node(ctx->xin->recirc_queue, > OFT_RECIRC_CONNTRACK, &ctx->xin->flow, > - ctx->xin->packet, recirc_id)) { > + ctx->xin->packet, recirc_id, zone)) { > xlate_report(ctx, OFT_DETAIL, "A clone of the packet is forked to " > "recirculate. The forked pipeline will be resumed at " > "table %u.", table); > @@ -5764,7 +5765,7 @@ compose_conntrack_action(struct xlate_ctx *ctx, struct ofpact_conntrack *ofc) > > if (ofc->recirc_table != NX_CT_RECIRC_NONE) { > ctx->conntracked = true; > - compose_recirculate_and_fork(ctx, ofc->recirc_table); > + compose_recirculate_and_fork(ctx, ofc->recirc_table, zone); > } > > /* The ct_* fields are only available in the scope of the 'recirc_table' > diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at > index 28a7e827cac2..c76ea4eee1cc 100644 > --- a/tests/ofproto-dpif.at > +++ b/tests/ofproto-dpif.at > @@ -9827,21 +9827,21 @@ table=0,priority=1,action=drop > dnl > dnl Table 1 > dnl > -table=1,priority=10,in_port=1,ct_state=+trk+new,udp,action=ct(commit,zone=0),2 > -table=1,priority=10,in_port=1,ct_state=+trk+est,udp,action=2 > -table=1,priority=10,in_port=2,ct_state=+trk+est,udp,action=1 > +table=1,priority=10,in_port=1,ct_zone=0,ct_state=+trk+new,udp,action=ct(commit,zone=0),2 > +table=1,priority=10,in_port=1,ct_zone=0,ct_state=+trk+est,udp,action=2 > +table=1,priority=10,in_port=2,ct_zone=0,ct_state=+trk+est,udp,action=1 > table=1,priority=1,action=drop > dnl > dnl Table 2 > dnl > -table=2,priority=10,in_port=1,tcp,ct_state=+trk+new,tcp,action=ct(commit,zone=1),ct(table=3,zone=2) > -table=2,priority=10,in_port=1,tcp,ct_state=+trk+est,tcp,action=ct(table=3,zone=2) > +table=2,priority=10,in_port=1,tcp,ct_zone=1,ct_state=+trk+new,tcp,action=ct(commit,zone=1),ct(table=3,zone=2) > +table=2,priority=10,in_port=1,tcp,ct_zone=1,ct_state=+trk+est,tcp,action=ct(table=3,zone=2) > table=2,priority=1,action=drop > dnl > dnl Table 3 > dnl > -table=3,priority=10,in_port=1,tcp,ct_state=+trk+new,tcp,action=ct(commit,zone=2),4 > -table=3,priority=10,in_port=1,tcp,ct_state=+trk+est,tcp,action=3 > +table=3,priority=10,in_port=1,tcp,ct_zone=2,ct_state=+trk+new,tcp,action=ct(commit,zone=2),4 > +table=3,priority=10,in_port=1,tcp,ct_zone=2,ct_state=+trk+est,tcp,action=3 > table=2,priority=1,action=drop > ]) > >
On Thu, Aug 31, 2017 at 02:38:35PM -0700, Greg Rose wrote: > On 08/25/2017 03:51 PM, Yi-Hung Wei wrote: > >This patch propagates ct_zone when ofproto/trace automatically runs > >through the recirculation process. > > > >Fixes: e6bc8e749381 ("ofproto/trace: Add support for tracing conntrack recirculation") > >Signed-off-by: Yi-Hung Wei <yihung.wei@gmail.com> > >--- > > ofproto/ofproto-dpif-trace.c | 4 +++- > > ofproto/ofproto-dpif-trace.h | 3 ++- > > ofproto/ofproto-dpif-xlate.c | 7 ++++--- > > tests/ofproto-dpif.at | 14 +++++++------- > > 4 files changed, 16 insertions(+), 12 deletions(-) > > > >diff --git a/ofproto/ofproto-dpif-trace.c b/ofproto/ofproto-dpif-trace.c > >index a45c9cfd9619..c3c929520a2d 100644 > >--- a/ofproto/ofproto-dpif-trace.c > >+++ b/ofproto/ofproto-dpif-trace.c > >@@ -91,7 +91,8 @@ oftrace_node_destroy(struct oftrace_node *node) > > bool > > oftrace_add_recirc_node(struct ovs_list *recirc_queue, > > enum oftrace_recirc_type type, const struct flow *flow, > >- const struct dp_packet *packet, uint32_t recirc_id) > >+ const struct dp_packet *packet, uint32_t recirc_id, > >+ const uint16_t zone) > > This function is beginning to get a lot of parameters. As a suggestion perhaps you might create a helper struct > to contain all the parameters and just pass a pointer. Personally I start looking for ways to cut down on parameter > passing when a function gets to 4 or more parameters. Again - just a personal predilection. > > Otherwise the patch LGTM. > > Reviewed-by: Greg Rose <gvrose8192@gmail.com> Thanks Yi-Hung and Greg. I applied this to master. Greg, I think that your comment is fair, but it's not a crisis yet so I'll leave that for a possible later improvement.
diff --git a/ofproto/ofproto-dpif-trace.c b/ofproto/ofproto-dpif-trace.c index a45c9cfd9619..c3c929520a2d 100644 --- a/ofproto/ofproto-dpif-trace.c +++ b/ofproto/ofproto-dpif-trace.c @@ -91,7 +91,8 @@ oftrace_node_destroy(struct oftrace_node *node) bool oftrace_add_recirc_node(struct ovs_list *recirc_queue, enum oftrace_recirc_type type, const struct flow *flow, - const struct dp_packet *packet, uint32_t recirc_id) + const struct dp_packet *packet, uint32_t recirc_id, + const uint16_t zone) { if (!recirc_id_node_find_and_ref(recirc_id)) { return false; @@ -104,6 +105,7 @@ oftrace_add_recirc_node(struct ovs_list *recirc_queue, node->recirc_id = recirc_id; node->flow = *flow; node->flow.recirc_id = recirc_id; + node->flow.ct_zone = zone; node->packet = packet ? dp_packet_clone(packet) : NULL; return true; diff --git a/ofproto/ofproto-dpif-trace.h b/ofproto/ofproto-dpif-trace.h index 5178d227ed07..5e51771b19b0 100644 --- a/ofproto/ofproto-dpif-trace.h +++ b/ofproto/ofproto-dpif-trace.h @@ -84,6 +84,7 @@ struct oftrace_node *oftrace_report(struct ovs_list *, enum oftrace_node_type, const char *text); bool oftrace_add_recirc_node(struct ovs_list *recirc_queue, enum oftrace_recirc_type, const struct flow *, - const struct dp_packet *, uint32_t recirc_id); + const struct dp_packet *, uint32_t recirc_id, + const uint16_t zone); #endif /* ofproto-dpif-trace.h */ diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c index 9e1f837cb23e..95c4fef0d9b0 100644 --- a/ofproto/ofproto-dpif-xlate.c +++ b/ofproto/ofproto-dpif-xlate.c @@ -4685,7 +4685,8 @@ finish_freezing(struct xlate_ctx *ctx) * the remainder of the current action list and asynchronously resume pipeline * processing in 'table' with the current metadata and action set. */ static void -compose_recirculate_and_fork(struct xlate_ctx *ctx, uint8_t table) +compose_recirculate_and_fork(struct xlate_ctx *ctx, uint8_t table, + const uint16_t zone) { uint32_t recirc_id; ctx->freezing = true; @@ -4694,7 +4695,7 @@ compose_recirculate_and_fork(struct xlate_ctx *ctx, uint8_t table) if (OVS_UNLIKELY(ctx->xin->trace) && recirc_id) { if (oftrace_add_recirc_node(ctx->xin->recirc_queue, OFT_RECIRC_CONNTRACK, &ctx->xin->flow, - ctx->xin->packet, recirc_id)) { + ctx->xin->packet, recirc_id, zone)) { xlate_report(ctx, OFT_DETAIL, "A clone of the packet is forked to " "recirculate. The forked pipeline will be resumed at " "table %u.", table); @@ -5764,7 +5765,7 @@ compose_conntrack_action(struct xlate_ctx *ctx, struct ofpact_conntrack *ofc) if (ofc->recirc_table != NX_CT_RECIRC_NONE) { ctx->conntracked = true; - compose_recirculate_and_fork(ctx, ofc->recirc_table); + compose_recirculate_and_fork(ctx, ofc->recirc_table, zone); } /* The ct_* fields are only available in the scope of the 'recirc_table' diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at index 28a7e827cac2..c76ea4eee1cc 100644 --- a/tests/ofproto-dpif.at +++ b/tests/ofproto-dpif.at @@ -9827,21 +9827,21 @@ table=0,priority=1,action=drop dnl dnl Table 1 dnl -table=1,priority=10,in_port=1,ct_state=+trk+new,udp,action=ct(commit,zone=0),2 -table=1,priority=10,in_port=1,ct_state=+trk+est,udp,action=2 -table=1,priority=10,in_port=2,ct_state=+trk+est,udp,action=1 +table=1,priority=10,in_port=1,ct_zone=0,ct_state=+trk+new,udp,action=ct(commit,zone=0),2 +table=1,priority=10,in_port=1,ct_zone=0,ct_state=+trk+est,udp,action=2 +table=1,priority=10,in_port=2,ct_zone=0,ct_state=+trk+est,udp,action=1 table=1,priority=1,action=drop dnl dnl Table 2 dnl -table=2,priority=10,in_port=1,tcp,ct_state=+trk+new,tcp,action=ct(commit,zone=1),ct(table=3,zone=2) -table=2,priority=10,in_port=1,tcp,ct_state=+trk+est,tcp,action=ct(table=3,zone=2) +table=2,priority=10,in_port=1,tcp,ct_zone=1,ct_state=+trk+new,tcp,action=ct(commit,zone=1),ct(table=3,zone=2) +table=2,priority=10,in_port=1,tcp,ct_zone=1,ct_state=+trk+est,tcp,action=ct(table=3,zone=2) table=2,priority=1,action=drop dnl dnl Table 3 dnl -table=3,priority=10,in_port=1,tcp,ct_state=+trk+new,tcp,action=ct(commit,zone=2),4 -table=3,priority=10,in_port=1,tcp,ct_state=+trk+est,tcp,action=3 +table=3,priority=10,in_port=1,tcp,ct_zone=2,ct_state=+trk+new,tcp,action=ct(commit,zone=2),4 +table=3,priority=10,in_port=1,tcp,ct_zone=2,ct_state=+trk+est,tcp,action=3 table=2,priority=1,action=drop ])
This patch propagates ct_zone when ofproto/trace automatically runs through the recirculation process. Fixes: e6bc8e749381 ("ofproto/trace: Add support for tracing conntrack recirculation") Signed-off-by: Yi-Hung Wei <yihung.wei@gmail.com> --- ofproto/ofproto-dpif-trace.c | 4 +++- ofproto/ofproto-dpif-trace.h | 3 ++- ofproto/ofproto-dpif-xlate.c | 7 ++++--- tests/ofproto-dpif.at | 14 +++++++------- 4 files changed, 16 insertions(+), 12 deletions(-)