| Message ID | 20240812053411.2492909-1-amusil@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev] controller: Make sure the meter and group tables are initialized. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | fail | github build: failed |
Hi Ales, On 8/12/24 07:34, Ales Musil wrote: > When the feature detection was done in two calls, the second call > might have returned false because flag wasn't updated. That would > prevent the tables from being updated, because at that time > ovs_feature_set_discovered() returned false. Remove the check I'm not sure I understand this part. Eventually ovs_feature_set_discovered() should return true at which point we would've set the right group/meter maximum ids, right? Or am I missing something? > for full discovery to prevent that issue, at the same time it wasn't > needed anyway, the table would be updated only if the maximum number > of groups/meters would change during the update. > I agree with this part so the change itself seems OK. But I would still like to better understand the case in which the original code didn't work. Regards, Dumitru > Signed-off-by: Ales Musil <amusil@redhat.com> > --- > controller/ovn-controller.c | 19 ++++++++----------- > 1 file changed, 8 insertions(+), 11 deletions(-) > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > index 27a4996a8..854d80bdf 100644 > --- a/controller/ovn-controller.c > +++ b/controller/ovn-controller.c > @@ -5501,17 +5501,14 @@ main(int argc, char *argv[]) > br_int_remote.probe_interval)) { > VLOG_INFO("OVS feature set changed, force recompute."); > engine_set_force_recompute(true); > - if (ovs_feature_set_discovered()) { > - uint32_t max_groups = ovs_feature_max_select_groups_get(); > - uint32_t max_meters = ovs_feature_max_meters_get(); > - struct ed_type_lflow_output *lflow_out_data = > - engine_get_internal_data(&en_lflow_output); > - > - ovn_extend_table_reinit(&lflow_out_data->group_table, > - max_groups); > - ovn_extend_table_reinit(&lflow_out_data->meter_table, > - max_meters); > - } > + > + struct ed_type_lflow_output *lflow_out_data = > + engine_get_internal_data(&en_lflow_output); > + > + ovn_extend_table_reinit(&lflow_out_data->group_table, > + ovs_feature_max_select_groups_get()); > + ovn_extend_table_reinit(&lflow_out_data->meter_table, > + ovs_feature_max_meters_get()); > } > > if (br_int) {
On Mon, Aug 12, 2024 at 3:05 PM Dumitru Ceara <dceara@redhat.com> wrote: > Hi Ales, > > On 8/12/24 07:34, Ales Musil wrote: > > When the feature detection was done in two calls, the second call > > might have returned false because flag wasn't updated. That would > > prevent the tables from being updated, because at that time > > ovs_feature_set_discovered() returned false. Remove the check > > I'm not sure I understand this part. Eventually > ovs_feature_set_discovered() should return true at which point we > would've set the right group/meter maximum ids, right? Or am I missing > something? > > > for full discovery to prevent that issue, at the same time it wasn't > > needed anyway, the table would be updated only if the maximum number > > of groups/meters would change during the update. > > > > I agree with this part so the change itself seems OK. But I would still > like to better understand the case in which the original code didn't work. > > Regards, > Dumitru > Hi Dumitru, thank you for the review. How about updating the commit message into one below? Does that explain better what is happening? controller: Make sure the meter and group tables are initialized. The following sequence of events could cause that the groups/meters tables are not properly initialized: 1) Main loop runs and calls ovs_feature_support_run() 2) ovs_feature_support_run() returns true 3) ovs_feature_set_discovered() returns false, because not every OpenFlow feature was discovered yet. 4) The inner loop for initialization doesn't run. 5) Main loop runs second time calling ovs_feature_support_run() 6) ovs_feature_support_run() returns false, because there wasn't any feature state update. 7) ovs_feature_set_discovered() would return true, because every OpenFlow feature was discovered with this run. 8) The inner if is never executed and the groups/meters are not initialized. Remove the check for full discovery to prevent that issue, at the same time it wasn't needed anyway, the table would be updated only if the maximum number of groups/meters would change during the update. Fixes: 8c165db6f7d7 ("controller: fix group_table and meter_table allocation") Signed-off-by: Ales Musil <amusil@redhat.com> > > Signed-off-by: Ales Musil <amusil@redhat.com> > > --- > > controller/ovn-controller.c | 19 ++++++++----------- > > 1 file changed, 8 insertions(+), 11 deletions(-) > > > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > > index 27a4996a8..854d80bdf 100644 > > --- a/controller/ovn-controller.c > > +++ b/controller/ovn-controller.c > > @@ -5501,17 +5501,14 @@ main(int argc, char *argv[]) > > > br_int_remote.probe_interval)) { > > VLOG_INFO("OVS feature set changed, force recompute."); > > engine_set_force_recompute(true); > > - if (ovs_feature_set_discovered()) { > > - uint32_t max_groups = > ovs_feature_max_select_groups_get(); > > - uint32_t max_meters = ovs_feature_max_meters_get(); > > - struct ed_type_lflow_output *lflow_out_data = > > - engine_get_internal_data(&en_lflow_output); > > - > > - > ovn_extend_table_reinit(&lflow_out_data->group_table, > > - max_groups); > > - > ovn_extend_table_reinit(&lflow_out_data->meter_table, > > - max_meters); > > - } > > + > > + struct ed_type_lflow_output *lflow_out_data = > > + engine_get_internal_data(&en_lflow_output); > > + > > + ovn_extend_table_reinit(&lflow_out_data->group_table, > > + > ovs_feature_max_select_groups_get()); > > + ovn_extend_table_reinit(&lflow_out_data->meter_table, > > + ovs_feature_max_meters_get()); > > } > > > > if (br_int) { > > Thanks, Ales
On 8/12/24 16:03, Ales Musil wrote: > On Mon, Aug 12, 2024 at 3:05 PM Dumitru Ceara <dceara@redhat.com> wrote: > >> Hi Ales, >> >> On 8/12/24 07:34, Ales Musil wrote: >>> When the feature detection was done in two calls, the second call >>> might have returned false because flag wasn't updated. That would >>> prevent the tables from being updated, because at that time >>> ovs_feature_set_discovered() returned false. Remove the check >> >> I'm not sure I understand this part. Eventually >> ovs_feature_set_discovered() should return true at which point we >> would've set the right group/meter maximum ids, right? Or am I missing >> something? >> >>> for full discovery to prevent that issue, at the same time it wasn't >>> needed anyway, the table would be updated only if the maximum number >>> of groups/meters would change during the update. >>> >> >> I agree with this part so the change itself seems OK. But I would still >> like to better understand the case in which the original code didn't work. >> >> Regards, >> Dumitru >> > > > Hi Dumitru, > > thank you for the review. > > How about updating the commit message into one below? Does that explain > better what is happening? > > controller: Make sure the meter and group tables are initialized. > > The following sequence of events could cause that the groups/meters > tables are not properly initialized: > 1) Main loop runs and calls ovs_feature_support_run() > 2) ovs_feature_support_run() returns true > 3) ovs_feature_set_discovered() returns false, because not every > OpenFlow feature was discovered yet. > 4) The inner loop for initialization doesn't run. > 5) Main loop runs second time calling ovs_feature_support_run() > 6) ovs_feature_support_run() returns false, because there wasn't > any feature state update. > 7) ovs_feature_set_discovered() would return true, because every > OpenFlow feature was discovered with this run. > 8) The inner if is never executed and the groups/meters > are not initialized. > > Remove the check for full discovery to prevent that issue, at the > same time it wasn't needed anyway, the table would be updated only > if the maximum number of groups/meters would change during the update. > > Fixes: 8c165db6f7d7 ("controller: fix group_table and meter_table allocation") Sounds good, I amended the commit log as above and applied the patch to main and all stable branches down to 23.06. Thanks, Dumitru > Signed-off-by: Ales Musil <amusil@redhat.com> > > >>> Signed-off-by: Ales Musil <amusil@redhat.com> >>> --- >>> controller/ovn-controller.c | 19 ++++++++----------- >>> 1 file changed, 8 insertions(+), 11 deletions(-) >>> >>> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c >>> index 27a4996a8..854d80bdf 100644 >>> --- a/controller/ovn-controller.c >>> +++ b/controller/ovn-controller.c >>> @@ -5501,17 +5501,14 @@ main(int argc, char *argv[]) >>> >> br_int_remote.probe_interval)) { >>> VLOG_INFO("OVS feature set changed, force recompute."); >>> engine_set_force_recompute(true); >>> - if (ovs_feature_set_discovered()) { >>> - uint32_t max_groups = >> ovs_feature_max_select_groups_get(); >>> - uint32_t max_meters = ovs_feature_max_meters_get(); >>> - struct ed_type_lflow_output *lflow_out_data = >>> - engine_get_internal_data(&en_lflow_output); >>> - >>> - >> ovn_extend_table_reinit(&lflow_out_data->group_table, >>> - max_groups); >>> - >> ovn_extend_table_reinit(&lflow_out_data->meter_table, >>> - max_meters); >>> - } >>> + >>> + struct ed_type_lflow_output *lflow_out_data = >>> + engine_get_internal_data(&en_lflow_output); >>> + >>> + ovn_extend_table_reinit(&lflow_out_data->group_table, >>> + >> ovs_feature_max_select_groups_get()); >>> + ovn_extend_table_reinit(&lflow_out_data->meter_table, >>> + ovs_feature_max_meters_get()); >>> } >>> >>> if (br_int) { >> >> > Thanks, > Ales
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c index 27a4996a8..854d80bdf 100644 --- a/controller/ovn-controller.c +++ b/controller/ovn-controller.c @@ -5501,17 +5501,14 @@ main(int argc, char *argv[]) br_int_remote.probe_interval)) { VLOG_INFO("OVS feature set changed, force recompute."); engine_set_force_recompute(true); - if (ovs_feature_set_discovered()) { - uint32_t max_groups = ovs_feature_max_select_groups_get(); - uint32_t max_meters = ovs_feature_max_meters_get(); - struct ed_type_lflow_output *lflow_out_data = - engine_get_internal_data(&en_lflow_output); - - ovn_extend_table_reinit(&lflow_out_data->group_table, - max_groups); - ovn_extend_table_reinit(&lflow_out_data->meter_table, - max_meters); - } + + struct ed_type_lflow_output *lflow_out_data = + engine_get_internal_data(&en_lflow_output); + + ovn_extend_table_reinit(&lflow_out_data->group_table, + ovs_feature_max_select_groups_get()); + ovn_extend_table_reinit(&lflow_out_data->meter_table, + ovs_feature_max_meters_get()); } if (br_int) {
When the feature detection was done in two calls, the second call might have returned false because flag wasn't updated. That would prevent the tables from being updated, because at that time ovs_feature_set_discovered() returned false. Remove the check for full discovery to prevent that issue, at the same time it wasn't needed anyway, the table would be updated only if the maximum number of groups/meters would change during the update. Signed-off-by: Ales Musil <amusil@redhat.com> --- controller/ovn-controller.c | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-)