On Mon, Aug 15, 2022 at 6:51 PM Han Zhou <hz...@ovn.org> wrote:
>
>
> On Thu, Aug 11, 2022 at 7:20 AM Ales Musil <amu...@redhat.com> 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 <amu...@redhat.com>
> > ---
> > v4: Rebase on top of current main.
> > Address comment from Numan.
> > v5: Rebase on top of current main.
> > Adress comment from Dumitru.
> > ---
> > northd/inc-proc-northd.c | 1 +
> > northd/mac-binding-aging.c | 33 +++++++++++++++++++++++++++++++--
> > ovn-nb.xml | 8 ++++++++
> > 3 files changed, 40 insertions(+), 2 deletions(-)
> >
> > diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c
> > index fc0d9e670..54e0ad3b0 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 3859c050b..36d0a6fd7 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
>
>
Hi Han,
> Thanks Ales. I just have a concern for this delay value. 10ms seems really
> too short. If I understand correctly, the delay is to avoid too many MAC
> binding deletion at the same time triggering lots of MAC-binding learning
> and population back to SB (please correct me if I am wrong, and better add
> some comment to explain the motivation in the code, too).
>
Your understanding is right, yes we want to avoid too much pressure on SB
and controllers if we remove everything at once and repopulate it again.
> If we think about the control plane latency at scale, it is easy to have a
> latency larger than hundreds of ms or even seconds. So wait for 10ms is
> nothing and it really depends on how long it takes for the ovn-northd to
> execute an iteration. I think the whole feature is just to avoid
> MAC-binding entry increases forever, so I think deleting some entries
> delayed for seconds or even minutes is not going to impact anything. So if
> the purpose here is to avoid scaling problem, shall we set the delay at
> least longer than 5s level?
>
Actually this can still happen only once per iteration, we can raise it but
if the loop takes a few seconds to run it will be delayed by that value.
However it is actually hard to find the right balance, the fact being that
it still can happen only once per iteration, it almost feels like another
config option
would be useful, but I don't want to introduce yet another config option
blindly.
Thanks,
Ales
>
> Thanks,
> Han
>
> > +
> > struct mac_binding_waker {
> > bool should_schedule;
> > long long next_wake_msec;
> > @@ -37,7 +39,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",
> > @@ -58,6 +61,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);
> > }
> > @@ -65,6 +72,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)
> > {
> > @@ -76,6 +97,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 mac_binding_waker *waker =
> > engine_get_input_data("mac_binding_aging_waker", 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 57c7d6174..bed1a3aa1 100644
> > --- a/ovn-nb.xml
> > +++ b/ovn-nb.xml
> > @@ -162,6 +162,14 @@
> > 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. When we hit the limit next batch removal is
> delayed by
> > + 10 ms.
> > + </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
> > --
> > 2.37.1
> >
>
--
Ales Musil
Senior Software Engineer - OVN Core
Red Hat EMEA <https://www.redhat.com>
amu...@redhat.com IM: amusil
<https://red.ht/sig>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
