| Message ID | 20230815110401.91767-5-amusil@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev,v2,1/5] tests: Check proper DP and port key | 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 | success | github build: passed |
Hi Ales, I agree with this change, and for the code itself, Acked-by: Mark Michelson <mmichels@redhat.com> However, I'm curious why this is grouped with the other changes in this series since it doesn't appear to be related to fixing tests. On 8/15/23 07:04, Ales Musil wrote: > We are correctly wrapping nb_cfg over when the value is LLONG_MAX. > However, the sync code is expecting the value to be always raising, > so it would exit early as the current >= next is true for the > wrap back to 0. To detect that use delta between those two values. > The delta threshold is set to INT32_MAX as we don't expect the nb_cfg > to be updated more than that during the sync call. > > Signed-off-by: Ales Musil <amusil@redhat.com> > --- > northd/ovn-northd.c | 4 +++- > utilities/ovn-nbctl.c | 4 +++- > 2 files changed, 6 insertions(+), 2 deletions(-) > > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c > index 4fa1b039e..68d58b337 100644 > --- a/northd/ovn-northd.c > +++ b/northd/ovn-northd.c > @@ -518,7 +518,9 @@ update_sequence_numbers(int64_t loop_start_time, > chassis_priv->name); > } > > - if (chassis_priv->nb_cfg < hv_cfg) { > + /* Detect if overflows happened within the cfg update. */ > + int64_t delta = chassis_priv->nb_cfg - hv_cfg; > + if (chassis_priv->nb_cfg < hv_cfg || delta > INT32_MAX) { > hv_cfg = chassis_priv->nb_cfg; > hv_cfg_ts = chassis_priv->nb_cfg_timestamp; > } else if (chassis_priv->nb_cfg == hv_cfg && > diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c > index 7a4f6b1b3..444fbd2fe 100644 > --- a/utilities/ovn-nbctl.c > +++ b/utilities/ovn-nbctl.c > @@ -123,7 +123,9 @@ nbctl_post_execute(struct ovsdb_idl *idl, struct ovsdb_idl_txn *txn, > int64_t cur_cfg = (wait_type == NBCTL_WAIT_SB > ? nb->sb_cfg > : MIN(nb->sb_cfg, nb->hv_cfg)); > - if (cur_cfg >= next_cfg) { > + /* Detect if overflows happened within the cfg update. */ > + int64_t delta = cur_cfg - next_cfg; > + if (cur_cfg >= next_cfg && delta < INT32_MAX) { > if (print_wait_time) { > printf("Time spent on processing nb_cfg %"PRId64":\n", > next_cfg);
On Mon, Aug 21, 2023 at 8:00 PM Mark Michelson <mmichels@redhat.com> wrote: > Hi Ales, > > I agree with this change, and for the code itself, > > Acked-by: Mark Michelson <mmichels@redhat.com> > > However, I'm curious why this is grouped with the other changes in this > series since it doesn't appear to be related to fixing tests. > Hi Mark, it actually fixes the "overflow the nb_cfg value across the tables" test. > On 8/15/23 07:04, Ales Musil wrote: > > We are correctly wrapping nb_cfg over when the value is LLONG_MAX. > > However, the sync code is expecting the value to be always raising, > > so it would exit early as the current >= next is true for the > > wrap back to 0. To detect that use delta between those two values. > > The delta threshold is set to INT32_MAX as we don't expect the nb_cfg > > to be updated more than that during the sync call. > > > > Signed-off-by: Ales Musil <amusil@redhat.com> > > --- > > northd/ovn-northd.c | 4 +++- > > utilities/ovn-nbctl.c | 4 +++- > > 2 files changed, 6 insertions(+), 2 deletions(-) > > > > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c > > index 4fa1b039e..68d58b337 100644 > > --- a/northd/ovn-northd.c > > +++ b/northd/ovn-northd.c > > @@ -518,7 +518,9 @@ update_sequence_numbers(int64_t loop_start_time, > > chassis_priv->name); > > } > > > > - if (chassis_priv->nb_cfg < hv_cfg) { > > + /* Detect if overflows happened within the cfg update. */ > > + int64_t delta = chassis_priv->nb_cfg - hv_cfg; > > + if (chassis_priv->nb_cfg < hv_cfg || delta > INT32_MAX) { > > hv_cfg = chassis_priv->nb_cfg; > > hv_cfg_ts = chassis_priv->nb_cfg_timestamp; > > } else if (chassis_priv->nb_cfg == hv_cfg && > > diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c > > index 7a4f6b1b3..444fbd2fe 100644 > > --- a/utilities/ovn-nbctl.c > > +++ b/utilities/ovn-nbctl.c > > @@ -123,7 +123,9 @@ nbctl_post_execute(struct ovsdb_idl *idl, struct > ovsdb_idl_txn *txn, > > int64_t cur_cfg = (wait_type == NBCTL_WAIT_SB > > ? nb->sb_cfg > > : MIN(nb->sb_cfg, nb->hv_cfg)); > > - if (cur_cfg >= next_cfg) { > > + /* Detect if overflows happened within the cfg update. */ > > + int64_t delta = cur_cfg - next_cfg; > > + if (cur_cfg >= next_cfg && delta < INT32_MAX) { > > if (print_wait_time) { > > printf("Time spent on processing nb_cfg > %"PRId64":\n", > > next_cfg); > > Thanks, Ales
On 8/22/23 01:38, Ales Musil wrote: > > > On Mon, Aug 21, 2023 at 8:00 PM Mark Michelson <mmichels@redhat.com > <mailto:mmichels@redhat.com>> wrote: > > Hi Ales, > > I agree with this change, and for the code itself, > > Acked-by: Mark Michelson <mmichels@redhat.com > <mailto:mmichels@redhat.com>> > > However, I'm curious why this is grouped with the other changes in this > series since it doesn't appear to be related to fixing tests. > > > > Hi Mark, > > it actually fixes the "overflow the nb_cfg value across the tables" test. Thanks for the clarification, Ales! > > > On 8/15/23 07:04, Ales Musil wrote: > > We are correctly wrapping nb_cfg over when the value is LLONG_MAX. > > However, the sync code is expecting the value to be always raising, > > so it would exit early as the current >= next is true for the > > wrap back to 0. To detect that use delta between those two values. > > The delta threshold is set to INT32_MAX as we don't expect the nb_cfg > > to be updated more than that during the sync call. > > > > Signed-off-by: Ales Musil <amusil@redhat.com > <mailto:amusil@redhat.com>> > > --- > > northd/ovn-northd.c | 4 +++- > > utilities/ovn-nbctl.c | 4 +++- > > 2 files changed, 6 insertions(+), 2 deletions(-) > > > > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c > > index 4fa1b039e..68d58b337 100644 > > --- a/northd/ovn-northd.c > > +++ b/northd/ovn-northd.c > > @@ -518,7 +518,9 @@ update_sequence_numbers(int64_t loop_start_time, > > chassis_priv->name); > > } > > > > - if (chassis_priv->nb_cfg < hv_cfg) { > > + /* Detect if overflows happened within the cfg > update. */ > > + int64_t delta = chassis_priv->nb_cfg - hv_cfg; > > + if (chassis_priv->nb_cfg < hv_cfg || delta > > INT32_MAX) { > > hv_cfg = chassis_priv->nb_cfg; > > hv_cfg_ts = chassis_priv->nb_cfg_timestamp; > > } else if (chassis_priv->nb_cfg == hv_cfg && > > diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c > > index 7a4f6b1b3..444fbd2fe 100644 > > --- a/utilities/ovn-nbctl.c > > +++ b/utilities/ovn-nbctl.c > > @@ -123,7 +123,9 @@ nbctl_post_execute(struct ovsdb_idl *idl, > struct ovsdb_idl_txn *txn, > > int64_t cur_cfg = (wait_type == NBCTL_WAIT_SB > > ? nb->sb_cfg > > : MIN(nb->sb_cfg, nb->hv_cfg)); > > - if (cur_cfg >= next_cfg) { > > + /* Detect if overflows happened within the cfg > update. */ > > + int64_t delta = cur_cfg - next_cfg; > > + if (cur_cfg >= next_cfg && delta < INT32_MAX) { > > if (print_wait_time) { > > printf("Time spent on processing nb_cfg > %"PRId64":\n", > > next_cfg); > > > Thanks, > Ales > > -- > > Ales Musil > > Senior Software Engineer - OVN Core > > Red Hat EMEA <https://www.redhat.com> > > amusil@redhat.com <mailto:amusil@redhat.com> > > <https://red.ht/sig> >
diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c index 4fa1b039e..68d58b337 100644 --- a/northd/ovn-northd.c +++ b/northd/ovn-northd.c @@ -518,7 +518,9 @@ update_sequence_numbers(int64_t loop_start_time, chassis_priv->name); } - if (chassis_priv->nb_cfg < hv_cfg) { + /* Detect if overflows happened within the cfg update. */ + int64_t delta = chassis_priv->nb_cfg - hv_cfg; + if (chassis_priv->nb_cfg < hv_cfg || delta > INT32_MAX) { hv_cfg = chassis_priv->nb_cfg; hv_cfg_ts = chassis_priv->nb_cfg_timestamp; } else if (chassis_priv->nb_cfg == hv_cfg && diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c index 7a4f6b1b3..444fbd2fe 100644 --- a/utilities/ovn-nbctl.c +++ b/utilities/ovn-nbctl.c @@ -123,7 +123,9 @@ nbctl_post_execute(struct ovsdb_idl *idl, struct ovsdb_idl_txn *txn, int64_t cur_cfg = (wait_type == NBCTL_WAIT_SB ? nb->sb_cfg : MIN(nb->sb_cfg, nb->hv_cfg)); - if (cur_cfg >= next_cfg) { + /* Detect if overflows happened within the cfg update. */ + int64_t delta = cur_cfg - next_cfg; + if (cur_cfg >= next_cfg && delta < INT32_MAX) { if (print_wait_time) { printf("Time spent on processing nb_cfg %"PRId64":\n", next_cfg);
We are correctly wrapping nb_cfg over when the value is LLONG_MAX. However, the sync code is expecting the value to be always raising, so it would exit early as the current >= next is true for the wrap back to 0. To detect that use delta between those two values. The delta threshold is set to INT32_MAX as we don't expect the nb_cfg to be updated more than that during the sync call. Signed-off-by: Ales Musil <amusil@redhat.com> --- northd/ovn-northd.c | 4 +++- utilities/ovn-nbctl.c | 4 +++- 2 files changed, 6 insertions(+), 2 deletions(-)