[ovs-dev] [PATCH ovn v5 01/16] ovn-northd: Move out Table 0 (ingress) operations to functions

Numan Siddique numans at ovn.org
Tue Sep 15 11:12:33 UTC 2020


On Fri, Sep 11, 2020 at 3:11 PM <anton.ivanov at cambridgegreys.com> wrote:

> From: Anton Ivanov <anton.ivanov at cambridgegreys.com>
>
> Signed-off-by: Anton Ivanov <anton.ivanov at cambridgegreys.com>
>

Hi Anton,

Thanks for this  series.

I found most of the patches to be splitting the code into functions into
proper logical blocks.
However, patches 3 to 7, split the code into functions which I think can be
further reorganized properly.
What I mean is that if we take ARP response flows for NAT entries as an
example, the
function build_lrouter_flows_ip_input_od() in the patch 3 adds ARP response
flows for some scenarios
and the function build_lrouter_flows_ipv4_input_table_3_op() in patch 4
adds ARP response flows
for other scenarios.

I think it's better to revisit patches 3 to 7 and move out the code into
functions which separates
the lflow generation properly.

I also think the function names are a bit odd and the naming can be
improved.

I found other patches in the series to be fine except for the function
naming.
Hence I reworked a bit renaming the functions and moving the comments from
the function
declarations to near the function definitions. I think this will be more
helpful.
And I applied the patches 1,2, 8 to 16 to master.

Please note I also removed the marker comment while committing the patch
16. I think we can remove it
as it's a bit odd to keep the comment.

Thanks
Numan


