Re: [ovs-dev] [PATCH ovn v3 08/16] northd: Refactor lflow management into a separate module.
On Wed, Dec 13, 2023 at 7:09 AM Dumitru Ceara wrote:
>
> On 11/28/23 03:36, num...@ovn.org wrote:
>
> [...]
>
> > +
> > +/* lflow ref mgr */
> > +struct lflow_ref {
> > +char *res_name;
> > +
> > +/* head of the list 'struct lflow_ref_node'. */
> > +struct ovs_list lflows_ref_list;
> > +
> > +/* hmapx_node is 'struct lflow *'. This is used to ensure
> > + * that there are no duplicates in 'lflow_ref_list' above. */
> > +struct hmapx lflows;
> > +};
> > +
> > +struct lflow_ref_node {
> > +struct ovs_list ref_list_node;
> > +
> > +struct ovn_lflow *lflow;
> > +size_t dp_index;
> > +};
> > +
> > +struct lflow_ref *
> > +lflow_ref_alloc(const char *res_name)
> > +{
>
> This does more than just allocating the lflow_ref. For consistency this
> should be called lflow_ref_create(..).
Ack. I'll handle it in v4.
>
> > +struct lflow_ref *lflow_ref = xzalloc(sizeof *lflow_ref);
> > +lflow_ref->res_name = xstrdup(res_name);
> > +ovs_list_init(&lflow_ref->lflows_ref_list);
> > +hmapx_init(&lflow_ref->lflows);
> > +return lflow_ref;
> > +}
> > +
> > +void
> > +lflow_ref_destroy(struct lflow_ref *lflow_ref)
> > +{
> > +free(lflow_ref->res_name);
> > +
> > +struct lflow_ref_node *l;
> > +LIST_FOR_EACH_SAFE (l, ref_list_node, &lflow_ref->lflows_ref_list) {
> > +ovs_list_remove(&l->ref_list_node);
> > +free(l);
> > +}
> > +
> > +hmapx_destroy(&lflow_ref->lflows);
> > +free(lflow_ref);
> > +}
> > +
> > +void
> > +lflow_ref_reset(struct lflow_ref *lflow_ref)
>
> IMO lflow_ref_clear() might be a better name.
>
> > +{
> > +struct lflow_ref_node *l;
> > +LIST_FOR_EACH_SAFE (l, ref_list_node, &lflow_ref->lflows_ref_list) {
> > +ovs_list_remove(&l->ref_list_node);
> > +free(l);
> > +}
> > +
> > +hmapx_clear(&lflow_ref->lflows);
> > +}
> > +
> > +void
> > +lflow_ref_clear_lflows(struct lflow_ref *lflow_ref)
>
> This doesn't clear, it just "unlinks" (clears all set datapath indices
> in the dp bitmap) flows. Should we remove the static
> unlink_lflows_from_datapath() function and inline it here and then call
> this lflow_ref_unlink_lflows()?
Ack. I'll handle it in v4.
>
> > +{
> > +unlink_lflows_from_datapath(lflow_ref);
> > +}
> > +
> > +void
> > +lflow_ref_clear_and_sync_lflows(struct lflow_ref *lflow_ref,
>
> I think a more accurate and concise name is lflow_ref_resync_flows().
Ack. I'll handle it in v4.
>
> > +struct lflow_table *lflow_table,
> > +struct ovsdb_idl_txn *ovnsb_txn,
> > +const struct ovn_datapaths *ls_datapaths,
> > +const struct ovn_datapaths *lr_datapaths,
> > +bool ovn_internal_version_changed,
> > +const struct sbrec_logical_flow_table *sbflow_table,
> > +const struct sbrec_logical_dp_group_table *dpgrp_table)
> > +{
> > +unlink_lflows_from_datapath(lflow_ref);
>
> This could be lflow_ref_unlink_lflows() instead.
Ack. I'll handle it in v4.
>
> > +lflow_ref_sync_lflows_to_sb__(lflow_ref, lflow_table, ovnsb_txn,
> > + ls_datapaths, lr_datapaths,
> > + ovn_internal_version_changed,
> > sbflow_table,
> > + dpgrp_table);
> > +}
> > +
> > +void
> > +lflow_ref_sync_lflows_to_sb(struct lflow_ref *lflow_ref,
>
> IMO it's enough to call this lflow_ref_sync_lflows(). It's implied that
> we're syncing to the SB.
Ack. I'll handle it in v4.
>
> > + struct lflow_table *lflow_table,
> > + struct ovsdb_idl_txn *ovnsb_txn,
> > + const struct ovn_datapaths *ls_datapaths,
> > + const struct ovn_datapaths *lr_datapaths,
> > + bool ovn_internal_version_changed,
> > + const struct sbrec_logical_flow_table *sbflow_table,
> > + const struct sbrec_logical_dp_group_table
> > *dpgrp_table)
> > +{
> > +lflow_ref_sync_lflows_to_sb__(lflow_ref, lflow_table, ovnsb_txn,
> > + ls_datapaths, lr_datapaths,
> > + ovn_internal_version_changed,
> > sbflow_table,
> > + dpgrp_table);
> > +}
> > +
>
> [...]
>
> > +
> > +static void
> > +lflow_ref_sync_lflows_to_sb__(struct lflow_ref *lflow_ref,
> > +struct lflow_table *lflow_table,
> > +struct ovsdb_idl_txn *ovnsb_txn,
> > +const struct ovn_datapaths *ls_datapaths,
> > +const struct ovn_datapaths *lr_datapaths,
> > +bool ovn_internal_version_changed,
> > +const struct sbrec_logical_flow_table
> > *sbflow_table,
> > +const struct sbrec_logical_dp_group_table
> > *dpgrp_table)
>
> IMO lflow_ref_sync_lflo
Re: [ovs-dev] [PATCH ovn v3 08/16] northd: Refactor lflow management into a separate module.
On 11/28/23 03:36, num...@ovn.org wrote:
[...]
> +
> +/* lflow ref mgr */
> +struct lflow_ref {
> +char *res_name;
> +
> +/* head of the list 'struct lflow_ref_node'. */
> +struct ovs_list lflows_ref_list;
> +
> +/* hmapx_node is 'struct lflow *'. This is used to ensure
> + * that there are no duplicates in 'lflow_ref_list' above. */
> +struct hmapx lflows;
> +};
> +
> +struct lflow_ref_node {
> +struct ovs_list ref_list_node;
> +
> +struct ovn_lflow *lflow;
> +size_t dp_index;
> +};
> +
> +struct lflow_ref *
> +lflow_ref_alloc(const char *res_name)
> +{
This does more than just allocating the lflow_ref. For consistency this
should be called lflow_ref_create(..).
> +struct lflow_ref *lflow_ref = xzalloc(sizeof *lflow_ref);
> +lflow_ref->res_name = xstrdup(res_name);
> +ovs_list_init(&lflow_ref->lflows_ref_list);
> +hmapx_init(&lflow_ref->lflows);
> +return lflow_ref;
> +}
> +
> +void
> +lflow_ref_destroy(struct lflow_ref *lflow_ref)
> +{
> +free(lflow_ref->res_name);
> +
> +struct lflow_ref_node *l;
> +LIST_FOR_EACH_SAFE (l, ref_list_node, &lflow_ref->lflows_ref_list) {
> +ovs_list_remove(&l->ref_list_node);
> +free(l);
> +}
> +
> +hmapx_destroy(&lflow_ref->lflows);
> +free(lflow_ref);
> +}
> +
> +void
> +lflow_ref_reset(struct lflow_ref *lflow_ref)
IMO lflow_ref_clear() might be a better name.
> +{
> +struct lflow_ref_node *l;
> +LIST_FOR_EACH_SAFE (l, ref_list_node, &lflow_ref->lflows_ref_list) {
> +ovs_list_remove(&l->ref_list_node);
> +free(l);
> +}
> +
> +hmapx_clear(&lflow_ref->lflows);
> +}
> +
> +void
> +lflow_ref_clear_lflows(struct lflow_ref *lflow_ref)
This doesn't clear, it just "unlinks" (clears all set datapath indices
in the dp bitmap) flows. Should we remove the static
unlink_lflows_from_datapath() function and inline it here and then call
this lflow_ref_unlink_lflows()?
> +{
> +unlink_lflows_from_datapath(lflow_ref);
> +}
> +
> +void
> +lflow_ref_clear_and_sync_lflows(struct lflow_ref *lflow_ref,
I think a more accurate and concise name is lflow_ref_resync_flows().
> +struct lflow_table *lflow_table,
> +struct ovsdb_idl_txn *ovnsb_txn,
> +const struct ovn_datapaths *ls_datapaths,
> +const struct ovn_datapaths *lr_datapaths,
> +bool ovn_internal_version_changed,
> +const struct sbrec_logical_flow_table *sbflow_table,
> +const struct sbrec_logical_dp_group_table *dpgrp_table)
> +{
> +unlink_lflows_from_datapath(lflow_ref);
This could be lflow_ref_unlink_lflows() instead.
> +lflow_ref_sync_lflows_to_sb__(lflow_ref, lflow_table, ovnsb_txn,
> + ls_datapaths, lr_datapaths,
> + ovn_internal_version_changed, sbflow_table,
> + dpgrp_table);
> +}
> +
> +void
> +lflow_ref_sync_lflows_to_sb(struct lflow_ref *lflow_ref,
IMO it's enough to call this lflow_ref_sync_lflows(). It's implied that
we're syncing to the SB.
> + struct lflow_table *lflow_table,
> + struct ovsdb_idl_txn *ovnsb_txn,
> + const struct ovn_datapaths *ls_datapaths,
> + const struct ovn_datapaths *lr_datapaths,
> + bool ovn_internal_version_changed,
> + const struct sbrec_logical_flow_table *sbflow_table,
> + const struct sbrec_logical_dp_group_table *dpgrp_table)
> +{
> +lflow_ref_sync_lflows_to_sb__(lflow_ref, lflow_table, ovnsb_txn,
> + ls_datapaths, lr_datapaths,
> + ovn_internal_version_changed, sbflow_table,
> + dpgrp_table);
> +}
> +
[...]
> +
> +static void
> +lflow_ref_sync_lflows_to_sb__(struct lflow_ref *lflow_ref,
> +struct lflow_table *lflow_table,
> +struct ovsdb_idl_txn *ovnsb_txn,
> +const struct ovn_datapaths *ls_datapaths,
> +const struct ovn_datapaths *lr_datapaths,
> +bool ovn_internal_version_changed,
> +const struct sbrec_logical_flow_table *sbflow_table,
> +const struct sbrec_logical_dp_group_table
> *dpgrp_table)
IMO lflow_ref_sync_lflows__() is explicit enough as a name.
Regards,
Dumitru
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn v3 08/16] northd: Refactor lflow management into a separate module.
On 11/28/23 03:36, num...@ovn.org wrote:
> From: Numan Siddique
>
> ovn_lflow_add() and other related functions/macros are now moved
> into a separate module - lflow-mgr.c. This module maintains a
> table 'struct lflow_table' for the logical flows. lflow table
> maintains a hmap to store the logical flows.
>
> It also maintains the logical switch and router dp groups.
>
> Previous commits which added lflow incremental processing for
> the VIF logical ports, stored the references to
> the logical ports' lflows using 'struct lflow_ref_list'. This
> struct is renamed to 'struct lflow_ref' and is part of lflow-mgr.c.
> It is modified a bit to store the resource to lflow references.
>
> Example usage of 'struct lflow_ref'.
>
> 'struct ovn_port' maintains 2 instances of lflow_ref. i,e
>
> struct ovn_port {
>...
>...
>struct lflow_ref *lflow_ref;
>struct lflow_ref *stateful_lflow_ref;
> };
>
> All the logical flows generated by
> build_lswitch_and_lrouter_iterate_by_lsp() uses the ovn_port->lflow_ref.
>
> All the logical flows generated by build_lsp_lflows_for_lbnats()
> uses the ovn_port->stateful_lflow_ref.
>
> When handling the ovn_port changes incrementally, the lflows referenced
> in 'struct ovn_port' are cleared and regenerated and synced to the
> SB logical flows.
>
> eg.
>
> lflow_ref_clear_lflows(op->lflow_ref);
> build_lswitch_and_lrouter_iterate_by_lsp(op, ...);
> lflow_ref_sync_lflows_to_sb(op->lflow_ref, ...);
>
> This patch does few more changes:
> - Logical flows are now hashed without the logical
> datapaths. If a logical flow is referenced by just one
> datapath, we don't rehash it.
>
> - The synthetic 'hash' column of sbrec_logical_flow now
> doesn't use the logical datapath. This means that
> when ovn-northd is updated/upgraded and has this commit,
> all the logical flows with 'logical_datapath' column
> set will get deleted and re-added causing some disruptions.
>
> - With the commit [1] which added I-P support for logical
> port changes, multiple logical flows with same match 'M'
> and actions 'A' are generated and stored without the
> dp groups, which was not the case prior to
> that patch.
> One example to generate these lflows is:
> ovn-nbctl lsp-set-addresses sw0p1 "MAC1 IP1"
> ovn-nbctl lsp-set-addresses sw1p1 "MAC1 IP1"
>ovn-nbctl lsp-set-addresses sw2p1 "MAC1 IP1"
>
> Now with this patch we go back to the earlier way. i.e
> one logical flow with logical_dp_groups set.
>
> - With this patch any updates to a logical port which
> doesn't result in new logical flows will not result in
> deletion and addition of same logical flows.
> Eg.
> ovn-nbctl set logical_switch_port sw0p1 external_ids:foo=bar
> will be a no-op to the SB logical flow table.
>
> [1] - 8bbd678("northd: Incremental processing of VIF additions in 'lflow'
> node.")
>
> Signed-off-by: Numan Siddique
> ---
Recheck-request: github-robot-_Build_and_Test
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn v3 08/16] northd: Refactor lflow management into a separate module.
Bleep bloop. Greetings Numan Siddique, I am a robot and I have tried out your patch. Thanks for your contribution. I encountered some error that I wasn't expecting. See the details below. checkpatch: WARNING: Line has non-spaces leading whitespace WARNING: Line has trailing whitespace #4484 FILE: northd/northd.c:15961: WARNING: Line is 82 characters long (recommended limit is 79) #4881 FILE: northd/northd.c:16067: lflow_ref_sync_lflows_to_sb(op->stateful_lflow_ref, lflows, ovnsb_txn, Lines checked: 5222, Warnings: 3, Errors: 0 Please check this out. If you feel there has been an error, please email acon...@redhat.com Thanks, 0-day Robot ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
