Re: [ovs-dev] [PATCH ovn 5/5] northd: Add incremental processing for NB port groups.
On 8/24/23 08:18, Han Zhou wrote:
> On Thu, Aug 10, 2023 at 5:45 AM Dumitru Ceara wrote:
>>
>> It's similar to the processing we do for address sets. There's a bit
>> more mechanics involved due to the fact that we need to split NB port
>> groups per datapath.
>>
>> We currently only partially implement incremental processing of
>> port_group changes in the lflow node. That is, we deal with the case
>> when the sets of "switches per port group" doesn't change. In that
>> specific case ACL lflows don't need to be reprocessed.
>>
>> In a synthetic benchmark that created (in this order):
>> - 500 switches
>> - 2000 port groups
>> - 4 ACLs per port group
>> - 1 ports distributed equally between the switches and port groups
>>
>> we measured the following ovn-northd CPU usage:
>>
>> +-+++
>> | Incremental processing? | --wait=sb? | northd avg cpu (%) |
>> +-+++
>> | N | Y |84.2|
>> +-+++
>> | Y | Y |41.5|
>> +-+++
>> | N | N |93.2|
>> +-+++
>> | Y | N |53.6|
>> +-+++
>>
>> where '--wait=sb' set to 'Y' means the benchmark was waiting for the
>> port and port group operations to be propagated to the Southbound DB
>> before continuing to the next operation.
>>
>> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2228162
>> Signed-off-by: Dumitru Ceara
>
> Thanks Dumitru for the improvement! Please see my comments below.
>
Hi Han,
Thanks for the review!
>> ---
>> northd/en-lflow.c| 17 ++
>> northd/en-lflow.h|1
>> northd/en-port-group.c | 451
> --
>> northd/en-port-group.h | 36 +++-
>> northd/inc-proc-northd.c | 13 +
>> northd/ovn-northd.c |4
>> tests/ovn-northd.at | 246 +
>> 7 files changed, 708 insertions(+), 60 deletions(-)
>>
>> diff --git a/northd/en-lflow.c b/northd/en-lflow.c
>> index 7f6a7872b2..1321f79036 100644
>> --- a/northd/en-lflow.c
>> +++ b/northd/en-lflow.c
>> @@ -119,6 +119,23 @@ lflow_northd_handler(struct engine_node *node,
>> return true;
>> }
>>
>> +bool
>> +lflow_port_group_handler(struct engine_node *node, void *data OVS_UNUSED)
>> +{
>> +struct port_group_data *pg_data =
>> +engine_get_input_data("port_group", node);
>> +
>> +/* If the set of switches per port group didn't change then there's
> no
>> + * need to reprocess lflows. Otherwise, there might be a need to add
>> + * port-group ACLs to new switches. */
>
> To be more accurate, the comment should say "there might be a need to
> add/delete port-group ACLs to/from switches."
>
Ack.
>> +if (!pg_data->ls_port_groups_sets_unchanged) {
>
> nit: it would be a little more natural to name the field as
> ls_port_groups_sets_changed and use positive check in the if condition.
>
Sure, I changed it like that.
>> +return false;
>> +}
>> +
>> +engine_set_node_state(node, EN_UPDATED);
>> +return true;
>> +}
>> +
>> void *en_lflow_init(struct engine_node *node OVS_UNUSED,
>> struct engine_arg *arg OVS_UNUSED)
>> {
>> diff --git a/northd/en-lflow.h b/northd/en-lflow.h
>> index 5e3fbc25e3..5417b2faff 100644
>> --- a/northd/en-lflow.h
>> +++ b/northd/en-lflow.h
>> @@ -13,5 +13,6 @@ void en_lflow_run(struct engine_node *node, void *data);
>> void *en_lflow_init(struct engine_node *node, struct engine_arg *arg);
>> void en_lflow_cleanup(void *data);
>> bool lflow_northd_handler(struct engine_node *, void *data);
>> +bool lflow_port_group_handler(struct engine_node *, void *data);
>>
>> #endif /* EN_LFLOW_H */
>> diff --git a/northd/en-port-group.c b/northd/en-port-group.c
>> index 2c36410246..6902695a01 100644
>> --- a/northd/en-port-group.c
>> +++ b/northd/en-port-group.c
>> @@ -33,15 +33,46 @@ static struct ls_port_group *ls_port_group_create(
>> static void ls_port_group_destroy(struct ls_port_group_table *,
>>struct ls_port_group *);
>>
>> +static bool ls_port_group_process(
>> +struct ls_port_group_table *,
>> +struct port_group_to_ls_table *,
>> +const struct hmap *ls_ports,
>> +const struct nbrec_port_group *,
>> +struct hmapx *updated_ls_port_groups
>> +);
>
> nit: It would be better to use the same coding style as the below
> prototype, i.e. don't put the last ");" in a separate line.
>
OK.
>> +
>> +static void ls_port_group_record_clear(
>> +struct ls_port_group_table *,
>> +struct port_group_to_ls *,
>> +st
Re: [ovs-dev] [PATCH ovn 5/5] northd: Add incremental processing for NB port groups.
On 8/22/23 08:58, Ales Musil wrote:
> On Thu, Aug 10, 2023 at 2:45 PM Dumitru Ceara wrote:
>
>> It's similar to the processing we do for address sets. There's a bit
>> more mechanics involved due to the fact that we need to split NB port
>> groups per datapath.
>>
>> We currently only partially implement incremental processing of
>> port_group changes in the lflow node. That is, we deal with the case
>> when the sets of "switches per port group" doesn't change. In that
>> specific case ACL lflows don't need to be reprocessed.
>>
>> In a synthetic benchmark that created (in this order):
>> - 500 switches
>> - 2000 port groups
>> - 4 ACLs per port group
>> - 1 ports distributed equally between the switches and port groups
>>
>> we measured the following ovn-northd CPU usage:
>>
>> +-+++
>> | Incremental processing? | --wait=sb? | northd avg cpu (%) |
>> +-+++
>> | N | Y |84.2|
>> +-+++
>> | Y | Y |41.5|
>> +-+++
>> | N | N |93.2|
>> +-+++
>> | Y | N |53.6|
>> +-+++
>>
>> where '--wait=sb' set to 'Y' means the benchmark was waiting for the
>> port and port group operations to be propagated to the Southbound DB
>> before continuing to the next operation.
>>
>> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2228162
>> Signed-off-by: Dumitru Ceara
>> ---
>>
>
> Hi Dumitru,
>
> I have a couple of comments down below.
>
Hi Ales,
Thanks for the review!
[..]
>> static struct ls_port_group_record *
>> ls_port_group_record_add(struct ls_port_group *ls_pg,
>> const struct nbrec_port_group *nb_pg,
>> const char *port_name)
>> {
>> -struct ls_port_group_record *ls_pg_rec = NULL;
>> +struct ls_port_group_record *ls_pg_rec =
>> +ls_port_group_record_find(ls_pg, nb_pg);
>> size_t hash = uuid_hash(&nb_pg->header_.uuid);
>>
>> -HMAP_FOR_EACH_WITH_HASH (ls_pg_rec, key_node, hash, &ls_pg->nb_pgs) {
>> -if (ls_pg_rec->nb_pg == nb_pg) {
>> -goto done;
>> -}
>> +if (!ls_pg_rec) {
>> +ls_pg_rec = xzalloc(sizeof *ls_pg_rec);
>>
>
> nit: No need for zeroed alloc as all the fields are immediately overwritten.
>
Fair, I'll change it to xmalloc().
> +*ls_pg_rec = (struct ls_port_group_record) {
>> +.nb_pg = nb_pg,
>> +.ports = SSET_INITIALIZER(&ls_pg_rec->ports),
>> +};
>> +hmap_insert(&ls_pg->nb_pgs, &ls_pg_rec->key_node, hash);
>> }
>>
>> -ls_pg_rec = xzalloc(sizeof *ls_pg_rec);
>> -*ls_pg_rec = (struct ls_port_group_record) {
>> -.nb_pg = nb_pg,
>> -.ports = SSET_INITIALIZER(&ls_pg_rec->ports),
>> -};
>> -hmap_insert(&ls_pg->nb_pgs, &ls_pg_rec->key_node, hash);
>> -done:
>> sset_add(&ls_pg_rec->ports, port_name);
>> return ls_pg_rec;
>> }
>>
>> +static struct ls_port_group_record *
>> +ls_port_group_record_find(struct ls_port_group *ls_pg,
>> + const struct nbrec_port_group *nb_pg)
>> +{
>> +size_t hash = uuid_hash(&nb_pg->header_.uuid);
>> +struct ls_port_group_record *ls_pg_rec;
>> +
>> +HMAP_FOR_EACH_WITH_HASH (ls_pg_rec, key_node, hash, &ls_pg->nb_pgs) {
>> +if (ls_pg_rec->nb_pg == nb_pg) {
>> +return ls_pg_rec;
>> +}
>> +}
>> +return NULL;
>> +}
>> +
>> +
>> static void
>> ls_port_group_record_destroy(struct ls_port_group *ls_pg,
>> struct ls_port_group_record *ls_pg_rec)
>> @@ -237,6 +372,71 @@ ls_port_group_record_destroy(struct ls_port_group
>> *ls_pg,
>> }
>> }
>>
>> +void
>> +port_group_to_ls_table_init(struct port_group_to_ls_table *table)
>> +{
>> +*table = (struct port_group_to_ls_table) {
>> +.entries = HMAP_INITIALIZER(&table->entries),
>> +};
>> +}
>> +
>> +void
>> +port_group_to_ls_table_clear(struct port_group_to_ls_table *table)
>> +{
>> +struct port_group_to_ls *pg_ls;
>> +HMAP_FOR_EACH_SAFE (pg_ls, key_node, &table->entries) {
>> +port_group_to_ls_destroy(table, pg_ls);
>> +}
>> +}
>> +
>> +void
>> +port_group_to_ls_table_destroy(struct port_group_to_ls_table *table)
>> +{
>> +port_group_to_ls_table_clear(table);
>> +hmap_destroy(&table->entries);
>> +}
>> +
>> +struct port_group_to_ls *
>> +port_group_to_ls_table_find(const struct port_group_to_ls_table *table,
>> +const struct nbrec_port_group *nb_pg)
>> +{
>> +struct port_group_to_ls *pg_ls;
>> +
>
Re: [ovs-dev] [PATCH ovn 5/5] northd: Add incremental processing for NB port groups.
On Thu, Aug 10, 2023 at 5:45 AM Dumitru Ceara wrote:
>
> It's similar to the processing we do for address sets. There's a bit
> more mechanics involved due to the fact that we need to split NB port
> groups per datapath.
>
> We currently only partially implement incremental processing of
> port_group changes in the lflow node. That is, we deal with the case
> when the sets of "switches per port group" doesn't change. In that
> specific case ACL lflows don't need to be reprocessed.
>
> In a synthetic benchmark that created (in this order):
> - 500 switches
> - 2000 port groups
> - 4 ACLs per port group
> - 1 ports distributed equally between the switches and port groups
>
> we measured the following ovn-northd CPU usage:
>
> +-+++
> | Incremental processing? | --wait=sb? | northd avg cpu (%) |
> +-+++
> | N | Y |84.2|
> +-+++
> | Y | Y |41.5|
> +-+++
> | N | N |93.2|
> +-+++
> | Y | N |53.6|
> +-+++
>
> where '--wait=sb' set to 'Y' means the benchmark was waiting for the
> port and port group operations to be propagated to the Southbound DB
> before continuing to the next operation.
>
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2228162
> Signed-off-by: Dumitru Ceara
Thanks Dumitru for the improvement! Please see my comments below.
> ---
> northd/en-lflow.c| 17 ++
> northd/en-lflow.h|1
> northd/en-port-group.c | 451
--
> northd/en-port-group.h | 36 +++-
> northd/inc-proc-northd.c | 13 +
> northd/ovn-northd.c |4
> tests/ovn-northd.at | 246 +
> 7 files changed, 708 insertions(+), 60 deletions(-)
>
> diff --git a/northd/en-lflow.c b/northd/en-lflow.c
> index 7f6a7872b2..1321f79036 100644
> --- a/northd/en-lflow.c
> +++ b/northd/en-lflow.c
> @@ -119,6 +119,23 @@ lflow_northd_handler(struct engine_node *node,
> return true;
> }
>
> +bool
> +lflow_port_group_handler(struct engine_node *node, void *data OVS_UNUSED)
> +{
> +struct port_group_data *pg_data =
> +engine_get_input_data("port_group", node);
> +
> +/* If the set of switches per port group didn't change then there's
no
> + * need to reprocess lflows. Otherwise, there might be a need to add
> + * port-group ACLs to new switches. */
To be more accurate, the comment should say "there might be a need to
add/delete port-group ACLs to/from switches."
> +if (!pg_data->ls_port_groups_sets_unchanged) {
nit: it would be a little more natural to name the field as
ls_port_groups_sets_changed and use positive check in the if condition.
> +return false;
> +}
> +
> +engine_set_node_state(node, EN_UPDATED);
> +return true;
> +}
> +
> void *en_lflow_init(struct engine_node *node OVS_UNUSED,
> struct engine_arg *arg OVS_UNUSED)
> {
> diff --git a/northd/en-lflow.h b/northd/en-lflow.h
> index 5e3fbc25e3..5417b2faff 100644
> --- a/northd/en-lflow.h
> +++ b/northd/en-lflow.h
> @@ -13,5 +13,6 @@ void en_lflow_run(struct engine_node *node, void *data);
> void *en_lflow_init(struct engine_node *node, struct engine_arg *arg);
> void en_lflow_cleanup(void *data);
> bool lflow_northd_handler(struct engine_node *, void *data);
> +bool lflow_port_group_handler(struct engine_node *, void *data);
>
> #endif /* EN_LFLOW_H */
> diff --git a/northd/en-port-group.c b/northd/en-port-group.c
> index 2c36410246..6902695a01 100644
> --- a/northd/en-port-group.c
> +++ b/northd/en-port-group.c
> @@ -33,15 +33,46 @@ static struct ls_port_group *ls_port_group_create(
> static void ls_port_group_destroy(struct ls_port_group_table *,
>struct ls_port_group *);
>
> +static bool ls_port_group_process(
> +struct ls_port_group_table *,
> +struct port_group_to_ls_table *,
> +const struct hmap *ls_ports,
> +const struct nbrec_port_group *,
> +struct hmapx *updated_ls_port_groups
> +);
nit: It would be better to use the same coding style as the below
prototype, i.e. don't put the last ");" in a separate line.
> +
> +static void ls_port_group_record_clear(
> +struct ls_port_group_table *,
> +struct port_group_to_ls *,
> +struct hmapx *updated_ls_port_groups);
> +static void ls_port_group_record_prune(struct ls_port_group *);
> +
> static struct ls_port_group_record *ls_port_group_record_add(
> struct ls_port_group *,
> const struct nbrec_port_group *,
> con
Re: [ovs-dev] [PATCH ovn 5/5] northd: Add incremental processing for NB port groups.
On Thu, Aug 10, 2023 at 2:45 PM Dumitru Ceara wrote:
> It's similar to the processing we do for address sets. There's a bit
> more mechanics involved due to the fact that we need to split NB port
> groups per datapath.
>
> We currently only partially implement incremental processing of
> port_group changes in the lflow node. That is, we deal with the case
> when the sets of "switches per port group" doesn't change. In that
> specific case ACL lflows don't need to be reprocessed.
>
> In a synthetic benchmark that created (in this order):
> - 500 switches
> - 2000 port groups
> - 4 ACLs per port group
> - 1 ports distributed equally between the switches and port groups
>
> we measured the following ovn-northd CPU usage:
>
> +-+++
> | Incremental processing? | --wait=sb? | northd avg cpu (%) |
> +-+++
> | N | Y |84.2|
> +-+++
> | Y | Y |41.5|
> +-+++
> | N | N |93.2|
> +-+++
> | Y | N |53.6|
> +-+++
>
> where '--wait=sb' set to 'Y' means the benchmark was waiting for the
> port and port group operations to be propagated to the Southbound DB
> before continuing to the next operation.
>
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2228162
> Signed-off-by: Dumitru Ceara
> ---
>
Hi Dumitru,
I have a couple of comments down below.
northd/en-lflow.c| 17 ++
> northd/en-lflow.h|1
> northd/en-port-group.c | 451
> --
> northd/en-port-group.h | 36 +++-
> northd/inc-proc-northd.c | 13 +
> northd/ovn-northd.c |4
> tests/ovn-northd.at | 246 +
> 7 files changed, 708 insertions(+), 60 deletions(-)
>
> diff --git a/northd/en-lflow.c b/northd/en-lflow.c
> index 7f6a7872b2..1321f79036 100644
> --- a/northd/en-lflow.c
> +++ b/northd/en-lflow.c
> @@ -119,6 +119,23 @@ lflow_northd_handler(struct engine_node *node,
> return true;
> }
>
> +bool
> +lflow_port_group_handler(struct engine_node *node, void *data OVS_UNUSED)
> +{
> +struct port_group_data *pg_data =
> +engine_get_input_data("port_group", node);
> +
> +/* If the set of switches per port group didn't change then there's no
> + * need to reprocess lflows. Otherwise, there might be a need to add
> + * port-group ACLs to new switches. */
> +if (!pg_data->ls_port_groups_sets_unchanged) {
> +return false;
> +}
> +
> +engine_set_node_state(node, EN_UPDATED);
> +return true;
> +}
> +
> void *en_lflow_init(struct engine_node *node OVS_UNUSED,
> struct engine_arg *arg OVS_UNUSED)
> {
> diff --git a/northd/en-lflow.h b/northd/en-lflow.h
> index 5e3fbc25e3..5417b2faff 100644
> --- a/northd/en-lflow.h
> +++ b/northd/en-lflow.h
> @@ -13,5 +13,6 @@ void en_lflow_run(struct engine_node *node, void *data);
> void *en_lflow_init(struct engine_node *node, struct engine_arg *arg);
> void en_lflow_cleanup(void *data);
> bool lflow_northd_handler(struct engine_node *, void *data);
> +bool lflow_port_group_handler(struct engine_node *, void *data);
>
> #endif /* EN_LFLOW_H */
> diff --git a/northd/en-port-group.c b/northd/en-port-group.c
> index 2c36410246..6902695a01 100644
> --- a/northd/en-port-group.c
> +++ b/northd/en-port-group.c
> @@ -33,15 +33,46 @@ static struct ls_port_group *ls_port_group_create(
> static void ls_port_group_destroy(struct ls_port_group_table *,
>struct ls_port_group *);
>
> +static bool ls_port_group_process(
> +struct ls_port_group_table *,
> +struct port_group_to_ls_table *,
> +const struct hmap *ls_ports,
> +const struct nbrec_port_group *,
> +struct hmapx *updated_ls_port_groups
> +);
> +
> +static void ls_port_group_record_clear(
> +struct ls_port_group_table *,
> +struct port_group_to_ls *,
> +struct hmapx *updated_ls_port_groups);
> +static void ls_port_group_record_prune(struct ls_port_group *);
> +
> static struct ls_port_group_record *ls_port_group_record_add(
> struct ls_port_group *,
> const struct nbrec_port_group *,
> const char *port_name);
>
> +static struct ls_port_group_record *ls_port_group_record_find(
> +struct ls_port_group *, const struct nbrec_port_group *nb_pg);
> +
> static void ls_port_group_record_destroy(
> struct ls_port_group *,
> struct ls_port_group_record *);
>
> +static struct port_group_to_ls *port_group_to_ls_create(
> +struct port_group_to_ls_table *,
> +const
[ovs-dev] [PATCH ovn 5/5] northd: Add incremental processing for NB port groups.
It's similar to the processing we do for address sets. There's a bit
more mechanics involved due to the fact that we need to split NB port
groups per datapath.
We currently only partially implement incremental processing of
port_group changes in the lflow node. That is, we deal with the case
when the sets of "switches per port group" doesn't change. In that
specific case ACL lflows don't need to be reprocessed.
In a synthetic benchmark that created (in this order):
- 500 switches
- 2000 port groups
- 4 ACLs per port group
- 1 ports distributed equally between the switches and port groups
we measured the following ovn-northd CPU usage:
+-+++
| Incremental processing? | --wait=sb? | northd avg cpu (%) |
+-+++
| N | Y |84.2|
+-+++
| Y | Y |41.5|
+-+++
| N | N |93.2|
+-+++
| Y | N |53.6|
+-+++
where '--wait=sb' set to 'Y' means the benchmark was waiting for the
port and port group operations to be propagated to the Southbound DB
before continuing to the next operation.
Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2228162
Signed-off-by: Dumitru Ceara
---
northd/en-lflow.c| 17 ++
northd/en-lflow.h|1
northd/en-port-group.c | 451 --
northd/en-port-group.h | 36 +++-
northd/inc-proc-northd.c | 13 +
northd/ovn-northd.c |4
tests/ovn-northd.at | 246 +
7 files changed, 708 insertions(+), 60 deletions(-)
diff --git a/northd/en-lflow.c b/northd/en-lflow.c
index 7f6a7872b2..1321f79036 100644
--- a/northd/en-lflow.c
+++ b/northd/en-lflow.c
@@ -119,6 +119,23 @@ lflow_northd_handler(struct engine_node *node,
return true;
}
+bool
+lflow_port_group_handler(struct engine_node *node, void *data OVS_UNUSED)
+{
+struct port_group_data *pg_data =
+engine_get_input_data("port_group", node);
+
+/* If the set of switches per port group didn't change then there's no
+ * need to reprocess lflows. Otherwise, there might be a need to add
+ * port-group ACLs to new switches. */
+if (!pg_data->ls_port_groups_sets_unchanged) {
+return false;
+}
+
+engine_set_node_state(node, EN_UPDATED);
+return true;
+}
+
void *en_lflow_init(struct engine_node *node OVS_UNUSED,
struct engine_arg *arg OVS_UNUSED)
{
diff --git a/northd/en-lflow.h b/northd/en-lflow.h
index 5e3fbc25e3..5417b2faff 100644
--- a/northd/en-lflow.h
+++ b/northd/en-lflow.h
@@ -13,5 +13,6 @@ void en_lflow_run(struct engine_node *node, void *data);
void *en_lflow_init(struct engine_node *node, struct engine_arg *arg);
void en_lflow_cleanup(void *data);
bool lflow_northd_handler(struct engine_node *, void *data);
+bool lflow_port_group_handler(struct engine_node *, void *data);
#endif /* EN_LFLOW_H */
diff --git a/northd/en-port-group.c b/northd/en-port-group.c
index 2c36410246..6902695a01 100644
--- a/northd/en-port-group.c
+++ b/northd/en-port-group.c
@@ -33,15 +33,46 @@ static struct ls_port_group *ls_port_group_create(
static void ls_port_group_destroy(struct ls_port_group_table *,
struct ls_port_group *);
+static bool ls_port_group_process(
+struct ls_port_group_table *,
+struct port_group_to_ls_table *,
+const struct hmap *ls_ports,
+const struct nbrec_port_group *,
+struct hmapx *updated_ls_port_groups
+);
+
+static void ls_port_group_record_clear(
+struct ls_port_group_table *,
+struct port_group_to_ls *,
+struct hmapx *updated_ls_port_groups);
+static void ls_port_group_record_prune(struct ls_port_group *);
+
static struct ls_port_group_record *ls_port_group_record_add(
struct ls_port_group *,
const struct nbrec_port_group *,
const char *port_name);
+static struct ls_port_group_record *ls_port_group_record_find(
+struct ls_port_group *, const struct nbrec_port_group *nb_pg);
+
static void ls_port_group_record_destroy(
struct ls_port_group *,
struct ls_port_group_record *);
+static struct port_group_to_ls *port_group_to_ls_create(
+struct port_group_to_ls_table *,
+const struct nbrec_port_group *);
+static void port_group_to_ls_destroy(struct port_group_to_ls_table *,
+ struct port_group_to_ls *);
+
+static void update_sb_port_group(struct sorted_array *nb_ports,
+ const struct sbrec_port_group *sb_pg);
+static void sync_port_group(