> ---
>  northd/ovn-northd.c | 135 +++++++++++++++++++++++++++-----------------
>  1 file changed, 82 insertions(+), 53 deletions(-)
>
> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> index b95d6cd8a..14e4a6939 100644
> --- a/northd/ovn-northd.c
> +++ b/northd/ovn-northd.c
> @@ -8482,6 +8482,26 @@ build_lrouter_force_snat_flows(struct hmap *lflows,
> struct ovn_datapath *od,
>      ds_destroy(&actions);
>  }
>
> +
> +/* Logical router ingress table 0: Admission control framework. */
> +static void
> +build_lrouter_flows_ingress_table_0_od(
> +        struct ovn_datapath *od, struct hmap *lflows);
> +
> +/* Logical router ingress table 0: match (priority 50). */
> +static void
> +build_lrouter_flows_ingress_table_0_op(
> +        struct ovn_port *op, struct hmap *lflows,
> +        struct ds *match, struct ds *actions);
> +
> +/*
> + * Do not remove this comment - it is here on purpose
> + * It serves as a marker so that pulling operations out
> + * of build_lrouter_flows results in a clean diff with
> + * a separate new operations function and clean changes
> + * to build_lroute_flows
> + */
> +
>  static void
>  build_lrouter_flows(struct hmap *datapaths, struct hmap *ports,
>                      struct hmap *lflows, struct shash *meter_groups,
> @@ -8493,65 +8513,14 @@ build_lrouter_flows(struct hmap *datapaths, struct
> hmap *ports,
>      struct ds match = DS_EMPTY_INITIALIZER;
>      struct ds actions = DS_EMPTY_INITIALIZER;
>
> -    /* Logical router ingress table 0: Admission control framework. */
>      struct ovn_datapath *od;
>      HMAP_FOR_EACH (od, key_node, datapaths) {
> -        if (!od->nbr) {
> -            continue;
> -        }
> -
> -        /* Logical VLANs not supported.
> -         * Broadcast/multicast source address is invalid. */
> -        ovn_lflow_add(lflows, od, S_ROUTER_IN_ADMISSION, 100,
> -                      "vlan.present || eth.src[40]", "drop;");
> +        build_lrouter_flows_ingress_table_0_od(od, lflows);
>      }
>
> -    /* Logical router ingress table 0: match (priority 50). */
>      struct ovn_port *op;
>      HMAP_FOR_EACH (op, key_node, ports) {
> -        if (!op->nbrp) {
> -            continue;
> -        }
> -
> -        if (!lrport_is_enabled(op->nbrp)) {
> -            /* Drop packets from disabled logical ports (since logical
> flow
> -             * tables are default-drop). */
> -            continue;
> -        }
> -
> -        if (op->derived) {
> -            /* No ingress packets should be received on a chassisredirect
> -             * port. */
> -            continue;
> -        }
> -
> -        /* Store the ethernet address of the port receiving the packet.
> -         * This will save us from having to match on inport further down
> in
> -         * the pipeline.
> -         */
> -        ds_clear(&actions);
> -        ds_put_format(&actions, REG_INPORT_ETH_ADDR " = %s; next;",
> -                      op->lrp_networks.ea_s);
> -
> -        ds_clear(&match);
> -        ds_put_format(&match, "eth.mcast && inport == %s", op->json_key);
> -        ovn_lflow_add_with_hint(lflows, op->od, S_ROUTER_IN_ADMISSION, 50,
> -                                ds_cstr(&match), ds_cstr(&actions),
> -                                &op->nbrp->header_);
> -
> -        ds_clear(&match);
> -        ds_put_format(&match, "eth.dst == %s && inport == %s",
> -                      op->lrp_networks.ea_s, op->json_key);
> -        if (op->od->l3dgw_port && op == op->od->l3dgw_port
> -            && op->od->l3redirect_port) {
> -            /* Traffic with eth.dst = l3dgw_port->lrp_networks.ea_s
> -             * should only be received on the "redirect-chassis". */
> -            ds_put_format(&match, " && is_chassis_resident(%s)",
> -                          op->od->l3redirect_port->json_key);
> -        }
> -        ovn_lflow_add_with_hint(lflows, op->od, S_ROUTER_IN_ADMISSION, 50,
> -                                ds_cstr(&match),  ds_cstr(&actions),
> -                                &op->nbrp->header_);
> +        build_lrouter_flows_ingress_table_0_op(op, lflows, &match,
> &actions);
>      }
>
>      /* Logical router ingress table 1: LOOKUP_NEIGHBOR and
> @@ -10956,6 +10925,66 @@ build_lrouter_flows(struct hmap *datapaths,
> struct hmap *ports,
>      ds_destroy(&actions);
>  }
>
> +static void
> +build_lrouter_flows_ingress_table_0_od(
> +        struct ovn_datapath *od, struct hmap *lflows)
> +{
> +    if (od->nbr) {
> +        /* Logical VLANs not supported.
> +         * Broadcast/multicast source address is invalid. */
> +        ovn_lflow_add(lflows, od, S_ROUTER_IN_ADMISSION, 100,
> +                      "vlan.present || eth.src[40]", "drop;");
> +    }
> +}
> +
> +static void
> +build_lrouter_flows_ingress_table_0_op(
> +        struct ovn_port *op, struct hmap *lflows,
> +        struct ds *match, struct ds *actions)
> +{
> +    if (op->nbrp) {
> +        if (!lrport_is_enabled(op->nbrp)) {
> +            /* Drop packets from disabled logical ports (since logical
> flow
> +             * tables are default-drop). */
> +            return;
> +        }
> +
> +        if (op->derived) {
> +            /* No ingress packets should be received on a chassisredirect
> +             * port. */
> +            return;
> +        }
> +
> +        /* Store the ethernet address of the port receiving the packet.
> +         * This will save us from having to match on inport further down
> in
> +         * the pipeline.
> +         */
> +        ds_clear(actions);
> +        ds_put_format(actions, REG_INPORT_ETH_ADDR " = %s; next;",
> +                      op->lrp_networks.ea_s);
> +
> +        ds_clear(match);
> +        ds_put_format(match, "eth.mcast && inport == %s", op->json_key);
> +        ovn_lflow_add_with_hint(lflows, op->od, S_ROUTER_IN_ADMISSION, 50,
> +                                ds_cstr(match), ds_cstr(actions),
> +                                &op->nbrp->header_);
> +
> +        ds_clear(match);
> +        ds_put_format(match, "eth.dst == %s && inport == %s",
> +                      op->lrp_networks.ea_s, op->json_key);
> +        if (op->od->l3dgw_port && op == op->od->l3dgw_port
> +            && op->od->l3redirect_port) {
> +            /* Traffic with eth.dst = l3dgw_port->lrp_networks.ea_s
> +             * should only be received on the "redirect-chassis". */
> +            ds_put_format(match, " && is_chassis_resident(%s)",
> +                          op->od->l3redirect_port->json_key);
> +        }
> +        ovn_lflow_add_with_hint(lflows, op->od, S_ROUTER_IN_ADMISSION, 50,
> +                                ds_cstr(match),  ds_cstr(actions),
> +                                &op->nbrp->header_);
> +    }
> +}
> +
>  /* Updates the Logical_Flow and Multicast_Group tables in the OVN_SB
> database,
>   * constructing their contents based on the OVN_NB database. */
>  static void
> --
> 2.20.1
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>


More information about the dev mailing list