| Message ID | 20180214215457.18110-4-blp@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev,1/4] ovsdb-idlc: Add "cDecls" and "hDecls" IDL schema extensions. | expand |
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 <jkbs@redhat.com> > Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2018-February/344404.html > Signed-off-by: Ben Pfaff <blp@ovn.org> > --- > 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 <jkbs@redhat.com> 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);
On Fri, Feb 16, 2018 at 05:10:29PM +0100, Jakub Sitnicki wrote: > 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 <jkbs@redhat.com> > > Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2018-February/344404.html > > Signed-off-by: Ben Pfaff <blp@ovn.org> > > --- > > 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 <jkbs@redhat.com> Thanks a lot for the testing. Please do feel free to continue reading the patch series. I am also surprised that there was so much impact--I guess that hashing text strings is really expensive! Thanks, Ben.
> On Fri, Feb 16, 2018 at 05:10:29PM +0100, Jakub Sitnicki wrote: >> 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 <jkbs@redhat.com> >> > Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2018-February/344404.html >> > Signed-off-by: Ben Pfaff <blp@ovn.org> >> > --- >> > 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 <jkbs@redhat.com> > > Thanks a lot for the testing. Please do feel free to continue reading > the patch series. I am also surprised that there was so much impact--I > guess that hashing text strings is really expensive! > > Thanks, > > Ben. > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev Hi Ben, working on automatic test support for OVN ACL reject action I discovered this commit breaks some ovn.at tests. In particular: - 2371: ovn -- dns lookup : 1 HV, 2 LS, 2 LSPs/LS - 2374: ovn -- 1 LR with distributed router gateway port I run git bisect on the current master branch (commit 430d7d156). Numan reproduced same issues. I guess they are probably timing issues Regards, Lorenzo
Hi,
While testing an unrelated change in OVS master (using make sandbox
SANDBOXFLAGS="--ovn"), I noticed that my laptop was making way more
noise than normal. Looking at `top` output, an ovsdb-server process was
running at 100% CPU, ovn-controller was running at 75% CPU, and
ovn-northd was running at about 38% CPU. If I start the ovs sandbox, the
problem does not present itself until after I run the "ovn-setup.sh"
script. As soon as I run the script, the CPU usage goes soaring.
If I revert to the patch just before this one ("implement synthetic
columns"), the problem does not occur. At least in a sandbox
environment, this patch appears to be causing some sort of busy loops
that drive the CPU% of processes up. Looking at the change, it's not
immediately obvious why this would be happening.
On 02/14/2018 03:54 PM, Ben Pfaff wrote:
> Jakub Sitnicki demonstrated that repeatedly calculating row hashes is
> expensive, so this should improve ovn-northd performance.
>
> Reported-by: Jakub Sitnicki <jkbs@redhat.com>
> Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2018-February/344404.html
> Signed-off-by: Ben Pfaff <blp@ovn.org>
> ---
> 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);
> +}
> +
> +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);
>
On Thu, Feb 22, 2018 at 4:49 AM, Mark Michelson <mmichels@redhat.com> wrote: > Hi, > > While testing an unrelated change in OVS master (using make sandbox > SANDBOXFLAGS="--ovn"), I noticed that my laptop was making way more noise > than normal. Looking at `top` output, an ovsdb-server process was running > at 100% CPU, ovn-controller was running at 75% CPU, and ovn-northd was > running at about 38% CPU. If I start the ovs sandbox, the problem does not > present itself until after I run the "ovn-setup.sh" script. As soon as I > run the script, the CPU usage goes soaring. > > If I revert to the patch just before this one ("implement synthetic > columns"), the problem does not occur. At least in a sandbox environment, > this patch appears to be causing some sort of busy loops that drive the > CPU% of processes up. Looking at the change, it's not immediately obvious > why this would be happening. > > I can confirm that I am seeing the same with my sandbox environment. OVN North db ovsdb-server is almost 100% and ovn-controller at around 90% in my case. Thanks Numan > On 02/14/2018 03:54 PM, Ben Pfaff wrote: > >> Jakub Sitnicki demonstrated that repeatedly calculating row hashes is >> expensive, so this should improve ovn-northd performance. >> >> Reported-by: Jakub Sitnicki <jkbs@redhat.com> >> Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2018-February >> /344404.html >> Signed-off-by: Ben Pfaff <blp@ovn.org> >> --- >> 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); >> +} >> + >> +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(l >> flow->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(lf >> low->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 > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
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 <jkbs@redhat.com> > Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2018-February/344404.html > Signed-off-by: Ben Pfaff <blp@ovn.org> > --- > 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);
On Thu, 22 Feb 2018 18:07:55 +0100 Jakub Sitnicki <jkbs@redhat.com> wrote: > On Wed, Feb 14, 2018 at 09:54 PM GMT, Ben Pfaff wrote: [...] > > +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. Proposed fix now posted: https://patchwork.ozlabs.org/patch/876842/ 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);
Jakub Sitnicki demonstrated that repeatedly calculating row hashes is expensive, so this should improve ovn-northd performance. Reported-by: Jakub Sitnicki <jkbs@redhat.com> Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2018-February/344404.html Signed-off-by: Ben Pfaff <blp@ovn.org> --- 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(-)