Re: [ovs-dev] [PATCH] Use the correct logical datapath UUID as input for logical flow hash.
On Mon, Feb 26, 2018 at 11:47:08AM +0100, Jakub Sitnicki wrote: > On Fri, Feb 23, 2018 at 09:05 PM GMT, Ben Pfaff wrote: > > On Thu, Feb 22, 2018 at 10:54:05PM +0100, 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. > > > > Thanks a lot for analyzing the problem and fixing it. > > > > There are basically two options here: use Datapath_Binding UUID > > consistently or use Logical_Switch/Logical_Router UUID consistently. > > This patch takes the latter approach. I'd prefer to take the former > > approach because it is less dependent on the structure of the northbound > > database (which seems desirable for something that is in the southbound > > database) and because it doesn't rely on the external-ids being correct. > > > > I sent a commit that takes the Datapath_Binding UUID approach: > > https://patchwork.ozlabs.org/patch/877306/ > > > > What do you think? > > In general - works for me. I'm wondering about a couple of things: > > 1) build_datapaths() -> join_datapaths() detects and removes duplicated >datapath bindings, AFAIU. I'm not sure how we can end up with >duplicated bindings but if it happens, can it lead to removal and >re-insertion of logical flows for a datapath? I guess it would, if there were duplicate datpath bindings. It's ovn-northd that actually creates the datapath bindings, though, so we would want to fix the problem in ovn-northd in that case. > 2) If I wanted to precalculate an lflow hash and cache it in a synthetic >column in NB DB, ACLs being the potential candidate for that, then it >won't be possible with this approach. That said, I don't know ATM if >this would give a significant gain. If this turns out to be important to performance, I'm more than willing to reconsider the approach. For now, I applied my patch to master. Thanks a lot! ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] Use the correct logical datapath UUID as input for logical flow hash.
On Fri, Feb 23, 2018 at 09:05 PM GMT, Ben Pfaff wrote: > On Thu, Feb 22, 2018 at 10:54:05PM +0100, 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. > > Thanks a lot for analyzing the problem and fixing it. > > There are basically two options here: use Datapath_Binding UUID > consistently or use Logical_Switch/Logical_Router UUID consistently. > This patch takes the latter approach. I'd prefer to take the former > approach because it is less dependent on the structure of the northbound > database (which seems desirable for something that is in the southbound > database) and because it doesn't rely on the external-ids being correct. > > I sent a commit that takes the Datapath_Binding UUID approach: > https://patchwork.ozlabs.org/patch/877306/ > > What do you think? In general - works for me. I'm wondering about a couple of things: 1) build_datapaths() -> join_datapaths() detects and removes duplicated datapath bindings, AFAIU. I'm not sure how we can end up with duplicated bindings but if it happens, can it lead to removal and re-insertion of logical flows for a datapath? 2) If I wanted to precalculate an lflow hash and cache it in a synthetic column in NB DB, ACLs being the potential candidate for that, then it won't be possible with this approach. That said, I don't know ATM if this would give a significant gain. Thanks, Jakub ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] Use the correct logical datapath UUID as input for logical flow hash.
On Thu, Feb 22, 2018 at 10:54:05PM +0100, 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. Thanks a lot for analyzing the problem and fixing it. There are basically two options here: use Datapath_Binding UUID consistently or use Logical_Switch/Logical_Router UUID consistently. This patch takes the latter approach. I'd prefer to take the former approach because it is less dependent on the structure of the northbound database (which seems desirable for something that is in the southbound database) and because it doesn't rely on the external-ids being correct. I sent a commit that takes the Datapath_Binding UUID approach: https://patchwork.ozlabs.org/patch/877306/ What do you think? Thanks, Ben. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] Use the correct logical datapath UUID as input for logical flow hash.
Jakub and I discussed this patch offline and I am now on board with this
patch.
Acked-By: Mark Michelson
Tested-By: Mark Michelson
On 02/23/2018 08:42 AM, Mark Michelson wrote:
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
---
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);
}
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] Use the correct logical datapath UUID as input for logical flow hash.
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 ObjectSymbol
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
---
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);
}
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] Use the correct logical datapath UUID as input for logical flow hash.
On Fri, Feb 23, 2018 at 3:24 AM, 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 ObjectSymbol
> 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
>
Acked-by: Numan Siddique
Tested-by: Numan Siddique
---
> 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);
> }
> --
> 2.14.3
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
