[ovs-dev] [PATCH] Use the correct logical datapath UUID as input for logical flow hash.
Mark Michelson
mmichels at redhat.com
Fri Feb 23 14:42:15 UTC 2018
Hi Jakub,
This patch fixes the issue. However, I think the resulting code can lead
to some confusion. The name of the first parameter given to
ovn_logical_flow_hash() is called "logical_datapath". The result of your
patch is that the actual first parameter passed to that function is
either a logical switch or logical router UUID, not a logical datapath UUID.
Looking more closely, I think the intent was to pass a SB logical
datapath UUID to the function. The error was that ovn_lflow_hash() in
ovn-northd.c mistakenly thought that an ovn_datapath's header._uuid is a
logical datapath UUID. I think the original intent was probably to pass
lflow->od->sb->_header.uuid to ovn_logical_flow_hash() from
ovn_lflow_hash().
We can go with your patch, but I would suggest changing the name of the
first parameter of ovn_logical_flow_hash() to avoid confusion.
Alternately, you can go with the suggestion I made above. The possible
advantage it has is that it requires no lookups of external-ids in the
southbound datapath.
On 02/22/2018 03:54 PM, Jakub Sitnicki wrote:
> Use the logical switch/router UUID as hash input, instead of the UUID of
> the Datapath_Binding row, when calculating the hash value for lflows in
> the SBDB.
>
> Otherwise the hash value will never match the one computed from NBDB
> contents, which will force ovn-northd to constantly drop and attempt to
> re-insert all the lflows.
>
> This brings down the performance boost from caching the hash values
> computed for logical flows in SBDB down to the expected level:
>
> Children Self Command Shared Object Symbol
> before:
> 76.19% 0.01% ovn-northd ovn-northd [.] ovnnb_db_run
> 11.04% 0.43% ovn-northd ovn-northd [.] ovn_lflow_find
> after:
> 75.16% 0.05% ovn-northd ovn-northd [.] ovnnb_db_run
> 2.49% 0.17% ovn-northd ovn-northd [.] ovn_lflow_find
>
> Fixes: 8bf332225d4a ("ovn-northd: Reduce amount of flow hashing.")
> Signed-off-by: Jakub Sitnicki <jkbs at redhat.com>
> ---
> ovn/lib/ovn-util.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/ovn/lib/ovn-util.c b/ovn/lib/ovn-util.c
> index e9464e926..1761defd9 100644
> --- a/ovn/lib/ovn-util.c
> +++ b/ovn/lib/ovn-util.c
> @@ -339,7 +339,13 @@ sbrec_logical_flow_hash(const struct sbrec_logical_flow *lf)
> return 0;
> }
>
> - return ovn_logical_flow_hash(&ld->header_.uuid,
> + struct uuid ld_uuid;
> + if (!smap_get_uuid(&ld->external_ids, "logical-switch", &ld_uuid) &&
> + !smap_get_uuid(&ld->external_ids, "logical-router", &ld_uuid)) {
> + return 0;
> + }
> +
> + return ovn_logical_flow_hash(&ld_uuid,
> lf->table_id, lf->pipeline,
> lf->priority, lf->match, lf->actions);
> }
>
More information about the dev
mailing list