Hi Ben,
On Wed, Feb 14, 2018 at 09:54 PM GMT, Ben Pfaff wrote:
> Jakub Sitnicki demonstrated that repeatedly calculating row hashes is
> expensive, so this should improve ovn-northd performance.
>
> Reported-by: Jakub Sitnicki <[email protected]>
> Reported-at:
> https://mail.openvswitch.org/pipermail/ovs-dev/2018-February/344404.html
> Signed-off-by: Ben Pfaff <[email protected]>
> ---
> ovn/lib/ovn-sb-idl.ann | 20 ++++++++++++++++++++
> ovn/lib/ovn-util.c | 27 +++++++++++++++++++++++++++
> ovn/lib/ovn-util.h | 7 +++++++
> ovn/northd/ovn-northd.c | 28 +++++++++++++++++-----------
> 4 files changed, 71 insertions(+), 11 deletions(-)
>
> diff --git a/ovn/lib/ovn-sb-idl.ann b/ovn/lib/ovn-sb-idl.ann
> index 2dfc64e3c4a7..e51238b92e97 100644
> --- a/ovn/lib/ovn-sb-idl.ann
> +++ b/ovn/lib/ovn-sb-idl.ann
> @@ -7,3 +7,23 @@
>
> s["idlPrefix"] = "sbrec_"
> s["idlHeader"] = "\"ovn/lib/ovn-sb-idl.h\""
> +
> +s["hDecls"] = '#include "ovn/lib/ovn-util.h"'
> +
> +# Adds an integer column named 'column' to 'table' in 's'. The column
> +# values is calculated with 'expression' based on the values of the columns
> +# named in the array 'dependencies'.
> +def synthesize_integer_column(s, table, column, dependencies, expression):
> + s["tables"][table]["columns"][column] = {
> + "type": "integer",
> + "extensions": {
> + "dependencies": dependencies,
> + "parse": "row->%s = %s;" % (column, expression),
> + "synthetic": True
> + }
> + }
> +
> +synthesize_integer_column(s, "Logical_Flow", "hash",
> + ["logical_datapath", "table_id", "pipeline",
> + "priority", "match", "actions"],
> + "sbrec_logical_flow_hash(row)")
> diff --git a/ovn/lib/ovn-util.c b/ovn/lib/ovn-util.c
> index a554c23f5ae8..e9464e926d74 100644
> --- a/ovn/lib/ovn-util.c
> +++ b/ovn/lib/ovn-util.c
> @@ -17,6 +17,7 @@
> #include "dirs.h"
> #include "openvswitch/vlog.h"
> #include "ovn/lib/ovn-nb-idl.h"
> +#include "ovn/lib/ovn-sb-idl.h"
>
> VLOG_DEFINE_THIS_MODULE(ovn_util);
>
> @@ -329,3 +330,29 @@ ovn_is_known_nb_lsp_type(const char *type)
>
> return false;
> }
> +
> +uint32_t
> +sbrec_logical_flow_hash(const struct sbrec_logical_flow *lf)
> +{
> + const struct sbrec_datapath_binding *ld = lf->logical_datapath;
> + if (!ld) {
> + return 0;
> + }
> +
> + return ovn_logical_flow_hash(&ld->header_.uuid,
> + lf->table_id, lf->pipeline,
> + lf->priority, lf->match, lf->actions);
> +}
It looks like we should be hashing the logical datapath UUID here,
instead of the UUID of the entry in Datapath_Binding. Right now, the
hash computed from SB record will never match the hash computed from
NBDB contents.
This leads to northd contantly dropping records from SBDB and inserting
them again and again. I believe this is the root cause of problems that
Mark and Numan are reporting.
-Jakub
> +
> +uint32_t
> +ovn_logical_flow_hash(const struct uuid *logical_datapath,
> + uint8_t table_id, const char *pipeline,
> + uint16_t priority,
> + const char *match, const char *actions)
> +{
> + size_t hash = uuid_hash(logical_datapath);
> + hash = hash_2words((table_id << 16) | priority, hash);
> + hash = hash_string(pipeline, hash);
> + hash = hash_string(match, hash);
> + return hash_string(actions, hash);
> +}
> diff --git a/ovn/lib/ovn-util.h b/ovn/lib/ovn-util.h
> index 9b456426dc67..7ff9f9a1c73b 100644
> --- a/ovn/lib/ovn-util.h
> +++ b/ovn/lib/ovn-util.h
> @@ -19,6 +19,7 @@
> #include "lib/packets.h"
>
> struct nbrec_logical_router_port;
> +struct sbrec_logical_flow;
> struct uuid;
>
> struct ipv4_netaddr {
> @@ -69,4 +70,10 @@ const char *default_sb_db(void);
>
> bool ovn_is_known_nb_lsp_type(const char *type);
>
> +uint32_t sbrec_logical_flow_hash(const struct sbrec_logical_flow *);
> +uint32_t ovn_logical_flow_hash(const struct uuid *logical_datapath,
> + uint8_t table_id, const char *pipeline,
> + uint16_t priority,
> + const char *match, const char *actions);
> +
> #endif
> diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
> index 4d95a3d9d40e..5d764f6e9404 100644
> --- a/ovn/northd/ovn-northd.c
> +++ b/ovn/northd/ovn-northd.c
> @@ -194,6 +194,13 @@ ovn_stage_get_pipeline(enum ovn_stage stage)
> return (stage >> 8) & 1;
> }
>
> +/* Returns the pipeline name to which 'stage' belongs. */
> +static const char *
> +ovn_stage_get_pipeline_name(enum ovn_stage stage)
> +{
> + return ovn_stage_get_pipeline(stage) == P_IN ? "ingress" : "egress";
> +}
> +
> /* Returns the table to which 'stage' belongs. */
> static uint8_t
> ovn_stage_get_table(enum ovn_stage stage)
> @@ -2271,10 +2278,11 @@ struct ovn_lflow {
> static size_t
> ovn_lflow_hash(const struct ovn_lflow *lflow)
> {
> - size_t hash = uuid_hash(&lflow->od->key);
> - hash = hash_2words((lflow->stage << 16) | lflow->priority, hash);
> - hash = hash_string(lflow->match, hash);
> - return hash_string(lflow->actions, hash);
> + return ovn_logical_flow_hash(&lflow->od->key,
> + ovn_stage_get_table(lflow->stage),
> + ovn_stage_get_pipeline_name(lflow->stage),
> + lflow->priority, lflow->match,
> + lflow->actions);
> }
>
> static bool
> @@ -2331,7 +2339,7 @@ ovn_lflow_add_at(struct hmap *lflow_map, struct
> ovn_datapath *od,
> static struct ovn_lflow *
> ovn_lflow_find(struct hmap *lflows, struct ovn_datapath *od,
> enum ovn_stage stage, uint16_t priority,
> - const char *match, const char *actions)
> + const char *match, const char *actions, uint32_t hash)
> {
> struct ovn_lflow target;
> ovn_lflow_init(&target, od, stage, priority,
> @@ -2339,8 +2347,7 @@ ovn_lflow_find(struct hmap *lflows, struct ovn_datapath
> *od,
> NULL, NULL);
>
> struct ovn_lflow *lflow;
> - HMAP_FOR_EACH_WITH_HASH (lflow, hmap_node, ovn_lflow_hash(&target),
> - lflows) {
> + HMAP_FOR_EACH_WITH_HASH (lflow, hmap_node, hash, lflows) {
> if (ovn_lflow_equal(lflow, &target)) {
> return lflow;
> }
> @@ -6014,7 +6021,7 @@ build_lflows(struct northd_context *ctx, struct hmap
> *datapaths,
> = !strcmp(sbflow->pipeline, "ingress") ? P_IN : P_OUT;
> struct ovn_lflow *lflow = ovn_lflow_find(
> &lflows, od, ovn_stage_build(dp_type, pipeline,
> sbflow->table_id),
> - sbflow->priority, sbflow->match, sbflow->actions);
> + sbflow->priority, sbflow->match, sbflow->actions, sbflow->hash);
> if (lflow) {
> ovn_lflow_destroy(&lflows, lflow);
> } else {
> @@ -6023,13 +6030,12 @@ build_lflows(struct northd_context *ctx, struct hmap
> *datapaths,
> }
> struct ovn_lflow *lflow, *next_lflow;
> HMAP_FOR_EACH_SAFE (lflow, next_lflow, hmap_node, &lflows) {
> - enum ovn_pipeline pipeline = ovn_stage_get_pipeline(lflow->stage);
> + const char *pipeline = ovn_stage_get_pipeline_name(lflow->stage);
> uint8_t table = ovn_stage_get_table(lflow->stage);
>
> sbflow = sbrec_logical_flow_insert(ctx->ovnsb_txn);
> sbrec_logical_flow_set_logical_datapath(sbflow, lflow->od->sb);
> - sbrec_logical_flow_set_pipeline(
> - sbflow, pipeline == P_IN ? "ingress" : "egress");
> + sbrec_logical_flow_set_pipeline(sbflow, pipeline);
> sbrec_logical_flow_set_table_id(sbflow, table);
> sbrec_logical_flow_set_priority(sbflow, lflow->priority);
> sbrec_logical_flow_set_match(sbflow, lflow->match);
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
