[ovs-dev,merge,native,tunneling,and,port,1/7] ofproto-dpif: Unfreeze within clone

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
Related show

Commit Message

Andy Zhou Sept. 12, 2017, 7:49 p.m.
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(+)

Comments

Gregory Rose Sept. 19, 2017, 4:55 p.m. | #1
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 {
>
Andy Zhou Sept. 19, 2017, 8:26 p.m. | #2
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?
Gregory Rose Sept. 19, 2017, 8:56 p.m. | #3
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
Andy Zhou Sept. 19, 2017, 11:11 p.m. | #4
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.
Gregory Rose Sept. 19, 2017, 11:31 p.m. | #5
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
Gregory Rose Sept. 21, 2017, 4:44 p.m. | #6
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>
Andy Zhou Sept. 27, 2017, 5:10 p.m. | #7
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.

Patch

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 {