diff mbox series

[ovs-dev,1/2] nbctl: Cache to which switch or router particular port belongs.

Message ID 20201208125617.1244626-2-i.maximets@ovn.org
State Changes Requested
Headers show
Series nbctl: Use mutations to update sets. | expand

Commit Message

Ilya Maximets Dec. 8, 2020, 12:56 p.m. UTC 
nbctl always iterates over all ports in all logical switches or routers
to find to which logical router/switch current port belongs.  This
could be optimized by iterating only once and caching the current
state.  This should improve a little bit performance of this utility
in case where many updates are passed in a single nbctl command.

However, this change alone will slightly reduce performance of
standalone commands, since we're iterating twice over ports on port
deletion.

Cache is required for the upcoming change that will make nbctl to use
partial set updates.  It will allow us to drop redundant iterations
over ports, i.e. to not duplicate work.

Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
---
 utilities/ovn-nbctl.c | 209 +++++++++++++++++++++++++++++-------------
 1 file changed, 146 insertions(+), 63 deletions(-)

Comments

Dumitru Ceara Dec. 9, 2020, 8:33 p.m. UTC | #1 
On 12/8/20 1:56 PM, Ilya Maximets wrote:
> nbctl always iterates over all ports in all logical switches or routers
> to find to which logical router/switch current port belongs.  This
> could be optimized by iterating only once and caching the current
> state.  This should improve a little bit performance of this utility
> in case where many updates are passed in a single nbctl command.
> 
> However, this change alone will slightly reduce performance of
> standalone commands, since we're iterating twice over ports on port
> deletion.
> 
> Cache is required for the upcoming change that will make nbctl to use
> partial set updates.  It will allow us to drop redundant iterations
> over ports, i.e. to not duplicate work.
> 
> Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
> ---
>  utilities/ovn-nbctl.c | 209 +++++++++++++++++++++++++++++-------------
>  1 file changed, 146 insertions(+), 63 deletions(-)
> 
> diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c
> index d19e1b6c6..3886da889 100644
> --- a/utilities/ovn-nbctl.c
> +++ b/utilities/ovn-nbctl.c
> @@ -125,6 +125,61 @@ static char * OVS_WARN_UNUSED_RESULT main_loop(const char *args,
>                                                 const struct timer *);
>  static void server_loop(struct ovsdb_idl *idl, int argc, char *argv[]);
>  
> +/* A context for keeping track of which switch/router certain ports are
> + * connected to. */
> +struct nbctl_context {
> +    struct ctl_context base;
> +    struct shash lsp_to_ls;
> +    struct shash lrp_to_lr;

Hi Ilya,

Tiny nit: There are already functions called lsp_to_ls/lrp_to_lr. Not a
big deal but it might create a bit of confusion. What about using
lsp_to_ls_map/lrp_to_lr_map?

The rest looks good to me, therefore:

Acked-by: Dumitru Ceara <dceara@redhat.com>

Thanks,
Dumitru
diff mbox series

Patch

diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c
index d19e1b6c6..3886da889 100644
--- a/utilities/ovn-nbctl.c
+++ b/utilities/ovn-nbctl.c
@@ -125,6 +125,61 @@  static char * OVS_WARN_UNUSED_RESULT main_loop(const char *args,
                                                const struct timer *);
 static void server_loop(struct ovsdb_idl *idl, int argc, char *argv[]);
 
+/* A context for keeping track of which switch/router certain ports are
+ * connected to. */
+struct nbctl_context {
+    struct ctl_context base;
+    struct shash lsp_to_ls;
+    struct shash lrp_to_lr;
+    bool context_valid;
+};
+
+static void
+nbctl_context_init(struct nbctl_context *nbctx)
+{
+    nbctx->context_valid = false;
+    shash_init(&nbctx->lsp_to_ls);
+    shash_init(&nbctx->lrp_to_lr);
+}
+
+static void
+nbctl_context_destroy(struct nbctl_context *nbctx)
+{
+    nbctx->context_valid = false;
+    shash_destroy(&nbctx->lsp_to_ls);
+    shash_destroy(&nbctx->lrp_to_lr);
+}
+
+/* Casts 'base' into 'struct nbctl_context' and initializes it if needed. */
+static struct nbctl_context *
+nbctl_context_get(struct ctl_context *base)
+{
+    struct nbctl_context *nbctx;
+
+    nbctx = CONTAINER_OF(base, struct nbctl_context, base);
+
+    if (nbctx->context_valid) {
+        return nbctx;
+    }
+
+    const struct nbrec_logical_switch *ls;
+    NBREC_LOGICAL_SWITCH_FOR_EACH (ls, base->idl) {
+        for (size_t i = 0; i < ls->n_ports; i++) {
+            shash_add_once(&nbctx->lsp_to_ls, ls->ports[i]->name, ls);
+        }
+    }
+
+    const struct nbrec_logical_router *lr;
+    NBREC_LOGICAL_ROUTER_FOR_EACH (lr, base->idl) {
+        for (size_t i = 0; i < lr->n_ports; i++) {
+            shash_add_once(&nbctx->lrp_to_lr, lr->ports[i]->name, lr);
+        }
+    }
+
+    nbctx->context_valid = true;
+    return nbctx;
+}
+
 int
 main(int argc, char *argv[])
 {
@@ -1249,6 +1304,7 @@  static void
 nbctl_ls_del(struct ctl_context *ctx)
 {
     bool must_exist = !shash_find(&ctx->options, "--if-exists");
+    struct nbctl_context *nbctx = nbctl_context_get(ctx);
     const char *id = ctx->argv[1];
     const struct nbrec_logical_switch *ls = NULL;
 
@@ -1261,6 +1317,11 @@  nbctl_ls_del(struct ctl_context *ctx)
         return;
     }
 
+    /* Updating runtime cache. */
+    for (size_t i = 0; i < ls->n_ports; i++) {
+        shash_find_and_delete(&nbctx->lsp_to_ls, ls->ports[i]->name);
+    }
+
     nbrec_logical_switch_delete(ls);
 }
 
@@ -1317,22 +1378,19 @@  lsp_by_name_or_uuid(struct ctl_context *ctx, const char *id,
 
 /* Returns the logical switch that contains 'lsp'. */
 static char * OVS_WARN_UNUSED_RESULT
-lsp_to_ls(const struct ovsdb_idl *idl,
+lsp_to_ls(struct ctl_context *ctx,
           const struct nbrec_logical_switch_port *lsp,
           const struct nbrec_logical_switch **ls_p)
 {
+    struct nbctl_context *nbctx = nbctl_context_get(ctx);
     const struct nbrec_logical_switch *ls;
     *ls_p = NULL;
 
-    NBREC_LOGICAL_SWITCH_FOR_EACH (ls, idl) {
-        for (size_t i = 0; i < ls->n_ports; i++) {
-            if (ls->ports[i] == lsp) {
-                *ls_p = ls;
-                return NULL;
-            }
-        }
+    ls = shash_find_data(&nbctx->lsp_to_ls, lsp->name);
+    if (ls) {
+        *ls_p = ls;
+        return NULL;
     }
-
     /* Can't happen because of the database schema */
     return xasprintf("logical port %s is not part of any logical switch",
                      lsp->name);
@@ -1353,6 +1411,7 @@  static void
 nbctl_lsp_add(struct ctl_context *ctx)
 {
     bool may_exist = shash_find(&ctx->options, "--may-exist") != NULL;
+    struct nbctl_context *nbctx = nbctl_context_get(ctx);
 
     const struct nbrec_logical_switch *ls = NULL;
     char *error = ls_by_name_or_uuid(ctx, ctx->argv[1], true, &ls);
@@ -1395,7 +1454,7 @@  nbctl_lsp_add(struct ctl_context *ctx)
         }
 
         const struct nbrec_logical_switch *lsw;
-        error = lsp_to_ls(ctx->idl, lsp, &lsw);
+        error = lsp_to_ls(ctx, lsp, &lsw);
         if (error) {
             ctx->error = error;
             return;
@@ -1456,13 +1515,21 @@  nbctl_lsp_add(struct ctl_context *ctx)
                                              lsp);
     nbrec_logical_switch_set_ports(ls, new_ports, ls->n_ports + 1);
     free(new_ports);
+
+    /* Updating runtime cache. */
+    shash_add(&nbctx->lsp_to_ls, lsp_name, ls);
 }
 
 /* Removes logical switch port 'ls->ports[idx]'. */
 static void
-remove_lsp(const struct nbrec_logical_switch *ls, size_t idx)
+remove_lsp(struct ctl_context *ctx, size_t idx,
+           const struct nbrec_logical_switch *ls,
+           const struct nbrec_logical_switch_port *lsp)
 {
-    const struct nbrec_logical_switch_port *lsp = ls->ports[idx];
+    struct nbctl_context *nbctx = nbctl_context_get(ctx);
+
+    /* Updating runtime cache. */
+    shash_find_and_delete(&nbctx->lsp_to_ls, lsp->name);
 
     /* First remove 'lsp' from the array of ports.  This is what will
      * actually cause the logical port to be deleted when the transaction is
@@ -1498,18 +1565,18 @@  nbctl_lsp_del(struct ctl_context *ctx)
 
     /* Find the switch that contains 'lsp', then delete it. */
     const struct nbrec_logical_switch *ls;
-    NBREC_LOGICAL_SWITCH_FOR_EACH (ls, ctx->idl) {
-        for (size_t i = 0; i < ls->n_ports; i++) {
-            if (ls->ports[i] == lsp) {
-                remove_lsp(ls, i);
-                return;
-            }
+
+    error = lsp_to_ls(ctx, lsp, &ls);
+    if (error) {
+        ctx->error = error;
+        return;
+    }
+    for (size_t i = 0; i < ls->n_ports; i++) {
+        if (ls->ports[i] == lsp) {
+            remove_lsp(ctx, i, ls, lsp);
+            break;
         }
     }
-
-    /* Can't happen because of the database schema. */
-    ctl_error(ctx, "logical port %s is not part of any logical switch",
-              ctx->argv[1]);
 }
 
 static void
@@ -1658,7 +1725,7 @@  nbctl_lsp_set_addresses(struct ctl_context *ctx)
     }
 
     const struct nbrec_logical_switch *ls;
-    error = lsp_to_ls(ctx->idl, lsp, &ls);
+    error = lsp_to_ls(ctx, lsp, &ls);
     if (error) {
         ctx->error = error;
         return;
@@ -3378,6 +3445,7 @@  static void
 nbctl_lr_del(struct ctl_context *ctx)
 {
     bool must_exist = !shash_find(&ctx->options, "--if-exists");
+    struct nbctl_context *nbctx = nbctl_context_get(ctx);
     const char *id = ctx->argv[1];
     const struct nbrec_logical_router *lr = NULL;
 
@@ -3390,6 +3458,11 @@  nbctl_lr_del(struct ctl_context *ctx)
         return;
     }
 
+    /* Updating runtime cache. */
+    for (size_t i = 0; i < lr->n_ports; i++) {
+        shash_find_and_delete(&nbctx->lrp_to_lr, lr->ports[i]->name);
+    }
+
     nbrec_logical_router_delete(lr);
 }
 
@@ -4667,20 +4740,18 @@  lrp_by_name_or_uuid(struct ctl_context *ctx, const char *id, bool must_exist,
 
 /* Returns the logical router that contains 'lrp'. */
 static char * OVS_WARN_UNUSED_RESULT
-lrp_to_lr(const struct ovsdb_idl *idl,
+lrp_to_lr(struct ctl_context *ctx,
           const struct nbrec_logical_router_port *lrp,
           const struct nbrec_logical_router **lr_p)
 {
+    struct nbctl_context *nbctx = nbctl_context_get(ctx);
     const struct nbrec_logical_router *lr;
     *lr_p = NULL;
 
-    NBREC_LOGICAL_ROUTER_FOR_EACH (lr, idl) {
-        for (size_t i = 0; i < lr->n_ports; i++) {
-            if (lr->ports[i] == lrp) {
-                *lr_p = lr;
-                return NULL;
-            }
-        }
+    lr = shash_find_data(&nbctx->lrp_to_lr, lrp->name);
+    if (lr) {
+        *lr_p = lr;
+        return NULL;
     }
 
     /* Can't happen because of the database schema */
@@ -4893,6 +4964,7 @@  static void
 nbctl_lrp_add(struct ctl_context *ctx)
 {
     bool may_exist = shash_find(&ctx->options, "--may-exist") != NULL;
+    struct nbctl_context *nbctx = nbctl_context_get(ctx);
 
     const struct nbrec_logical_router *lr = NULL;
     char *error = lr_by_name_or_uuid(ctx, ctx->argv[1], true, &lr);
@@ -4942,7 +5014,7 @@  nbctl_lrp_add(struct ctl_context *ctx)
         }
 
         const struct nbrec_logical_router *bound_lr;
-        error = lrp_to_lr(ctx->idl, lrp, &bound_lr);
+        error = lrp_to_lr(ctx, lrp, &bound_lr);
         if (error) {
             ctx->error = error;
             return;
@@ -5048,13 +5120,21 @@  nbctl_lrp_add(struct ctl_context *ctx)
                                              lrp);
     nbrec_logical_router_set_ports(lr, new_ports, lr->n_ports + 1);
     free(new_ports);
+
+    /* Updating runtime cache. */
+    shash_add(&nbctx->lrp_to_lr, lrp->name, lr);
 }
 
 /* Removes logical router port 'lr->ports[idx]'. */
 static void
-remove_lrp(const struct nbrec_logical_router *lr, size_t idx)
+remove_lrp(struct ctl_context *ctx, size_t idx,
+           const struct nbrec_logical_router *lr,
+           const struct nbrec_logical_router_port *lrp)
 {
-    const struct nbrec_logical_router_port *lrp = lr->ports[idx];
+    struct nbctl_context *nbctx = nbctl_context_get(ctx);
+
+    /* Updating runtime cache. */
+    shash_find_and_delete(&nbctx->lrp_to_lr, lrp->name);
 
     /* First remove 'lrp' from the array of ports.  This is what will
      * actually cause the logical port to be deleted when the transaction is
@@ -5090,18 +5170,18 @@  nbctl_lrp_del(struct ctl_context *ctx)
 
     /* Find the router that contains 'lrp', then delete it. */
     const struct nbrec_logical_router *lr;
-    NBREC_LOGICAL_ROUTER_FOR_EACH (lr, ctx->idl) {
-        for (size_t i = 0; i < lr->n_ports; i++) {
-            if (lr->ports[i] == lrp) {
-                remove_lrp(lr, i);
-                return;
-            }
+
+    error = lrp_to_lr(ctx, lrp, &lr);
+    if (error) {
+        ctx->error = error;
+        return;
+    }
+    for (size_t i = 0; i < lr->n_ports; i++) {
+        if (lr->ports[i] == lrp) {
+            remove_lrp(ctx, i, lr, lrp);
+            break;
         }
     }
-
-    /* Can't happen because of the database schema. */
-    ctl_error(ctx, "logical port %s is not part of any logical router",
-              ctx->argv[1]);
 }
 
 /* Print a list of logical router ports. */
@@ -5275,7 +5355,7 @@  fwd_group_to_logical_switch(struct ctl_context *ctx,
     }
 
     const struct nbrec_logical_switch *ls;
-    error = lsp_to_ls(ctx->idl, lsp, &ls);
+    error = lsp_to_ls(ctx, lsp, &ls);
     if (error) {
         ctx->error = error;
         return NULL;
@@ -5350,7 +5430,7 @@  nbctl_fwd_group_add(struct ctl_context *ctx)
             return;
         }
         if (lsp) {
-            error = lsp_to_ls(ctx->idl, lsp, &ls);
+            error = lsp_to_ls(ctx, lsp, &ls);
             if (error) {
                 ctx->error = error;
                 return;
@@ -6231,7 +6311,7 @@  do_nbctl(const char *args, struct ctl_command *commands, size_t n_commands,
     struct ovsdb_idl_txn *txn;
     enum ovsdb_idl_txn_status status;
     struct ovsdb_symbol_table *symtab;
-    struct ctl_context ctx;
+    struct nbctl_context ctx;
     struct ctl_command *c;
     struct shash_node *node;
     int64_t next_cfg = 0;
@@ -6268,25 +6348,26 @@  do_nbctl(const char *args, struct ctl_command *commands, size_t n_commands,
         ds_init(&c->output);
         c->table = NULL;
     }
-    ctl_context_init(&ctx, NULL, idl, txn, symtab, NULL);
+    nbctl_context_init(&ctx);
+    ctl_context_init(&ctx.base, NULL, idl, txn, symtab, NULL);
     for (c = commands; c < &commands[n_commands]; c++) {
-        ctl_context_init_command(&ctx, c);
+        ctl_context_init_command(&ctx.base, c);
         if (c->syntax->run) {
-            (c->syntax->run)(&ctx);
+            (c->syntax->run)(&ctx.base);
         }
-        if (ctx.error) {
-            error = xstrdup(ctx.error);
-            ctl_context_done(&ctx, c);
+        if (ctx.base.error) {
+            error = xstrdup(ctx.base.error);
+            ctl_context_done(&ctx.base, c);
             goto out_error;
         }
-        ctl_context_done_command(&ctx, c);
+        ctl_context_done_command(&ctx.base, c);
 
-        if (ctx.try_again) {
-            ctl_context_done(&ctx, NULL);
+        if (ctx.base.try_again) {
+            ctl_context_done(&ctx.base, NULL);
             goto try_again;
         }
     }
-    ctl_context_done(&ctx, NULL);
+    ctl_context_done(&ctx.base, NULL);
 
     SHASH_FOR_EACH (node, &symtab->sh) {
         struct ovsdb_symbol *symbol = node->data;
@@ -6317,14 +6398,14 @@  do_nbctl(const char *args, struct ctl_command *commands, size_t n_commands,
     if (status == TXN_UNCHANGED || status == TXN_SUCCESS) {
         for (c = commands; c < &commands[n_commands]; c++) {
             if (c->syntax->postprocess) {
-                ctl_context_init(&ctx, c, idl, txn, symtab, NULL);
-                (c->syntax->postprocess)(&ctx);
-                if (ctx.error) {
-                    error = xstrdup(ctx.error);
-                    ctl_context_done(&ctx, c);
+                ctl_context_init(&ctx.base, c, idl, txn, symtab, NULL);
+                (c->syntax->postprocess)(&ctx.base);
+                if (ctx.base.error) {
+                    error = xstrdup(ctx.base.error);
+                    ctl_context_done(&ctx.base, c);
                     goto out_error;
                 }
-                ctl_context_done(&ctx, c);
+                ctl_context_done(&ctx.base, c);
             }
         }
     }
@@ -6412,6 +6493,7 @@  do_nbctl(const char *args, struct ctl_command *commands, size_t n_commands,
     done: ;
     }
 
+    nbctl_context_destroy(&ctx);
     ovsdb_symbol_table_destroy(symtab);
     ovsdb_idl_txn_destroy(txn);
     the_idl_txn = NULL;
@@ -6429,6 +6511,7 @@  out_error:
     ovsdb_idl_txn_destroy(txn);
     the_idl_txn = NULL;
 
+    nbctl_context_destroy(&ctx);
     ovsdb_symbol_table_destroy(symtab);
     return error;
 }