| Message ID | 20220809110527.669627-7-amusil@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | MAC binding aging mechanism | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | warning | apply and check: warning |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | fail | github build: failed |
Bleep bloop. Greetings Ales Musil, I am a robot and I have tried out your patch.
Thanks for your contribution.
I encountered some error that I wasn't expecting. See the details below.
checkpatch:
WARNING: Line is 84 characters long (recommended limit is 79)
#126 FILE: ovn-nb.xml:166:
type='{"type": "integer", "minInteger": 0, "maxInteger": 4294967295}'>
Lines checked: 137, Warnings: 1, Errors: 0
Please check this out. If you feel there has been an error, please email aconole@redhat.com
Thanks,
0-day Robot
On 8/9/22 13:05, Ales Musil wrote: > Add configuration option into NB global table > called "mac_binding_removal_limit" defaulting to 0. > This option allows to limit number of MAC bindings > that can be removed by the aging mechanism in a single > transaction. The 0 means that the mechanism is disabled. > If the limit is reached next removal will be delayed by > 10 ms. This option when being set 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. > v4: Rebase on top of current main. > Address comment from Numan. > --- > northd/inc-proc-northd.c | 1 + > northd/mac-binding-aging.c | 33 +++++++++++++++++++++++++++++++-- > ovn-nb.xml | 7 +++++++ > 3 files changed, 39 insertions(+), 2 deletions(-) > > diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c > index 4a3699106..cdfd39cc6 100644 > --- a/northd/inc-proc-northd.c > +++ b/northd/inc-proc-northd.c > @@ -215,6 +215,7 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, > engine_add_input(&en_northd, &en_sb_load_balancer, NULL); > engine_add_input(&en_northd, &en_sb_fdb, NULL); > engine_add_input(&en_northd, &en_sb_static_mac_binding, NULL); > + engine_add_input(&en_mac_binding_aging, &en_nb_nb_global, NULL); > engine_add_input(&en_mac_binding_aging, &en_sb_mac_binding, NULL); > engine_add_input(&en_mac_binding_aging, &en_northd, NULL); > engine_add_input(&en_mac_binding_aging, &en_mac_binding_aging_waker, NULL); > diff --git a/northd/mac-binding-aging.c b/northd/mac-binding-aging.c > index e8217e8bc..10e995f63 100644 > --- a/northd/mac-binding-aging.c > +++ b/northd/mac-binding-aging.c > @@ -28,6 +28,8 @@ > > VLOG_DEFINE_THIS_MODULE(mac_binding_aging); > > +#define MAC_BINDING_BULK_REMOVAL_DELAY_MSEC 10 > + > struct mac_binding_waker { > bool should_schedule; > long long next_wake_msec; > @@ -39,7 +41,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, > + uint32_t removal_limit, uint32_t *removed_n) > { > uint64_t threshold = smap_get_uint(&nbr->options, > "mac_binding_age_threshold", > @@ -60,6 +63,10 @@ mac_binding_aging_run_for_datapath(const struct sbrec_datapath_binding *dp, > continue; > } else if (elapsed >= threshold) { > sbrec_mac_binding_delete(mb); > + (*removed_n)++; > + if (removal_limit && *removed_n == removal_limit) { > + break; > + } > } else { > *wake_delay = MIN(*wake_delay, threshold - elapsed); > } > @@ -67,6 +74,20 @@ mac_binding_aging_run_for_datapath(const struct sbrec_datapath_binding *dp, > sbrec_mac_binding_index_destroy_row(mb_index_row); > } > > +static uint32_t > +get_removal_limit(struct engine_node *node) > +{ > + const struct nbrec_nb_global_table *nb_global_table = > + EN_OVSDB_GET(engine_get_input("NB_nb_global", node)); > + const struct nbrec_nb_global *nb = > + nbrec_nb_global_table_first(nb_global_table); > + if (!nb) { > + return 0; > + } > + > + return smap_get_uint(&nb->options, "mac_binding_removal_limit", 0); > +} > + > void > en_mac_binding_aging_run(struct engine_node *node, void *data OVS_UNUSED) > { > @@ -78,6 +99,8 @@ 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(); > + uint32_t removal_limit = get_removal_limit(node); > + uint32_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 +111,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, > + removal_limit, &removed_n);> + if (removal_limit && removed_n == removal_limit) { > + /* Schedule the next run after specified delay. */ > + next_expire_msec = MAC_BINDING_BULK_REMOVAL_DELAY_MSEC; > + break; > + } > } > } > > diff --git a/ovn-nb.xml b/ovn-nb.xml > index d3fba9bdc..83f60265f 100644 > --- a/ovn-nb.xml > +++ b/ovn-nb.xml > @@ -162,6 +162,13 @@ > dynamically assigned, e.g. <code>00:11:22</code> > </column> > > + <column name="options" key="mac_binding_removal_limit" > + type='{"type": "integer", "minInteger": 0, "maxInteger": 4294967295}'> > + MAC binding aging bulk removal limit. This limits how many rows > + can expire in a single transaction. Default value is 0 which > + is unlimited. I think we should also mention the 10msec delay if the limit is hit. Thanks, Dumitru > + </column> > + > <column name="options" key="controller_event" type='{"type": "boolean"}'> > Value set by the CMS to enable/disable ovn-controller event reporting. > Traffic into OVS can raise a 'controller' event that results in a
diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c index 4a3699106..cdfd39cc6 100644 --- a/northd/inc-proc-northd.c +++ b/northd/inc-proc-northd.c @@ -215,6 +215,7 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, engine_add_input(&en_northd, &en_sb_load_balancer, NULL); engine_add_input(&en_northd, &en_sb_fdb, NULL); engine_add_input(&en_northd, &en_sb_static_mac_binding, NULL); + engine_add_input(&en_mac_binding_aging, &en_nb_nb_global, NULL); engine_add_input(&en_mac_binding_aging, &en_sb_mac_binding, NULL); engine_add_input(&en_mac_binding_aging, &en_northd, NULL); engine_add_input(&en_mac_binding_aging, &en_mac_binding_aging_waker, NULL); diff --git a/northd/mac-binding-aging.c b/northd/mac-binding-aging.c index e8217e8bc..10e995f63 100644 --- a/northd/mac-binding-aging.c +++ b/northd/mac-binding-aging.c @@ -28,6 +28,8 @@ VLOG_DEFINE_THIS_MODULE(mac_binding_aging); +#define MAC_BINDING_BULK_REMOVAL_DELAY_MSEC 10 + struct mac_binding_waker { bool should_schedule; long long next_wake_msec; @@ -39,7 +41,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, + uint32_t removal_limit, uint32_t *removed_n) { uint64_t threshold = smap_get_uint(&nbr->options, "mac_binding_age_threshold", @@ -60,6 +63,10 @@ mac_binding_aging_run_for_datapath(const struct sbrec_datapath_binding *dp, continue; } else if (elapsed >= threshold) { sbrec_mac_binding_delete(mb); + (*removed_n)++; + if (removal_limit && *removed_n == removal_limit) { + break; + } } else { *wake_delay = MIN(*wake_delay, threshold - elapsed); } @@ -67,6 +74,20 @@ mac_binding_aging_run_for_datapath(const struct sbrec_datapath_binding *dp, sbrec_mac_binding_index_destroy_row(mb_index_row); } +static uint32_t +get_removal_limit(struct engine_node *node) +{ + const struct nbrec_nb_global_table *nb_global_table = + EN_OVSDB_GET(engine_get_input("NB_nb_global", node)); + const struct nbrec_nb_global *nb = + nbrec_nb_global_table_first(nb_global_table); + if (!nb) { + return 0; + } + + return smap_get_uint(&nb->options, "mac_binding_removal_limit", 0); +} + void en_mac_binding_aging_run(struct engine_node *node, void *data OVS_UNUSED) { @@ -78,6 +99,8 @@ 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(); + uint32_t removal_limit = get_removal_limit(node); + uint32_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 +111,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, + removal_limit, &removed_n); + if (removal_limit && removed_n == removal_limit) { + /* Schedule the next run after specified delay. */ + next_expire_msec = MAC_BINDING_BULK_REMOVAL_DELAY_MSEC; + break; + } } } diff --git a/ovn-nb.xml b/ovn-nb.xml index d3fba9bdc..83f60265f 100644 --- a/ovn-nb.xml +++ b/ovn-nb.xml @@ -162,6 +162,13 @@ dynamically assigned, e.g. <code>00:11:22</code> </column> + <column name="options" key="mac_binding_removal_limit" + type='{"type": "integer", "minInteger": 0, "maxInteger": 4294967295}'> + MAC binding aging bulk removal limit. This limits how many rows + can expire in a single transaction. Default value is 0 which + is unlimited. + </column> + <column name="options" key="controller_event" type='{"type": "boolean"}'> Value set by the CMS to enable/disable ovn-controller event reporting. Traffic into OVS can raise a 'controller' event that results in a
Add configuration option into NB global table called "mac_binding_removal_limit" defaulting to 0. This option allows to limit number of MAC bindings that can be removed by the aging mechanism in a single transaction. The 0 means that the mechanism is disabled. If the limit is reached next removal will be delayed by 10 ms. This option when being set 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. v4: Rebase on top of current main. Address comment from Numan. --- northd/inc-proc-northd.c | 1 + northd/mac-binding-aging.c | 33 +++++++++++++++++++++++++++++++-- ovn-nb.xml | 7 +++++++ 3 files changed, 39 insertions(+), 2 deletions(-)