| Message ID | 20220720084851.60041-7-amusil@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | MAC binding aging mechanism | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
Acked-By: Ihar Hrachyshka <ihrachys@redhat.com> On Wed, Jul 20, 2022 at 4:56 AM Ales Musil <amusil@redhat.com> wrote: > > Because the transaction is limited, in terms of how > many operations it can do, we should not allow > more than 15 MAC bindings to be removed at once every > 10 ms. This has a downside that in theory we could > never finish the removal, however in practice it is > unlikely considering that not all routers will have aging > enabled and the enabled will be with reasonable threshold. > > Reported-at: https://bugzilla.redhat.com/2084668 > Signed-off-by: Ales Musil <amusil@redhat.com> > --- > v3: Rebase on top of current main. > Update according to the final conclusion. > --- > northd/mac-binding-aging.c | 19 +++++++++++++++++-- > 1 file changed, 17 insertions(+), 2 deletions(-) > > diff --git a/northd/mac-binding-aging.c b/northd/mac-binding-aging.c > index 8af477ff1..32beef38b 100644 > --- a/northd/mac-binding-aging.c > +++ b/northd/mac-binding-aging.c > @@ -28,6 +28,9 @@ > > VLOG_DEFINE_THIS_MODULE(mac_binding_aging); > > +#define MAC_BINDING_BULK_REMOVAL_LIMIT 15 > +#define MAC_BINDING_BULK_REMOVAL_DELAY_MSEC 10 > + > struct mac_binding_waker { > bool should_schedule; > long long next_wake_msec; > @@ -39,7 +42,8 @@ static void > mac_binding_aging_run_for_datapath(const struct sbrec_datapath_binding *dp, > const struct nbrec_logical_router *nbr, > struct ovsdb_idl_index *mb_by_datapath, > - int64_t now, int64_t *wake_delay) > + int64_t now, int64_t *wake_delay, > + uint8_t *removed_n) > { > uint64_t threshold = smap_get_uint(&nbr->options, > "mac_binding_age_threshold", > @@ -60,6 +64,10 @@ mac_binding_aging_run_for_datapath(const struct sbrec_datapath_binding *dp, > return; > } else if (elapsed >= threshold) { > sbrec_mac_binding_delete(mb); > + (*removed_n)++; > + if (*removed_n == MAC_BINDING_BULK_REMOVAL_LIMIT) { > + break; > + } > } else { > *wake_delay = MIN(*wake_delay, threshold - elapsed); > } > @@ -78,6 +86,7 @@ en_mac_binding_aging_run(struct engine_node *node, void *data OVS_UNUSED) > > int64_t next_expire_msec = INT64_MAX; > int64_t now = time_wall_msec(); > + uint8_t removed_n = 0; > struct northd_data *northd_data = engine_get_input_data("northd", node); > struct ovsdb_idl_index *sbrec_mac_binding_by_datapath = > engine_ovsdb_node_get_index(engine_get_input("SB_mac_binding", node), > @@ -88,7 +97,13 @@ en_mac_binding_aging_run(struct engine_node *node, void *data OVS_UNUSED) > if (od->sb && od->nbr) { > mac_binding_aging_run_for_datapath(od->sb, od->nbr, > sbrec_mac_binding_by_datapath, > - now, &next_expire_msec); > + now, &next_expire_msec, > + &removed_n); > + if (removed_n == MAC_BINDING_BULK_REMOVAL_LIMIT) { > + /* Schedule the next run after specified delay. */ > + next_expire_msec = MAC_BINDING_BULK_REMOVAL_DELAY_MSEC; > + break; > + } > } > } > > -- > 2.35.3 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On Wed, Jul 27, 2022 at 12:09 AM Ihar Hrachyshka <ihrachys@redhat.com> wrote: > > Acked-By: Ihar Hrachyshka <ihrachys@redhat.com> > > On Wed, Jul 20, 2022 at 4:56 AM Ales Musil <amusil@redhat.com> wrote: > > > > Because the transaction is limited, in terms of how > > many operations it can do, we should not allow > > more than 15 MAC bindings to be removed at once every > > 10 ms. This has a downside that in theory we could > > never finish the removal, however in practice it is > > unlikely considering that not all routers will have aging > > enabled and the enabled will be with reasonable threshold. > > > > Reported-at: https://bugzilla.redhat.com/2084668 > > Signed-off-by: Ales Musil <amusil@redhat.com> > > --- Based on the offline discussion we had, I think it's better to make this removal limit configurable. I think NB_Global.options seems to be the right place. Also, since IDL or ovsdb-server doesn't impose any limit on the number of txn items in each txn, I'd suggest to have the default value to 0 (i,e don't limit the bulk removal). CMS can set it to a desired value if required. Other than this, the series looks fine to me. Thanks Numan > > v3: Rebase on top of current main. > > Update according to the final conclusion. > > --- > > northd/mac-binding-aging.c | 19 +++++++++++++++++-- > > 1 file changed, 17 insertions(+), 2 deletions(-) > > > > diff --git a/northd/mac-binding-aging.c b/northd/mac-binding-aging.c > > index 8af477ff1..32beef38b 100644 > > --- a/northd/mac-binding-aging.c > > +++ b/northd/mac-binding-aging.c > > @@ -28,6 +28,9 @@ > > > > VLOG_DEFINE_THIS_MODULE(mac_binding_aging); > > > > +#define MAC_BINDING_BULK_REMOVAL_LIMIT 15 > > +#define MAC_BINDING_BULK_REMOVAL_DELAY_MSEC 10 > > + > > struct mac_binding_waker { > > bool should_schedule; > > long long next_wake_msec; > > @@ -39,7 +42,8 @@ static void > > mac_binding_aging_run_for_datapath(const struct sbrec_datapath_binding *dp, > > const struct nbrec_logical_router *nbr, > > struct ovsdb_idl_index *mb_by_datapath, > > - int64_t now, int64_t *wake_delay) > > + int64_t now, int64_t *wake_delay, > > + uint8_t *removed_n) > > { > > uint64_t threshold = smap_get_uint(&nbr->options, > > "mac_binding_age_threshold", > > @@ -60,6 +64,10 @@ mac_binding_aging_run_for_datapath(const struct sbrec_datapath_binding *dp, > > return; > > } else if (elapsed >= threshold) { > > sbrec_mac_binding_delete(mb); > > + (*removed_n)++; > > + if (*removed_n == MAC_BINDING_BULK_REMOVAL_LIMIT) { > > + break; > > + } > > } else { > > *wake_delay = MIN(*wake_delay, threshold - elapsed); > > } > > @@ -78,6 +86,7 @@ en_mac_binding_aging_run(struct engine_node *node, void *data OVS_UNUSED) > > > > int64_t next_expire_msec = INT64_MAX; > > int64_t now = time_wall_msec(); > > + uint8_t removed_n = 0; > > struct northd_data *northd_data = engine_get_input_data("northd", node); > > struct ovsdb_idl_index *sbrec_mac_binding_by_datapath = > > engine_ovsdb_node_get_index(engine_get_input("SB_mac_binding", node), > > @@ -88,7 +97,13 @@ en_mac_binding_aging_run(struct engine_node *node, void *data OVS_UNUSED) > > if (od->sb && od->nbr) { > > mac_binding_aging_run_for_datapath(od->sb, od->nbr, > > sbrec_mac_binding_by_datapath, > > - now, &next_expire_msec); > > + now, &next_expire_msec, > > + &removed_n); > > + if (removed_n == MAC_BINDING_BULK_REMOVAL_LIMIT) { > > + /* Schedule the next run after specified delay. */ > > + next_expire_msec = MAC_BINDING_BULK_REMOVAL_DELAY_MSEC; > > + break; > > + } > > } > > } > > > > -- > > 2.35.3 > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
diff --git a/northd/mac-binding-aging.c b/northd/mac-binding-aging.c index 8af477ff1..32beef38b 100644 --- a/northd/mac-binding-aging.c +++ b/northd/mac-binding-aging.c @@ -28,6 +28,9 @@ VLOG_DEFINE_THIS_MODULE(mac_binding_aging); +#define MAC_BINDING_BULK_REMOVAL_LIMIT 15 +#define MAC_BINDING_BULK_REMOVAL_DELAY_MSEC 10 + struct mac_binding_waker { bool should_schedule; long long next_wake_msec; @@ -39,7 +42,8 @@ static void mac_binding_aging_run_for_datapath(const struct sbrec_datapath_binding *dp, const struct nbrec_logical_router *nbr, struct ovsdb_idl_index *mb_by_datapath, - int64_t now, int64_t *wake_delay) + int64_t now, int64_t *wake_delay, + uint8_t *removed_n) { uint64_t threshold = smap_get_uint(&nbr->options, "mac_binding_age_threshold", @@ -60,6 +64,10 @@ mac_binding_aging_run_for_datapath(const struct sbrec_datapath_binding *dp, return; } else if (elapsed >= threshold) { sbrec_mac_binding_delete(mb); + (*removed_n)++; + if (*removed_n == MAC_BINDING_BULK_REMOVAL_LIMIT) { + break; + } } else { *wake_delay = MIN(*wake_delay, threshold - elapsed); } @@ -78,6 +86,7 @@ en_mac_binding_aging_run(struct engine_node *node, void *data OVS_UNUSED) int64_t next_expire_msec = INT64_MAX; int64_t now = time_wall_msec(); + uint8_t removed_n = 0; struct northd_data *northd_data = engine_get_input_data("northd", node); struct ovsdb_idl_index *sbrec_mac_binding_by_datapath = engine_ovsdb_node_get_index(engine_get_input("SB_mac_binding", node), @@ -88,7 +97,13 @@ en_mac_binding_aging_run(struct engine_node *node, void *data OVS_UNUSED) if (od->sb && od->nbr) { mac_binding_aging_run_for_datapath(od->sb, od->nbr, sbrec_mac_binding_by_datapath, - now, &next_expire_msec); + now, &next_expire_msec, + &removed_n); + if (removed_n == MAC_BINDING_BULK_REMOVAL_LIMIT) { + /* Schedule the next run after specified delay. */ + next_expire_msec = MAC_BINDING_BULK_REMOVAL_DELAY_MSEC; + break; + } } }
Because the transaction is limited, in terms of how many operations it can do, we should not allow more than 15 MAC bindings to be removed at once every 10 ms. This has a downside that in theory we could never finish the removal, however in practice it is unlikely considering that not all routers will have aging enabled and the enabled will be with reasonable threshold. Reported-at: https://bugzilla.redhat.com/2084668 Signed-off-by: Ales Musil <amusil@redhat.com> --- v3: Rebase on top of current main. Update according to the final conclusion. --- northd/mac-binding-aging.c | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-)