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(-)
>
With this series applied the total of CPU cycles consumed by northd and
the per-function profile changes dramatically when running a simple
benchmark of creating 15 lswitches, 100 lports per each lswitch.
`perf stat` for ovn-northd looks like so:
before:
Performance counter stats for process id '7091':
85263.043641 task-clock (msec) # 0.843 CPUs utilized
9,859 context-switches # 0.116 K/sec
575 cpu-migrations # 0.007 K/sec
2,243 page-faults # 0.026 K/sec
225,663,620,029 cycles # 2.647 GHz
302,290,105,647 instructions # 1.34 insn per cycle
53,556,381,940 branches # 628.131 M/sec
435,374,510 branch-misses # 0.81% of all branches
101.202109604 seconds time elapsed
after:
Performance counter stats for process id '25306':
50362.124282 task-clock (msec) # 0.474 CPUs utilized
5,120 context-switches # 0.102 K/sec
1,025 cpu-migrations # 0.020 K/sec
9,546 page-faults # 0.190 K/sec
134,756,308,237 cycles # 2.676 GHz
154,810,279,583 instructions # 1.15 insn per cycle
27,350,003,179 branches # 543.067 M/sec
216,219,142 branch-misses # 0.79% of all branches
106.207558198 seconds time elapsed
CPU time spent in ovn_lflow_find becomes insignificant (<0.1%) after the
changes, so instead let's look at the total cycles spent processing NBDB
contents:
Children Self Command Shared Object Symbol
before:
75.95% 0.01% ovn-northd ovn-northd [.] ovnnb_db_run
after:
8.55% 0.02% ovn-northd ovn-northd [.] ovnnb_db_run
Honestly, I am not sure where the huge boost comes from. I need to read
through the changes more closely. Feel free to add my:
Tested-by: Jakub Sitnicki <[email protected]>
Thanks,
Jakub
> 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);
> +}
> +
> +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
