"dev" <dev-boun...@openvswitch.org> wrote on 08/06/2016 08:38:27 AM:
> From: Ryan Moats/Omaha/IBM@IBMUS
> To: Hui Kang <hkang.sun...@gmail.com>
> Cc: dev@openvswitch.org
> Date: 08/06/2016 08:38 AM
> Subject: Re: [ovs-dev] [PATCH, v5] Scanning only changed entries in the
ovnsb
> Sent by: "dev" <dev-boun...@openvswitch.org>
>
>
>
>
> "dev" <dev-boun...@openvswitch.org> wrote on 07/27/2016 08:11:11 PM:
>
> > From: Hui Kang <hkang.sun...@gmail.com>
> > To: dev@openvswitch.org
> > Date: 07/27/2016 08:11 PM
> > Subject: [ovs-dev] [PATCH, v5] Scanning only changed entries in the
ovnsb
> > Sent by: "dev" <dev-boun...@openvswitch.org>
> >
> > - Improve performance by scanning only changed port binding entries
> > when determining whether to mark the logical switch port up or
> > down
> >
> > ---
> > v4->v5:
> > - rebase
> >
> > v3->v4:
> > - Add an initialization function to scan all entries in Port_binding
> table
> > when ovn-northd restarts or fails over
> >
> > Signed-off-by: Hui Kang <ka...@us.ibm.com>
> > ---
>
>
> While git am doesn't apply this patch cleanly, patch -p1 appears to
> and all the functional tests pass.
>
> There are a couple of places I would change things, shown in line
> below...
>
> > ovn/northd/ovn-northd.c | 100 +++++++++++++++++++++++++++++++
> > +----------------
> > 1 file changed, 68 insertions(+), 32 deletions(-)
> >
> > diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
> > index 716f123..32c14a2 100644
> > --- a/ovn/northd/ovn-northd.c
> > +++ b/ovn/northd/ovn-northd.c
> > @@ -53,6 +53,11 @@ struct northd_context {
> > struct ovsdb_idl_txn *ovnsb_txn;
> > };
> >
> > +struct lport_hash_node {
> > + struct hmap_node node;
> > + const struct nbrec_logical_switch_port *nbsp;
> > +};
> > +
> > static const char *ovnnb_db;
> > static const char *ovnsb_db;
> >
> > @@ -3206,20 +3211,51 @@ ovnnb_db_run(struct northd_context *ctx,
> > struct ovsdb_idl_loop *sb_loop)
> > }
> > }
> >
> > +static void
> > +sb_chassis_update_nbsec(const struct sbrec_port_binding *sb,
> > + struct hmap *lports_hmap)
> > +
>
> Style: the blank line can be removed
>
> > +{
> > + struct lport_hash_node *hash_node;
> > + const struct nbrec_logical_switch_port *nbsp;
> > +
> > + nbsp = NULL;
> > + HMAP_FOR_EACH_WITH_HASH(hash_node, node,
> > + hash_string(sb->logical_port, 0),
> > + lports_hmap) {
> > + if (!strcmp(sb->logical_port, hash_node->nbsp->name)) {
> > + nbsp = hash_node->nbsp;
> > + break;
> > + }
> > + }
> > +
> > + if (!nbsp) {
> > + /* The logical port doesn't exist for this port binding. This
> can
> > + * happen under normal circumstances when ovn-northd hasn't
> gotten
> > + * around to pruning the Port_Binding yet. */
> > + return;
> > + }
> > +
> > + if (sb->chassis && (!nbsp->up || !*nbsp->up)) {
> > + bool up = true;
> > + nbrec_logical_switch_port_set_up(nbsp, &up, 1);
> > + } else if (!sb->chassis && (!nbsp->up || *nbsp->up)) {
> > + bool up = false;
> > + nbrec_logical_switch_port_set_up(nbsp, &up, 1);
> > + }
> > +}
> > +
> > /* Handle changes to the 'chassis' column of the 'Port_Binding' table.
> When
> > * this column is not empty, it means we need to set the
> > corresponding logical
> > * port as 'up' in the northbound DB. */
> > static void
> > -update_logical_port_status(struct northd_context *ctx)
> > +update_logical_port_status(struct northd_context *ctx, bool is_init)
> > {
> > struct hmap lports_hmap;
> > const struct sbrec_port_binding *sb;
> > const struct nbrec_logical_switch_port *nbsp;
> >
> > - struct lport_hash_node {
> > - struct hmap_node node;
> > - const struct nbrec_logical_switch_port *nbsp;
> > - } *hash_node;
> > + struct lport_hash_node *hash_node;
> >
> > hmap_init(&lports_hmap);
> >
> > @@ -3229,30 +3265,13 @@ update_logical_port_status(struct
northd_context
> *ctx)
> > hmap_insert(&lports_hmap, &hash_node->node,
> > hash_string(nbsp->name, 0));
> > }
> >
> > - SBREC_PORT_BINDING_FOR_EACH(sb, ctx->ovnsb_idl) {
> > - nbsp = NULL;
> > - HMAP_FOR_EACH_WITH_HASH(hash_node, node,
> > - hash_string(sb->logical_port, 0),
> > - &lports_hmap) {
> > - if (!strcmp(sb->logical_port, hash_node->nbsp->name)) {
> > - nbsp = hash_node->nbsp;
> > - break;
> > - }
> > - }
> > -
> > - if (!nbsp) {
> > - /* The logical port doesn't exist for this port
> > binding. This can
> > - * happen under normal circumstances when ovn-northd
> > hasn't gotten
> > - * around to pruning the Port_Binding yet. */
> > - continue;
> > + if (is_init) {
> > + SBREC_PORT_BINDING_FOR_EACH(sb, ctx->ovnsb_idl) {
> > + sb_chassis_update_nbsec(sb, &lports_hmap);
> > }
> > -
> > - if (sb->chassis && (!nbsp->up || !*nbsp->up)) {
> > - bool up = true;
> > - nbrec_logical_switch_port_set_up(nbsp, &up, 1);
> > - } else if (!sb->chassis && (!nbsp->up || *nbsp->up)) {
> > - bool up = false;
> > - nbrec_logical_switch_port_set_up(nbsp, &up, 1);
> > + } else {
> > + SBREC_PORT_BINDING_FOR_EACH_TRACKED(sb, ctx->ovnsb_idl) {
> > + sb_chassis_update_nbsec(sb, &lports_hmap);
> > }
> > }
> >
> > @@ -3354,13 +3373,14 @@ update_northbound_cfg(struct northd_context
*ctx,
> >
> > /* Handle a fairly small set of changes in the southbound database. */
> > static void
> > -ovnsb_db_run(struct northd_context *ctx, struct ovsdb_idl_loop
*sb_loop)
> > +ovnsb_db_run(struct northd_context *ctx, struct ovsdb_idl_loop
*sb_loop,
> > + bool is_init)
> > {
> > if (!ctx->ovnnb_txn || !ovsdb_idl_has_ever_connected(ctx->
> ovnsb_idl)) {
> > return;
> > }
> >
> > - update_logical_port_status(ctx);
> > + update_logical_port_status(ctx, is_init);
> > update_northbound_cfg(ctx, sb_loop);
> > }
> >
> > @@ -3463,6 +3483,19 @@ add_column_noalert(struct ovsdb_idl *idl,
> > ovsdb_idl_omit_alert(idl, column);
> > }
> >
> > +static void
> > +ovnsb_db_run_init(struct ovsdb_idl_loop *ovnsb_idl_loop)
> > +{
> > + struct northd_context ctx = {
> > + .ovnnb_idl = NULL,
> > + .ovnnb_txn = NULL,
> > + .ovnsb_idl = ovnsb_idl_loop->idl,
> > + .ovnsb_txn = ovsdb_idl_loop_run(ovnsb_idl_loop),
> > + };
> > +
> > + ovnsb_db_run(&ctx, ovnsb_idl_loop, true);
> > +}
> > +
> > int
> > main(int argc, char *argv[])
> > {
> > @@ -3537,7 +3570,6 @@ main(int argc, char *argv[])
> > add_column_noalert(ovnsb_idl_loop.idl,
> &sbrec_port_binding_col_type);
> > add_column_noalert(ovnsb_idl_loop.idl,
> &sbrec_port_binding_col_options);
> > add_column_noalert(ovnsb_idl_loop.idl,
&sbrec_port_binding_col_mac);
> > - ovsdb_idl_add_column(ovnsb_idl_loop.idl,
> > &sbrec_port_binding_col_chassis);
> > ovsdb_idl_add_table(ovnsb_idl_loop.idl,
&sbrec_table_dhcp_options);
> > add_column_noalert(ovnsb_idl_loop.idl,
> &sbrec_dhcp_options_col_code);
> > add_column_noalert(ovnsb_idl_loop.idl,
> &sbrec_dhcp_options_col_type);
> > @@ -3550,6 +3582,9 @@ main(int argc, char *argv[])
> > ovsdb_idl_add_table(ovnsb_idl_loop.idl, &sbrec_table_chassis);
> > ovsdb_idl_add_column(ovnsb_idl_loop.idl,
&sbrec_chassis_col_nb_cfg);
> >
> > + ovnsb_db_run_init(&ovnsb_idl_loop);
> > + ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
> > &sbrec_port_binding_col_chassis);
>
> The above order looks a little odd to me, I would have expected them
> to be reversed...
>
> > +
> > /* Main loop. */
> > exiting = false;
> > while (!exiting) {
> > @@ -3561,7 +3596,7 @@ main(int argc, char *argv[])
> > };
> >
> > ovnnb_db_run(&ctx, &ovnsb_idl_loop);
> > - ovnsb_db_run(&ctx, &ovnsb_idl_loop);
> > + ovnsb_db_run(&ctx, &ovnsb_idl_loop, false);
> > if (ctx.ovnsb_txn) {
> > check_and_add_supported_dhcp_opts_to_sb_db(&ctx);
> > }
> > @@ -3573,6 +3608,7 @@ main(int argc, char *argv[])
> > }
> > ovsdb_idl_loop_commit_and_wait(&ovnnb_idl_loop);
> > ovsdb_idl_loop_commit_and_wait(&ovnsb_idl_loop);
> > + ovsdb_idl_track_clear(ctx.ovnsb_idl);
> >
> > poll_block();
> > if (should_service_stop()) {
> > --
>
> Before I give it an ack, I'm curious - what kind of improvement
> does this patch add?
Thanks for your comments; I will modify it later. The purpose of
this patch is to avoid unnecessary CPU cycles spent on scanning
all entries in sb_port_binding (SBREC_PORT_BINDING_FOR_EACH).
Therefore I change this to SBREC_PORT_BINDING_FOR_EACH_TRACKED.
- Hui
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
