On 8/12/24 16:03, Ales Musil wrote: > On Mon, Aug 12, 2024 at 3:05 PM Dumitru Ceara <[email protected]> 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 <[email protected]> > > >>> Signed-off-by: Ales Musil <[email protected]> >>> --- >>> 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 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
