Re: [ovs-dev] [PATCH ovn v3 3/3] ovn-controller: Fix incremental processing for logical port references.
On Thu, Jun 24, 2021 at 10:33 AM Dumitru Ceara wrote: > > On 6/24/21 7:20 PM, Han Zhou wrote: > > For the reason mentioned above, we can't make this change. In fact, I > > wouldn't worry much about lflow_ref_lookup()'s cost. It is O(1) operation. > > If it really turns out to be a bottleneck, we could optimize the > > function/data-structure, without worrying about the logic. > > The real performance impact part is probably not being able to cache the > > "match" for lflows that have logical port references, but I will work on > > some other solutions to optimize that. > > OTOH, on real deployments the lflow cache limits should be enforced by > the CMS. Therefore I would expect some of these flows to not make it in > the cache anyway (even without your change). I don't have data to back > this up but I'm guessing the impact of the change in this patch will be > minimal. > > Regards, > Dumitru > Thanks Dumitru. Numan, I sent v4 that adds more coverage in the test case. Please take a look: https://patchwork.ozlabs.org/project/ovn/patch/20210629192257.1699504-1-hz...@ovn.org/ Thanks, Han ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn v3 3/3] ovn-controller: Fix incremental processing for logical port references.
On 6/24/21 7:20 PM, Han Zhou wrote: > For the reason mentioned above, we can't make this change. In fact, I > wouldn't worry much about lflow_ref_lookup()'s cost. It is O(1) operation. > If it really turns out to be a bottleneck, we could optimize the > function/data-structure, without worrying about the logic. > The real performance impact part is probably not being able to cache the > "match" for lflows that have logical port references, but I will work on > some other solutions to optimize that. OTOH, on real deployments the lflow cache limits should be enforced by the CMS. Therefore I would expect some of these flows to not make it in the cache anyway (even without your change). I don't have data to back this up but I'm guessing the impact of the change in this patch will be minimal. Regards, Dumitru ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn v3 3/3] ovn-controller: Fix incremental processing for logical port references.
On Thu, Jun 24, 2021 at 7:46 AM Numan Siddique wrote:
>
> On Wed, Jun 23, 2021 at 8:10 PM Han Zhou wrote:
> >
> > On Wed, Jun 23, 2021 at 7:48 AM Numan Siddique wrote:
> > >
> > > On Mon, Jun 21, 2021 at 2:52 AM 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
> > >
> > > Hi Han,
> > >
> > > Thanks for fixing these issues. I've a few questions. I haven't
> > > reviewed the patch completely.
> > >
> > >
> > > > ---
> > > > controller/lflow.c | 63
++---
> > > > controller/lflow.h | 3 ++
> > > > controller/ovn-controller.c | 35 -
> > > > include/ovn/expr.h | 2 +-
> > > > lib/expr.c | 14 +++--
> > > > tests/ovn.at| 47 +++
> > > > tests/test-ovn.c| 4 +--
> > > > utilities/ovn-trace.c | 2 +-
> > > > 8 files changed, 132 insertions(+), 38 deletions(-)
> > > >
> > > > diff --git a/controller/lflow.c b/controller/lflow.c
> > > > index 34eca135a..b7699a309 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,
> > > > + >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,
> > > > + _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,
> > > > - _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,
> > > > - >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
Re: [ovs-dev] [PATCH ovn v3 3/3] ovn-controller: Fix incremental processing for logical port references.
On Wed, Jun 23, 2021 at 8:10 PM Han Zhou wrote:
>
> On Wed, Jun 23, 2021 at 7:48 AM Numan Siddique wrote:
> >
> > On Mon, Jun 21, 2021 at 2:52 AM 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
> >
> > Hi Han,
> >
> > Thanks for fixing these issues. I've a few questions. I haven't
> > reviewed the patch completely.
> >
> >
> > > ---
> > > controller/lflow.c | 63 ++---
> > > controller/lflow.h | 3 ++
> > > controller/ovn-controller.c | 35 -
> > > include/ovn/expr.h | 2 +-
> > > lib/expr.c | 14 +++--
> > > tests/ovn.at| 47 +++
> > > tests/test-ovn.c| 4 +--
> > > utilities/ovn-trace.c | 2 +-
> > > 8 files changed, 132 insertions(+), 38 deletions(-)
> > >
> > > diff --git a/controller/lflow.c b/controller/lflow.c
> > > index 34eca135a..b7699a309 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,
> > > + >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,
> > > + _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,
> > > - _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,
> > > - >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,
> > > @@
Re: [ovs-dev] [PATCH ovn v3 3/3] ovn-controller: Fix incremental processing for logical port references.
On Wed, Jun 23, 2021 at 7:48 AM Numan Siddique wrote:
>
> On Mon, Jun 21, 2021 at 2:52 AM 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
>
> Hi Han,
>
> Thanks for fixing these issues. I've a few questions. I haven't
> reviewed the patch completely.
>
>
> > ---
> > controller/lflow.c | 63 ++---
> > controller/lflow.h | 3 ++
> > controller/ovn-controller.c | 35 -
> > include/ovn/expr.h | 2 +-
> > lib/expr.c | 14 +++--
> > tests/ovn.at| 47 +++
> > tests/test-ovn.c| 4 +--
> > utilities/ovn-trace.c | 2 +-
> > 8 files changed, 132 insertions(+), 38 deletions(-)
> >
> > diff --git a/controller/lflow.c b/controller/lflow.c
> > index 34eca135a..b7699a309 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,
> > + >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,
> > + _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,
> > - _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,
> > - >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,
> > @@ -805,7 +810,6 @@ consider_logical_flow__(const struct
sbrec_logical_flow *lflow,
> > struct hmap *matches = NULL;
> > size_t matches_size = 0;
> >
> > -bool is_cr_cond_present = false;
> > bool pg_addr_set_ref = false;
> > uint32_t n_conjs = 0;
> >
> > @@
Re: [ovs-dev] [PATCH ovn v3 3/3] ovn-controller: Fix incremental processing for logical port references.
On Mon, Jun 21, 2021 at 2:52 AM 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
Hi Han,
Thanks for fixing these issues. I've a few questions. I haven't
reviewed the patch completely.
> ---
> controller/lflow.c | 63 ++---
> controller/lflow.h | 3 ++
> controller/ovn-controller.c | 35 -
> include/ovn/expr.h | 2 +-
> lib/expr.c | 14 +++--
> tests/ovn.at| 47 +++
> tests/test-ovn.c| 4 +--
> utilities/ovn-trace.c | 2 +-
> 8 files changed, 132 insertions(+), 38 deletions(-)
>
> diff --git a/controller/lflow.c b/controller/lflow.c
> index 34eca135a..b7699a309 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,
> + >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,
> + _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,
> - _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,
> - >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,
> @@ -805,7 +810,6 @@ consider_logical_flow__(const struct sbrec_logical_flow
> *lflow,
> struct hmap *matches = NULL;
> size_t matches_size = 0;
>
> -bool is_cr_cond_present = false;
> bool pg_addr_set_ref = false;
> uint32_t n_conjs = 0;
>
> @@ -843,8 +847,8 @@ consider_logical_flow__(const struct sbrec_logical_flow
> *lflow,
> case LCACHE_T_NONE:
> case LCACHE_T_CONJ_ID:
> case LCACHE_T_EXPR:
> -expr = expr_evaluate_condition(expr, is_chassis_resident_cb,
>
