"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?
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
