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")
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
--
Ales Musil
Senior Software Engineer - OVN Core
Red Hat EMEA <https://www.redhat.com>
[email protected]
<https://red.ht/sig>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
