Re: [ovs-dev] [PATCH ovn v2 5/5] ovn-controller: Fix incremental processing for logical port references.
On 6/21/21 8:53 AM, Han Zhou wrote: > is_cr_cond_present only checks if is_chassis_resident() exists (which would > reference a logical port). lflow_ref_lookup() checks for all the logical > port references (not just by is_chassis_resident()). > For lflows that reference logical ports, we can cache the expr but not the > match. I wondered if this could impact performance but from my earlier > tests there was no impact, but maybe there are scenarios that may have > performance impact but I didn't test. Makes sense, thanks for the clarification! > I also noticed that I forgot to remove the variable is_cr_cond_present > which is not used any more. I removed it in v3. I'll have a look at v3 this week. Regards, Dumitru ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn v2 5/5] ovn-controller: Fix incremental processing for logical port references.
On Fri, Jun 18, 2021 at 8:52 AM Dumitru Ceara wrote:
>
> On 6/11/21 9:35 PM, Han Zhou wrote:
> > If a lflow has an lport name in the match, but when the lflow is
> > processed the port-binding is not seen by ovn-controller, the
> > corresponding openflow will not be created. Later if the port-binding is
> > created/monitored by ovn-controller, the lflow is not reprocessed
> > because the lflow didn't change and ovn-controller doesn't know that the
> > port-binding affects the lflow. This patch fixes the problem by tracking
> > the references when parsing the lflow, even if the port-binding is not
> > found when the lflow is firstly parsed. A test case is also added to
> > cover the scenario.
> >
> > Signed-off-by: Han Zhou
> > ---
> > controller/lflow.c | 64 +++--
> > controller/lflow.h | 3 ++
> > controller/ovn-controller.c | 17 --
> > tests/ovn.at| 47 +++
> > 4 files changed, 113 insertions(+), 18 deletions(-)
> >
> > diff --git a/controller/lflow.c b/controller/lflow.c
> > index 34eca135a..7ae0ed15e 100644
> > --- a/controller/lflow.c
> > +++ b/controller/lflow.c
> > @@ -61,6 +61,7 @@ struct lookup_port_aux {
> >
> > struct condition_aux {
> > struct ovsdb_idl_index *sbrec_port_binding_by_name;
> > +const struct sbrec_datapath_binding *dp;
> > const struct sbrec_chassis *chassis;
> > const struct sset *active_tunnels;
> > const struct sbrec_logical_flow *lflow;
> > @@ -98,6 +99,12 @@ lookup_port_cb(const void *aux_, const char
*port_name, unsigned int *portp)
> >
> > const struct lookup_port_aux *aux = aux_;
> >
> > +/* Store the name that used to lookup the lport to lflow
reference, so that
> > + * in the future when the lport's port binding changes, the
logical flow
> > + * that references this lport can be reprocessed. */
> > +lflow_resource_add(aux->lfrr, REF_TYPE_PORTBINDING, port_name,
> > + &aux->lflow->header_.uuid);
> > +
> > const struct sbrec_port_binding *pb
> > = lport_lookup_by_name(aux->sbrec_port_binding_by_name,
port_name);
> > if (pb && pb->datapath == aux->dp) {
> > @@ -149,19 +156,18 @@ is_chassis_resident_cb(const void *c_aux_, const
char *port_name)
> > {
> > const struct condition_aux *c_aux = c_aux_;
> >
> > +/* Store the port name that used to lookup the lport to lflow
reference, so
> > + * that in the future when the lport's port-binding changes the
logical
> > + * flow that references this lport can be reprocessed. */
> > +lflow_resource_add(c_aux->lfrr, REF_TYPE_PORTBINDING, port_name,
> > + &c_aux->lflow->header_.uuid);
> > +
> > const struct sbrec_port_binding *pb
> > = lport_lookup_by_name(c_aux->sbrec_port_binding_by_name,
port_name);
> > if (!pb) {
> > return false;
> > }
> >
> > -/* Store the port_name to lflow reference. */
> > -int64_t dp_id = pb->datapath->tunnel_key;
> > -char buf[16];
> > -get_unique_lport_key(dp_id, pb->tunnel_key, buf, sizeof(buf));
> > -lflow_resource_add(c_aux->lfrr, REF_TYPE_PORTBINDING, buf,
> > - &c_aux->lflow->header_.uuid);
> > -
> > if (strcmp(pb->type, "chassisredirect")) {
> > /* for non-chassisredirect ports */
> > return pb->chassis && pb->chassis == c_aux->chassis;
> > @@ -623,8 +629,6 @@ add_matches_to_flow_table(const struct
sbrec_logical_flow *lflow,
> > int64_t dp_id = dp->tunnel_key;
> > char buf[16];
> > get_unique_lport_key(dp_id, port_id, buf, sizeof(buf));
> > -lflow_resource_add(l_ctx_out->lfrr,
REF_TYPE_PORTBINDING, buf,
> > - &lflow->header_.uuid);
> > if (!sset_contains(l_ctx_in->local_lport_ids, buf)) {
> > VLOG_DBG("lflow "UUID_FMT
> > " port %s in match is not local, skip",
> > @@ -788,6 +792,7 @@ consider_logical_flow__(const struct
sbrec_logical_flow *lflow,
> > };
> > struct condition_aux cond_aux = {
> > .sbrec_port_binding_by_name =
l_ctx_in->sbrec_port_binding_by_name,
> > +.dp = dp,
> > .chassis = l_ctx_in->chassis,
> > .active_tunnels = l_ctx_in->active_tunnels,
> > .lflow = lflow,
> > @@ -883,7 +888,9 @@ consider_logical_flow__(const struct
sbrec_logical_flow *lflow,
> >
> > /* Cache new entry if caching is enabled. */
> > if (lflow_cache_is_enabled(l_ctx_out->lflow_cache)) {
> > -if (cached_expr && !is_cr_cond_present) {
> > +if (cached_expr
> > +&& !lflow_ref_lookup(&l_ctx_out->lfrr->lflow_ref_table,
> > + &lflow->header_.uuid)) {
>
> Why is "!is_cr_cond_present" not OK here? It seems to me that the
> result is the same except that lflow_ref_lookup() will do more wor
Re: [ovs-dev] [PATCH ovn v2 5/5] ovn-controller: Fix incremental processing for logical port references.
On 6/11/21 9:35 PM, Han Zhou wrote:
> If a lflow has an lport name in the match, but when the lflow is
> processed the port-binding is not seen by ovn-controller, the
> corresponding openflow will not be created. Later if the port-binding is
> created/monitored by ovn-controller, the lflow is not reprocessed
> because the lflow didn't change and ovn-controller doesn't know that the
> port-binding affects the lflow. This patch fixes the problem by tracking
> the references when parsing the lflow, even if the port-binding is not
> found when the lflow is firstly parsed. A test case is also added to
> cover the scenario.
>
> Signed-off-by: Han Zhou
> ---
> controller/lflow.c | 64 +++--
> controller/lflow.h | 3 ++
> controller/ovn-controller.c | 17 --
> tests/ovn.at| 47 +++
> 4 files changed, 113 insertions(+), 18 deletions(-)
>
> diff --git a/controller/lflow.c b/controller/lflow.c
> index 34eca135a..7ae0ed15e 100644
> --- a/controller/lflow.c
> +++ b/controller/lflow.c
> @@ -61,6 +61,7 @@ struct lookup_port_aux {
>
> struct condition_aux {
> struct ovsdb_idl_index *sbrec_port_binding_by_name;
> +const struct sbrec_datapath_binding *dp;
> const struct sbrec_chassis *chassis;
> const struct sset *active_tunnels;
> const struct sbrec_logical_flow *lflow;
> @@ -98,6 +99,12 @@ lookup_port_cb(const void *aux_, const char *port_name,
> unsigned int *portp)
>
> const struct lookup_port_aux *aux = aux_;
>
> +/* Store the name that used to lookup the lport to lflow reference, so
> that
> + * in the future when the lport's port binding changes, the logical flow
> + * that references this lport can be reprocessed. */
> +lflow_resource_add(aux->lfrr, REF_TYPE_PORTBINDING, port_name,
> + &aux->lflow->header_.uuid);
> +
> const struct sbrec_port_binding *pb
> = lport_lookup_by_name(aux->sbrec_port_binding_by_name, port_name);
> if (pb && pb->datapath == aux->dp) {
> @@ -149,19 +156,18 @@ is_chassis_resident_cb(const void *c_aux_, const char
> *port_name)
> {
> const struct condition_aux *c_aux = c_aux_;
>
> +/* Store the port name that used to lookup the lport to lflow reference,
> so
> + * that in the future when the lport's port-binding changes the logical
> + * flow that references this lport can be reprocessed. */
> +lflow_resource_add(c_aux->lfrr, REF_TYPE_PORTBINDING, port_name,
> + &c_aux->lflow->header_.uuid);
> +
> const struct sbrec_port_binding *pb
> = lport_lookup_by_name(c_aux->sbrec_port_binding_by_name, port_name);
> if (!pb) {
> return false;
> }
>
> -/* Store the port_name to lflow reference. */
> -int64_t dp_id = pb->datapath->tunnel_key;
> -char buf[16];
> -get_unique_lport_key(dp_id, pb->tunnel_key, buf, sizeof(buf));
> -lflow_resource_add(c_aux->lfrr, REF_TYPE_PORTBINDING, buf,
> - &c_aux->lflow->header_.uuid);
> -
> if (strcmp(pb->type, "chassisredirect")) {
> /* for non-chassisredirect ports */
> return pb->chassis && pb->chassis == c_aux->chassis;
> @@ -623,8 +629,6 @@ add_matches_to_flow_table(const struct sbrec_logical_flow
> *lflow,
> int64_t dp_id = dp->tunnel_key;
> char buf[16];
> get_unique_lport_key(dp_id, port_id, buf, sizeof(buf));
> -lflow_resource_add(l_ctx_out->lfrr, REF_TYPE_PORTBINDING,
> buf,
> - &lflow->header_.uuid);
> if (!sset_contains(l_ctx_in->local_lport_ids, buf)) {
> VLOG_DBG("lflow "UUID_FMT
> " port %s in match is not local, skip",
> @@ -788,6 +792,7 @@ consider_logical_flow__(const struct sbrec_logical_flow
> *lflow,
> };
> struct condition_aux cond_aux = {
> .sbrec_port_binding_by_name = l_ctx_in->sbrec_port_binding_by_name,
> +.dp = dp,
> .chassis = l_ctx_in->chassis,
> .active_tunnels = l_ctx_in->active_tunnels,
> .lflow = lflow,
> @@ -883,7 +888,9 @@ consider_logical_flow__(const struct sbrec_logical_flow
> *lflow,
>
> /* Cache new entry if caching is enabled. */
> if (lflow_cache_is_enabled(l_ctx_out->lflow_cache)) {
> -if (cached_expr && !is_cr_cond_present) {
> +if (cached_expr
> +&& !lflow_ref_lookup(&l_ctx_out->lfrr->lflow_ref_table,
> + &lflow->header_.uuid)) {
Why is "!is_cr_cond_present" not OK here? It seems to me that the
result is the same except that lflow_ref_lookup() will do more work.
> lflow_cache_add_matches(l_ctx_out->lflow_cache,
> &lflow->header_.uuid, matches,
> matches_size);
> @@ -1746
[ovs-dev] [PATCH ovn v2 5/5] ovn-controller: Fix incremental processing for logical port references.
If a lflow has an lport name in the match, but when the lflow is
processed the port-binding is not seen by ovn-controller, the
corresponding openflow will not be created. Later if the port-binding is
created/monitored by ovn-controller, the lflow is not reprocessed
because the lflow didn't change and ovn-controller doesn't know that the
port-binding affects the lflow. This patch fixes the problem by tracking
the references when parsing the lflow, even if the port-binding is not
found when the lflow is firstly parsed. A test case is also added to
cover the scenario.
Signed-off-by: Han Zhou
---
controller/lflow.c | 64 +++--
controller/lflow.h | 3 ++
controller/ovn-controller.c | 17 --
tests/ovn.at| 47 +++
4 files changed, 113 insertions(+), 18 deletions(-)
diff --git a/controller/lflow.c b/controller/lflow.c
index 34eca135a..7ae0ed15e 100644
--- a/controller/lflow.c
+++ b/controller/lflow.c
@@ -61,6 +61,7 @@ struct lookup_port_aux {
struct condition_aux {
struct ovsdb_idl_index *sbrec_port_binding_by_name;
+const struct sbrec_datapath_binding *dp;
const struct sbrec_chassis *chassis;
const struct sset *active_tunnels;
const struct sbrec_logical_flow *lflow;
@@ -98,6 +99,12 @@ lookup_port_cb(const void *aux_, const char *port_name,
unsigned int *portp)
const struct lookup_port_aux *aux = aux_;
+/* Store the name that used to lookup the lport to lflow reference, so that
+ * in the future when the lport's port binding changes, the logical flow
+ * that references this lport can be reprocessed. */
+lflow_resource_add(aux->lfrr, REF_TYPE_PORTBINDING, port_name,
+ &aux->lflow->header_.uuid);
+
const struct sbrec_port_binding *pb
= lport_lookup_by_name(aux->sbrec_port_binding_by_name, port_name);
if (pb && pb->datapath == aux->dp) {
@@ -149,19 +156,18 @@ is_chassis_resident_cb(const void *c_aux_, const char
*port_name)
{
const struct condition_aux *c_aux = c_aux_;
+/* Store the port name that used to lookup the lport to lflow reference, so
+ * that in the future when the lport's port-binding changes the logical
+ * flow that references this lport can be reprocessed. */
+lflow_resource_add(c_aux->lfrr, REF_TYPE_PORTBINDING, port_name,
+ &c_aux->lflow->header_.uuid);
+
const struct sbrec_port_binding *pb
= lport_lookup_by_name(c_aux->sbrec_port_binding_by_name, port_name);
if (!pb) {
return false;
}
-/* Store the port_name to lflow reference. */
-int64_t dp_id = pb->datapath->tunnel_key;
-char buf[16];
-get_unique_lport_key(dp_id, pb->tunnel_key, buf, sizeof(buf));
-lflow_resource_add(c_aux->lfrr, REF_TYPE_PORTBINDING, buf,
- &c_aux->lflow->header_.uuid);
-
if (strcmp(pb->type, "chassisredirect")) {
/* for non-chassisredirect ports */
return pb->chassis && pb->chassis == c_aux->chassis;
@@ -623,8 +629,6 @@ add_matches_to_flow_table(const struct sbrec_logical_flow
*lflow,
int64_t dp_id = dp->tunnel_key;
char buf[16];
get_unique_lport_key(dp_id, port_id, buf, sizeof(buf));
-lflow_resource_add(l_ctx_out->lfrr, REF_TYPE_PORTBINDING, buf,
- &lflow->header_.uuid);
if (!sset_contains(l_ctx_in->local_lport_ids, buf)) {
VLOG_DBG("lflow "UUID_FMT
" port %s in match is not local, skip",
@@ -788,6 +792,7 @@ consider_logical_flow__(const struct sbrec_logical_flow
*lflow,
};
struct condition_aux cond_aux = {
.sbrec_port_binding_by_name = l_ctx_in->sbrec_port_binding_by_name,
+.dp = dp,
.chassis = l_ctx_in->chassis,
.active_tunnels = l_ctx_in->active_tunnels,
.lflow = lflow,
@@ -883,7 +888,9 @@ consider_logical_flow__(const struct sbrec_logical_flow
*lflow,
/* Cache new entry if caching is enabled. */
if (lflow_cache_is_enabled(l_ctx_out->lflow_cache)) {
-if (cached_expr && !is_cr_cond_present) {
+if (cached_expr
+&& !lflow_ref_lookup(&l_ctx_out->lfrr->lflow_ref_table,
+ &lflow->header_.uuid)) {
lflow_cache_add_matches(l_ctx_out->lflow_cache,
&lflow->header_.uuid, matches,
matches_size);
@@ -1746,19 +1753,44 @@ lflow_processing_end:
return handled;
}
+/* Handles a port-binding change that is possibly related to a lport's
+ * residence status on this chassis. */
bool
lflow_handle_flows_for_lport(const struct sbrec_port_binding *pb,
struct lflow_ctx_in *l_ctx_in,
struct lflow_ctx_out *l_ctx_out)
{
+bool re
