| Message ID | 1505245749-3402-1-git-send-email-azhou@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev,merge,native,tunneling,and,port,1/7] ofproto-dpif: Unfreeze within clone | expand |
On 09/12/2017 12:49 PM, Andy Zhou wrote: > When translating actions within open flow clone, actions generated > by finish_freezeing() should also be enclosed within the datapath > clone netlink encoding. > > Signed-off-by: Andy Zhou <azhou@ovn.org> Andy, I am reviewing and testing your patches. I have applied them to my private github repository on a branch named test-813027-35. https://github.com/gvrose8192/ovs-experimental/tree/test-813027-35 However, the Travis 'TESTSUITE=1 KERNEL=3.16.46 build fails: https://travis-ci.org/gvrose8192/ovs-experimental/jobs/277364409 Have you noticed this as well? The current master branch does not have the same error. Thanks, - Greg > --- > ofproto/ofproto-dpif-xlate.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c > index 9e1f837cb23e..e5ad832d7c47 100644 > --- a/ofproto/ofproto-dpif-xlate.c > +++ b/ofproto/ofproto-dpif-xlate.c > @@ -5353,6 +5353,9 @@ compose_clone(struct xlate_ctx *ctx, const struct ofpact_nest *oc) > if (reversible_actions(oc->actions, oc_actions_len)) { > old_flow = ctx->xin->flow; > do_xlate_actions(oc->actions, oc_actions_len, ctx); > + if (ctx->freezing) { > + finish_freezing(ctx); > + } > goto xlate_done; > } > > @@ -5372,6 +5375,9 @@ compose_clone(struct xlate_ctx *ctx, const struct ofpact_nest *oc) > offset = nl_msg_start_nested(ctx->odp_actions, OVS_ACTION_ATTR_CLONE); > ac_offset = ctx->odp_actions->size; > do_xlate_actions(oc->actions, oc_actions_len, ctx); > + if (ctx->freezing) { > + finish_freezing(ctx); > + } > nl_msg_end_non_empty_nested(ctx->odp_actions, offset); > goto dp_clone_done; > } > @@ -5382,6 +5388,9 @@ compose_clone(struct xlate_ctx *ctx, const struct ofpact_nest *oc) > ac_offset = nl_msg_start_nested(ctx->odp_actions, > OVS_SAMPLE_ATTR_ACTIONS); > do_xlate_actions(oc->actions, oc_actions_len, ctx); > + if (ctx->freezing) { > + finish_freezing(ctx); > + } > if (nl_msg_end_non_empty_nested(ctx->odp_actions, ac_offset)) { > nl_msg_cancel_nested(ctx->odp_actions, offset); > } else { >
On Tue, Sep 19, 2017 at 9:55 AM, Greg Rose <gvrose8192@gmail.com> wrote: > On 09/12/2017 12:49 PM, Andy Zhou wrote: >> >> When translating actions within open flow clone, actions generated >> by finish_freezeing() should also be enclosed within the datapath >> clone netlink encoding. >> >> Signed-off-by: Andy Zhou <azhou@ovn.org> > > > Andy, > > I am reviewing and testing your patches. I have applied them to my private > github repository > on a branch named test-813027-35. > > https://github.com/gvrose8192/ovs-experimental/tree/test-813027-35 > > However, the Travis 'TESTSUITE=1 KERNEL=3.16.46 build fails: > > https://travis-ci.org/gvrose8192/ovs-experimental/jobs/277364409 > > Have you noticed this as well? No. It passed my local test, and passed travis test from my private branch (just rebased this morning) https://github.com/azhou-nicira/ovs-review/tree/patch_port https://travis-ci.org/azhou-nicira/ovs-review/builds/277412765 (The --disable-ssl build is slow for some reason, same as master). May be this is caused by travis running slow for some reason? Did your local test pass?
On 09/19/2017 01:26 PM, Andy Zhou wrote: > On Tue, Sep 19, 2017 at 9:55 AM, Greg Rose <gvrose8192@gmail.com> wrote: >> On 09/12/2017 12:49 PM, Andy Zhou wrote: >>> >>> When translating actions within open flow clone, actions generated >>> by finish_freezeing() should also be enclosed within the datapath >>> clone netlink encoding. >>> >>> Signed-off-by: Andy Zhou <azhou@ovn.org> >> >> >> Andy, >> >> I am reviewing and testing your patches. I have applied them to my private >> github repository >> on a branch named test-813027-35. >> >> https://github.com/gvrose8192/ovs-experimental/tree/test-813027-35 >> >> However, the Travis 'TESTSUITE=1 KERNEL=3.16.46 build fails: >> >> https://travis-ci.org/gvrose8192/ovs-experimental/jobs/277364409 >> >> Have you noticed this as well? > > No. It passed my local test, and passed travis test from my private > branch (just rebased this morning) > > https://github.com/azhou-nicira/ovs-review/tree/patch_port > > https://travis-ci.org/azhou-nicira/ovs-review/builds/277412765 > > (The --disable-ssl build is slow for some reason, same as master). > > May be this is caused by travis running slow for some reason? > > Did your local test pass? > Yes, I just tried on a VM running Centos 7.3 with the 4.9 kernel and it passed there. /shrug? OK, I'll continue with review then. Thanks! - Greg
On Tue, Sep 19, 2017 at 1:56 PM, Greg Rose <gvrose8192@gmail.com> wrote: > On 09/19/2017 01:26 PM, Andy Zhou wrote: >> >> On Tue, Sep 19, 2017 at 9:55 AM, Greg Rose <gvrose8192@gmail.com> wrote: >>> >>> On 09/12/2017 12:49 PM, Andy Zhou wrote: >>>> >>>> >>>> When translating actions within open flow clone, actions generated >>>> by finish_freezeing() should also be enclosed within the datapath >>>> clone netlink encoding. >>>> >>>> Signed-off-by: Andy Zhou <azhou@ovn.org> >>> >>> >>> >>> Andy, >>> >>> I am reviewing and testing your patches. I have applied them to my >>> private >>> github repository >>> on a branch named test-813027-35. >>> >>> https://github.com/gvrose8192/ovs-experimental/tree/test-813027-35 >>> >>> However, the Travis 'TESTSUITE=1 KERNEL=3.16.46 build fails: >>> >>> https://travis-ci.org/gvrose8192/ovs-experimental/jobs/277364409 >>> >>> Have you noticed this as well? >> >> >> No. It passed my local test, and passed travis test from my private >> branch (just rebased this morning) >> >> https://github.com/azhou-nicira/ovs-review/tree/patch_port >> >> https://travis-ci.org/azhou-nicira/ovs-review/builds/277412765 >> >> (The --disable-ssl build is slow for some reason, same as master). >> >> May be this is caused by travis running slow for some reason? >> >> Did your local test pass? >> > > Yes, I just tried on a VM running Centos 7.3 with the 4.9 kernel and it > passed there. > > /shrug? > > OK, I'll continue with review then. > > Thanks! > > - Greg FWIW. the --disable-ssl build finally passed. The total build/test time is 4hr 42min. Travis CI is definitely slow today.
On 09/19/2017 04:11 PM, Andy Zhou wrote: > On Tue, Sep 19, 2017 at 1:56 PM, Greg Rose <gvrose8192@gmail.com> wrote: >> On 09/19/2017 01:26 PM, Andy Zhou wrote: >>> >> >> Yes, I just tried on a VM running Centos 7.3 with the 4.9 kernel and it >> passed there. >> >> /shrug? >> >> OK, I'll continue with review then. >> >> Thanks! >> >> - Greg > > FWIW. the --disable-ssl build finally passed. The total build/test > time is 4hr 42min. > Travis CI is definitely slow today. > Well, it is free. :^) Thanks, - Greg
On 09/12/2017 12:49 PM, Andy Zhou wrote: > When translating actions within open flow clone, actions generated > by finish_freezeing() should also be enclosed within the datapath > clone netlink encoding. > > Signed-off-by: Andy Zhou <azhou@ovn.org> > --- > ofproto/ofproto-dpif-xlate.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c > index 9e1f837cb23e..e5ad832d7c47 100644 > --- a/ofproto/ofproto-dpif-xlate.c > +++ b/ofproto/ofproto-dpif-xlate.c > @@ -5353,6 +5353,9 @@ compose_clone(struct xlate_ctx *ctx, const struct ofpact_nest *oc) > if (reversible_actions(oc->actions, oc_actions_len)) { > old_flow = ctx->xin->flow; > do_xlate_actions(oc->actions, oc_actions_len, ctx); > + if (ctx->freezing) { > + finish_freezing(ctx); > + } > goto xlate_done; > } > > @@ -5372,6 +5375,9 @@ compose_clone(struct xlate_ctx *ctx, const struct ofpact_nest *oc) > offset = nl_msg_start_nested(ctx->odp_actions, OVS_ACTION_ATTR_CLONE); > ac_offset = ctx->odp_actions->size; > do_xlate_actions(oc->actions, oc_actions_len, ctx); > + if (ctx->freezing) { > + finish_freezing(ctx); > + } > nl_msg_end_non_empty_nested(ctx->odp_actions, offset); > goto dp_clone_done; > } > @@ -5382,6 +5388,9 @@ compose_clone(struct xlate_ctx *ctx, const struct ofpact_nest *oc) > ac_offset = nl_msg_start_nested(ctx->odp_actions, > OVS_SAMPLE_ATTR_ACTIONS); > do_xlate_actions(oc->actions, oc_actions_len, ctx); > + if (ctx->freezing) { > + finish_freezing(ctx); > + } > if (nl_msg_end_non_empty_nested(ctx->odp_actions, ac_offset)) { > nl_msg_cancel_nested(ctx->odp_actions, offset); > } else { > Tested-by: Greg Rose <gvrose8192@gmail.com> Reviewed-by: Greg Rose <gvrose8192@gmail.com>
On Thu, Sep 21, 2017 at 9:44 AM, Greg Rose <gvrose8192@gmail.com> wrote: > On 09/12/2017 12:49 PM, Andy Zhou wrote: >> >> When translating actions within open flow clone, actions generated >> by finish_freezeing() should also be enclosed within the datapath >> clone netlink encoding. >> >> Signed-off-by: Andy Zhou <azhou@ovn.org> >> --- >> ofproto/ofproto-dpif-xlate.c | 9 +++++++++ >> 1 file changed, 9 insertions(+) >> >> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c >> index 9e1f837cb23e..e5ad832d7c47 100644 >> --- a/ofproto/ofproto-dpif-xlate.c >> +++ b/ofproto/ofproto-dpif-xlate.c >> @@ -5353,6 +5353,9 @@ compose_clone(struct xlate_ctx *ctx, const struct >> ofpact_nest *oc) >> if (reversible_actions(oc->actions, oc_actions_len)) { >> old_flow = ctx->xin->flow; >> do_xlate_actions(oc->actions, oc_actions_len, ctx); >> + if (ctx->freezing) { >> + finish_freezing(ctx); >> + } >> goto xlate_done; >> } >> @@ -5372,6 +5375,9 @@ compose_clone(struct xlate_ctx *ctx, const struct >> ofpact_nest *oc) >> offset = nl_msg_start_nested(ctx->odp_actions, >> OVS_ACTION_ATTR_CLONE); >> ac_offset = ctx->odp_actions->size; >> do_xlate_actions(oc->actions, oc_actions_len, ctx); >> + if (ctx->freezing) { >> + finish_freezing(ctx); >> + } >> nl_msg_end_non_empty_nested(ctx->odp_actions, offset); >> goto dp_clone_done; >> } >> @@ -5382,6 +5388,9 @@ compose_clone(struct xlate_ctx *ctx, const struct >> ofpact_nest *oc) >> ac_offset = nl_msg_start_nested(ctx->odp_actions, >> OVS_SAMPLE_ATTR_ACTIONS); >> do_xlate_actions(oc->actions, oc_actions_len, ctx); >> + if (ctx->freezing) { >> + finish_freezing(ctx); >> + } >> if (nl_msg_end_non_empty_nested(ctx->odp_actions, ac_offset)) { >> nl_msg_cancel_nested(ctx->odp_actions, offset); >> } else { >> > > Tested-by: Greg Rose <gvrose8192@gmail.com> > Reviewed-by: Greg Rose <gvrose8192@gmail.com> Thanks for the review Greg! I applied your suggestion on patch 6 and pushed the series to master.
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c index 9e1f837cb23e..e5ad832d7c47 100644 --- a/ofproto/ofproto-dpif-xlate.c +++ b/ofproto/ofproto-dpif-xlate.c @@ -5353,6 +5353,9 @@ compose_clone(struct xlate_ctx *ctx, const struct ofpact_nest *oc) if (reversible_actions(oc->actions, oc_actions_len)) { old_flow = ctx->xin->flow; do_xlate_actions(oc->actions, oc_actions_len, ctx); + if (ctx->freezing) { + finish_freezing(ctx); + } goto xlate_done; } @@ -5372,6 +5375,9 @@ compose_clone(struct xlate_ctx *ctx, const struct ofpact_nest *oc) offset = nl_msg_start_nested(ctx->odp_actions, OVS_ACTION_ATTR_CLONE); ac_offset = ctx->odp_actions->size; do_xlate_actions(oc->actions, oc_actions_len, ctx); + if (ctx->freezing) { + finish_freezing(ctx); + } nl_msg_end_non_empty_nested(ctx->odp_actions, offset); goto dp_clone_done; } @@ -5382,6 +5388,9 @@ compose_clone(struct xlate_ctx *ctx, const struct ofpact_nest *oc) ac_offset = nl_msg_start_nested(ctx->odp_actions, OVS_SAMPLE_ATTR_ACTIONS); do_xlate_actions(oc->actions, oc_actions_len, ctx); + if (ctx->freezing) { + finish_freezing(ctx); + } if (nl_msg_end_non_empty_nested(ctx->odp_actions, ac_offset)) { nl_msg_cancel_nested(ctx->odp_actions, offset); } else {
When translating actions within open flow clone, actions generated by finish_freezeing() should also be enclosed within the datapath clone netlink encoding. Signed-off-by: Andy Zhou <azhou@ovn.org> --- ofproto/ofproto-dpif-xlate.c | 9 +++++++++ 1 file changed, 9 insertions(+)