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

Anton Ivanov anton.ivanov at cambridgegreys.com
Tue Sep 15 15:28:24 UTC 2020


On 15/09/2020 12:12, Numan Siddique wrote:
>
>
>
> On Fri, Sep 11, 2020 at 3:11 PM <anton.ivanov at cambridgegreys.com 
> <mailto:anton.ivanov at cambridgegreys.com>> wrote:
>
>     From: Anton Ivanov <anton.ivanov at cambridgegreys.com
>     <mailto:anton.ivanov at cambridgegreys.com>>
>
>     Signed-off-by: Anton Ivanov <anton.ivanov at cambridgegreys.com
>     <mailto: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.

Cool, thanks. I will rebase my branch off that.

>
> 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.

Cool.

I found that in its absence, diff generates stuff that is not readable 
and impossible to rebase even after minimal changes.

In the meantime, I reworked the actual multi-threaded processing patch 
so that it unifies lswitch and lrouter processing and runs the worker 
trheadpool once per iteration. I am going to re-test that - it should 
improve performance and scalability a bit further.

A.

>
> 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 <mailto:dev at openvswitch.org>
>     https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>

-- 
Anton R. Ivanov
Cambridgegreys Limited. Registered in England. Company Number 10273661
https://www.cambridgegreys.com/



More information about the dev mailing list