diff mbox series

[ovs-dev,v8,1/3] northd: Disable parallel processing for logical_dp_groups

Message ID 20210915124340.1765-1-anton.ivanov@cambridgegreys.com
State Accepted
Headers show
Series [ovs-dev,v8,1/3] northd: Disable parallel processing for logical_dp_groups | expand

Checks

Context Check Description
ovsrobot/apply-robot success apply and check: success
ovsrobot/github-robot-_Build_and_Test fail github build: failed
ovsrobot/github-robot-_ovn-kubernetes fail github build: failed

Commit Message

Anton Ivanov Sept. 15, 2021, 12:43 p.m. UTC 
From: Anton Ivanov <anton.ivanov@cambridgegreys.com>

Work on improving processing with dp_groups enabled has
discovered that the locking mechanism presently in use
is not reliable. Disabling parallel processing if dp_groups
are enabled until the root cause is determined and fixed.

Signed-off-by: Anton Ivanov <anton.ivanov@cambridgegreys.com>
---
 northd/ovn-northd.c | 2 +-
 ovs                 | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

Comments

Mark Michelson Sept. 17, 2021, 5:23 p.m. UTC | #1 
Hi Anton,

For the series:

Acked-by: Mark Michelson <mmichels@redhat.com>

On 9/15/21 8:43 AM, anton.ivanov@cambridgegreys.com wrote:
> From: Anton Ivanov <anton.ivanov@cambridgegreys.com>
> 
> Work on improving processing with dp_groups enabled has
> discovered that the locking mechanism presently in use
> is not reliable. Disabling parallel processing if dp_groups
> are enabled until the root cause is determined and fixed.
> 
> Signed-off-by: Anton Ivanov <anton.ivanov@cambridgegreys.com>
> ---
>   northd/ovn-northd.c | 2 +-
>   ovs                 | 2 +-
>   2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> index baaddb73e..3113fafc7 100644
> --- a/northd/ovn-northd.c
> +++ b/northd/ovn-northd.c
> @@ -12974,7 +12974,7 @@ build_lswitch_and_lrouter_flows(struct hmap *datapaths, struct hmap *ports,
>           }
>       }
>   
> -    if (use_parallel_build) {
> +    if (use_parallel_build && (!use_logical_dp_groups)) {
>           struct hmap *lflow_segs;
>           struct lswitch_flow_build_info *lsiv;
>           int index;
> diff --git a/ovs b/ovs
> index 748010ff3..50e5523b9 160000
> --- a/ovs
> +++ b/ovs
> @@ -1 +1 @@
> -Subproject commit 748010ff304b7cd2c43f4eb98a554433f0df07f9
> +Subproject commit 50e5523b9b2b154e5fafc5acdcdec85e9cc5a330
>
Ilya Maximets Sept. 17, 2021, 5:38 p.m. UTC | #2 
On 9/15/21 14:43, anton.ivanov@cambridgegreys.com wrote:
> From: Anton Ivanov <anton.ivanov@cambridgegreys.com>
> 
> Work on improving processing with dp_groups enabled has
> discovered that the locking mechanism presently in use
> is not reliable. Disabling parallel processing if dp_groups
> are enabled until the root cause is determined and fixed.
> 
> Signed-off-by: Anton Ivanov <anton.ivanov@cambridgegreys.com>
> ---
>  northd/ovn-northd.c | 2 +-
>  ovs                 | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> index baaddb73e..3113fafc7 100644
> --- a/northd/ovn-northd.c
> +++ b/northd/ovn-northd.c
> @@ -12974,7 +12974,7 @@ build_lswitch_and_lrouter_flows(struct hmap *datapaths, struct hmap *ports,
>          }
>      }
>  
> -    if (use_parallel_build) {
> +    if (use_parallel_build && (!use_logical_dp_groups)) {
>          struct hmap *lflow_segs;
>          struct lswitch_flow_build_info *lsiv;
>          int index;
> diff --git a/ovs b/ovs
> index 748010ff3..50e5523b9 160000
> --- a/ovs
> +++ b/ovs
> @@ -1 +1 @@
> -Subproject commit 748010ff304b7cd2c43f4eb98a554433f0df07f9
> +Subproject commit 50e5523b9b2b154e5fafc5acdcdec85e9cc5a330
> 

This change breaks the build due to submodule downgrade.

Best regards, Ilya Maximets.
Anton Ivanov Sept. 17, 2021, 6:23 p.m. UTC | #3 
On 17/09/2021 18:23, Mark Michelson wrote:
> Hi Anton,
>
> For the series:
>
> Acked-by: Mark Michelson <mmichels@redhat.com>
Thanks.
>
> On 9/15/21 8:43 AM, anton.ivanov@cambridgegreys.com wrote:
>> From: Anton Ivanov <anton.ivanov@cambridgegreys.com>
>>
>> Work on improving processing with dp_groups enabled has
>> discovered that the locking mechanism presently in use
>> is not reliable. Disabling parallel processing if dp_groups
>> are enabled until the root cause is determined and fixed.
>>
>> Signed-off-by: Anton Ivanov <anton.ivanov@cambridgegreys.com>
>> ---
>>   northd/ovn-northd.c | 2 +-
>>   ovs                 | 2 +-
>>   2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
>> index baaddb73e..3113fafc7 100644
>> --- a/northd/ovn-northd.c
>> +++ b/northd/ovn-northd.c
>> @@ -12974,7 +12974,7 @@ build_lswitch_and_lrouter_flows(struct hmap 
>> *datapaths, struct hmap *ports,
>>           }
>>       }
>>   -    if (use_parallel_build) {
>> +    if (use_parallel_build && (!use_logical_dp_groups)) {
>>           struct hmap *lflow_segs;
>>           struct lswitch_flow_build_info *lsiv;
>>           int index;
>> diff --git a/ovs b/ovs
>> index 748010ff3..50e5523b9 160000

As noted by Ilia I had a submodule change from ovs filter through, so I 
will resend the series.

>> --- a/ovs
>> +++ b/ovs
>> @@ -1 +1 @@
>> -Subproject commit 748010ff304b7cd2c43f4eb98a554433f0df07f9
>> +Subproject commit 50e5523b9b2b154e5fafc5acdcdec85e9cc5a330
>>
>
>
Mark Michelson Sept. 17, 2021, 7:49 p.m. UTC | #4 
On 9/17/21 1:38 PM, Ilya Maximets wrote:
> On 9/15/21 14:43, anton.ivanov@cambridgegreys.com wrote:
>> From: Anton Ivanov <anton.ivanov@cambridgegreys.com>
>>
>> Work on improving processing with dp_groups enabled has
>> discovered that the locking mechanism presently in use
>> is not reliable. Disabling parallel processing if dp_groups
>> are enabled until the root cause is determined and fixed.
>>
>> Signed-off-by: Anton Ivanov <anton.ivanov@cambridgegreys.com>
>> ---
>>   northd/ovn-northd.c | 2 +-
>>   ovs                 | 2 +-
>>   2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
>> index baaddb73e..3113fafc7 100644
>> --- a/northd/ovn-northd.c
>> +++ b/northd/ovn-northd.c
>> @@ -12974,7 +12974,7 @@ build_lswitch_and_lrouter_flows(struct hmap *datapaths, struct hmap *ports,
>>           }
>>       }
>>   
>> -    if (use_parallel_build) {
>> +    if (use_parallel_build && (!use_logical_dp_groups)) {
>>           struct hmap *lflow_segs;
>>           struct lswitch_flow_build_info *lsiv;
>>           int index;
>> diff --git a/ovs b/ovs
>> index 748010ff3..50e5523b9 160000
>> --- a/ovs
>> +++ b/ovs
>> @@ -1 +1 @@
>> -Subproject commit 748010ff304b7cd2c43f4eb98a554433f0df07f9
>> +Subproject commit 50e5523b9b2b154e5fafc5acdcdec85e9cc5a330
>>
> 
> This change breaks the build due to submodule downgrade.
> 
> Best regards, Ilya Maximets.
> 

Thanks for the note, Ilya. I did a manual rebase (which was non-trivial 
due to the northd split) and pushed to my fork. Seems to be OK with the 
submodule downgrade fixed: 
https://github.com/putnopvut/ovn/runs/3635759940?check_suite_focus=true
Mark Michelson Sept. 17, 2021, 8:44 p.m. UTC | #5 
Based on the successful GHA run I had in my fork, I fixed the submodule 
downgrade (and fixed a couple of grammar errors in comments). I've 
pushed this to master.

On 9/17/21 3:49 PM, Mark Michelson wrote:
> On 9/17/21 1:38 PM, Ilya Maximets wrote:
>> On 9/15/21 14:43, anton.ivanov@cambridgegreys.com wrote:
>>> From: Anton Ivanov <anton.ivanov@cambridgegreys.com>
>>>
>>> Work on improving processing with dp_groups enabled has
>>> discovered that the locking mechanism presently in use
>>> is not reliable. Disabling parallel processing if dp_groups
>>> are enabled until the root cause is determined and fixed.
>>>
>>> Signed-off-by: Anton Ivanov <anton.ivanov@cambridgegreys.com>
>>> ---
>>>   northd/ovn-northd.c | 2 +-
>>>   ovs                 | 2 +-
>>>   2 files changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
>>> index baaddb73e..3113fafc7 100644
>>> --- a/northd/ovn-northd.c
>>> +++ b/northd/ovn-northd.c
>>> @@ -12974,7 +12974,7 @@ build_lswitch_and_lrouter_flows(struct hmap 
>>> *datapaths, struct hmap *ports,
>>>           }
>>>       }
>>> -    if (use_parallel_build) {
>>> +    if (use_parallel_build && (!use_logical_dp_groups)) {
>>>           struct hmap *lflow_segs;
>>>           struct lswitch_flow_build_info *lsiv;
>>>           int index;
>>> diff --git a/ovs b/ovs
>>> index 748010ff3..50e5523b9 160000
>>> --- a/ovs
>>> +++ b/ovs
>>> @@ -1 +1 @@
>>> -Subproject commit 748010ff304b7cd2c43f4eb98a554433f0df07f9
>>> +Subproject commit 50e5523b9b2b154e5fafc5acdcdec85e9cc5a330
>>>
>>
>> This change breaks the build due to submodule downgrade.
>>
>> Best regards, Ilya Maximets.
>>
> 
> Thanks for the note, Ilya. I did a manual rebase (which was non-trivial 
> due to the northd split) and pushed to my fork. Seems to be OK with the 
> submodule downgrade fixed: 
> https://github.com/putnopvut/ovn/runs/3635759940?check_suite_focus=true
Anton Ivanov Sept. 18, 2021, 11:10 a.m. UTC | #6 
On 17/09/2021 21:44, Mark Michelson wrote:
> Based on the successful GHA run I had in my fork, I fixed the 
> submodule downgrade (and fixed a couple of grammar errors in 
> comments). I've pushed this to master.

Thanks,

I will rebase the thread API improvements on top of that.

A.

>
> On 9/17/21 3:49 PM, Mark Michelson wrote:
>> On 9/17/21 1:38 PM, Ilya Maximets wrote:
>>> On 9/15/21 14:43, anton.ivanov@cambridgegreys.com wrote:
>>>> From: Anton Ivanov <anton.ivanov@cambridgegreys.com>
>>>>
>>>> Work on improving processing with dp_groups enabled has
>>>> discovered that the locking mechanism presently in use
>>>> is not reliable. Disabling parallel processing if dp_groups
>>>> are enabled until the root cause is determined and fixed.
>>>>
>>>> Signed-off-by: Anton Ivanov <anton.ivanov@cambridgegreys.com>
>>>> ---
>>>>   northd/ovn-northd.c | 2 +-
>>>>   ovs                 | 2 +-
>>>>   2 files changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
>>>> index baaddb73e..3113fafc7 100644
>>>> --- a/northd/ovn-northd.c
>>>> +++ b/northd/ovn-northd.c
>>>> @@ -12974,7 +12974,7 @@ build_lswitch_and_lrouter_flows(struct hmap 
>>>> *datapaths, struct hmap *ports,
>>>>           }
>>>>       }
>>>> -    if (use_parallel_build) {
>>>> +    if (use_parallel_build && (!use_logical_dp_groups)) {
>>>>           struct hmap *lflow_segs;
>>>>           struct lswitch_flow_build_info *lsiv;
>>>>           int index;
>>>> diff --git a/ovs b/ovs
>>>> index 748010ff3..50e5523b9 160000
>>>> --- a/ovs
>>>> +++ b/ovs
>>>> @@ -1 +1 @@
>>>> -Subproject commit 748010ff304b7cd2c43f4eb98a554433f0df07f9
>>>> +Subproject commit 50e5523b9b2b154e5fafc5acdcdec85e9cc5a330
>>>>
>>>
>>> This change breaks the build due to submodule downgrade.
>>>
>>> Best regards, Ilya Maximets.
>>>
>>
>> Thanks for the note, Ilya. I did a manual rebase (which was 
>> non-trivial due to the northd split) and pushed to my fork. Seems to 
>> be OK with the submodule downgrade fixed: 
>> https://github.com/putnopvut/ovn/runs/3635759940?check_suite_focus=true
>
>
diff mbox series

Patch

diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
index baaddb73e..3113fafc7 100644
--- a/northd/ovn-northd.c
+++ b/northd/ovn-northd.c
@@ -12974,7 +12974,7 @@  build_lswitch_and_lrouter_flows(struct hmap *datapaths, struct hmap *ports,
         }
     }
 
-    if (use_parallel_build) {
+    if (use_parallel_build && (!use_logical_dp_groups)) {
         struct hmap *lflow_segs;
         struct lswitch_flow_build_info *lsiv;
         int index;
diff --git a/ovs b/ovs
index 748010ff3..50e5523b9 160000
--- a/ovs
+++ b/ovs
@@ -1 +1 @@ 
-Subproject commit 748010ff304b7cd2c43f4eb98a554433f0df07f9
+Subproject commit 50e5523b9b2b154e5fafc5acdcdec85e9cc5a330