| Message ID | 20220907065454.548167-1-amusil@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev,v5,1/2] ofproto-dpif-xlate: Extract the freezing processing into a function | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | success | test: success |
On 7 Sep 2022, at 8:54, Ales Musil wrote: > Through out the code there is the same pattern that occurs > in regards to to finish_freezing when ctx->freezing=true or > xlate_action_set when ctx->freezing=false. Extract it to common > function that is called from those places instead. > > Signed-off-by: Ales Musil <amusil@redhat.com> Thanks for this change, it looks good to me. Acked-by: Eelco Chaudron <echaudro@redhat.com>
On Fri, Sep 23, 2022 at 12:29 PM Eelco Chaudron <echaudro@redhat.com> wrote: > > > On 7 Sep 2022, at 8:54, Ales Musil wrote: > > > Through out the code there is the same pattern that occurs > > in regards to to finish_freezing when ctx->freezing=true or > > xlate_action_set when ctx->freezing=false. Extract it to common > > function that is called from those places instead. > > > > Signed-off-by: Ales Musil <amusil@redhat.com> > > Thanks for this change, it looks good to me. > > Acked-by: Eelco Chaudron <echaudro@redhat.com> > > Thank you for the review. Actually I think I have made a mistake. I did not realize that the xlate_action_set() can actually start freezing again. So the following diff should be applied to this patch set. If there will be another version I'll will apply the diff below: diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c index e181e3089..c84d6c9d0 100644 --- a/ofproto/ofproto-dpif-xlate.c +++ b/ofproto/ofproto-dpif-xlate.c @@ -3884,10 +3884,11 @@ xlate_flow_is_protected(const struct xlate_ctx *ctx, const struct flow *flow, co static void xlate_ctx_process_freezing(struct xlate_ctx *ctx) { + if (!ctx->freezing) { + xlate_action_set(ctx); + } if (ctx->freezing) { finish_freezing(ctx); - } else { - xlate_action_set(ctx); } } Thanks, Ales
On 23 Sep 2022, at 12:35, Ales Musil wrote: > On Fri, Sep 23, 2022 at 12:29 PM Eelco Chaudron <echaudro@redhat.com> wrote: > >> >> >> On 7 Sep 2022, at 8:54, Ales Musil wrote: >> >>> Through out the code there is the same pattern that occurs >>> in regards to to finish_freezing when ctx->freezing=true or >>> xlate_action_set when ctx->freezing=false. Extract it to common >>> function that is called from those places instead. >>> >>> Signed-off-by: Ales Musil <amusil@redhat.com> >> >> Thanks for this change, it looks good to me. >> >> Acked-by: Eelco Chaudron <echaudro@redhat.com> >> >> > Thank you for the review. Actually I think I have made a mistake. > I did not realize that the xlate_action_set() can actually start freezing > again. > So the following diff should be applied to this patch set. If there will be > another version > I'll will apply the diff below: > > > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c > index e181e3089..c84d6c9d0 100644 > --- a/ofproto/ofproto-dpif-xlate.c > +++ b/ofproto/ofproto-dpif-xlate.c > @@ -3884,10 +3884,11 @@ xlate_flow_is_protected(const struct xlate_ctx > *ctx, const struct flow *flow, co > static void > xlate_ctx_process_freezing(struct xlate_ctx *ctx) > { > + if (!ctx->freezing) { > + xlate_action_set(ctx); > + } > if (ctx->freezing) { > finish_freezing(ctx); > - } else { > - xlate_action_set(ctx); > } > } > Yes you are right, I totally missed the “else” when reviewing :( //Eelco
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c index fda802e83..0b5080e39 100644 --- a/ofproto/ofproto-dpif-xlate.c +++ b/ofproto/ofproto-dpif-xlate.c @@ -3861,6 +3861,16 @@ xlate_flow_is_protected(const struct xlate_ctx *ctx, const struct flow *flow, co xport_in->xbundle->protected && xport_out->xbundle->protected); } +static void +xlate_ctx_process_freezing(struct xlate_ctx *ctx) +{ + if (ctx->freezing) { + finish_freezing(ctx); + } else { + xlate_action_set(ctx); + } +} + /* Function handles when a packet is sent from one bridge to another bridge. * * The bridges are internally connected, either with patch ports or with @@ -3921,12 +3931,7 @@ patch_port_output(struct xlate_ctx *ctx, const struct xport *in_dev, xlate_table_action(ctx, flow->in_port.ofp_port, 0, true, true, false, is_last_action, clone_xlate_actions); - if (!ctx->freezing) { - xlate_action_set(ctx); - } - if (ctx->freezing) { - finish_freezing(ctx); - } + xlate_ctx_process_freezing(ctx); } else { /* Forwarding is disabled by STP and RSTP. Let OFPP_NORMAL and * the learning action look at the packet, then drop it. */ @@ -5844,12 +5849,7 @@ clone_xlate_actions(const struct ofpact *actions, size_t actions_len, 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); - if (!ctx->freezing) { - xlate_action_set(ctx); - } - if (ctx->freezing) { - finish_freezing(ctx); - } + xlate_ctx_process_freezing(ctx); goto xlate_done; } @@ -5868,12 +5868,7 @@ clone_xlate_actions(const struct ofpact *actions, size_t actions_len, /* Use clone action as datapath clone. */ offset = nl_msg_start_nested(ctx->odp_actions, OVS_ACTION_ATTR_CLONE); do_xlate_actions(actions, actions_len, ctx, true, false); - if (!ctx->freezing) { - xlate_action_set(ctx); - } - if (ctx->freezing) { - finish_freezing(ctx); - } + xlate_ctx_process_freezing(ctx); nl_msg_end_non_empty_nested(ctx->odp_actions, offset); goto dp_clone_done; } @@ -5884,12 +5879,7 @@ clone_xlate_actions(const struct ofpact *actions, size_t actions_len, ac_offset = nl_msg_start_nested(ctx->odp_actions, OVS_SAMPLE_ATTR_ACTIONS); do_xlate_actions(actions, actions_len, ctx, true, false); - if (!ctx->freezing) { - xlate_action_set(ctx); - } - if (ctx->freezing) { - finish_freezing(ctx); - } + xlate_ctx_process_freezing(ctx); if (nl_msg_end_non_empty_nested(ctx->odp_actions, ac_offset)) { nl_msg_cancel_nested(ctx->odp_actions, offset); } else { @@ -6450,12 +6440,7 @@ xlate_check_pkt_larger(struct xlate_ctx *ctx, value.u8_val = 1; mf_write_subfield_flow(&check_pkt_larger->dst, &value, &ctx->xin->flow); do_xlate_actions(remaining_acts, remaining_acts_len, ctx, true, false); - if (!ctx->freezing) { - xlate_action_set(ctx); - } - if (ctx->freezing) { - finish_freezing(ctx); - } + xlate_ctx_process_freezing(ctx); nl_msg_end_nested(ctx->odp_actions, offset_attr); ctx->base_flow = old_base; @@ -6475,12 +6460,7 @@ xlate_check_pkt_larger(struct xlate_ctx *ctx, value.u8_val = 0; mf_write_subfield_flow(&check_pkt_larger->dst, &value, &ctx->xin->flow); do_xlate_actions(remaining_acts, remaining_acts_len, ctx, true, false); - if (!ctx->freezing) { - xlate_action_set(ctx); - } - if (ctx->freezing) { - finish_freezing(ctx); - } + xlate_ctx_process_freezing(ctx); nl_msg_end_nested(ctx->odp_actions, offset_attr); nl_msg_end_nested(ctx->odp_actions, offset);
Through out the code there is the same pattern that occurs in regards to to finish_freezing when ctx->freezing=true or xlate_action_set when ctx->freezing=false. Extract it to common function that is called from those places instead. Signed-off-by: Ales Musil <amusil@redhat.com> --- ofproto/ofproto-dpif-xlate.c | 52 +++++++++++------------------------- 1 file changed, 16 insertions(+), 36 deletions(-)